This revision was automatically updated to reflect the committed changes.
Closed by commit R286:ae3c38293421: increase test coverage of taglibwriter
(authored by astippich).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D15704?vs=42519=42521#toc
REPOSITORY
R286 KFileMetaData
CHANGES
bruns accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R286 KFileMetaData
BRANCH
taglib_write_mimetypes
REVISION DETAIL
https://phabricator.kde.org/D15704
To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov,
astippich updated this revision to Diff 42519.
astippich added a comment.
- remove untested mime types and sort
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15704?vs=42196=42519
BRANCH
taglib_write_mimetypes
REVISION DETAIL
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> taglibwritertest.cpp:74
> +
> +QTest::addRow("mp3")
> +<< QStringLiteral("mp3")
Can you sort the tests alphabetically? It does not matter much, but
astippich added inline comments.
INLINE COMMENTS
> svuorela wrote in taglibwriter.cpp:22
> Unrelated. but consider making this static ?
Never really though about that, but should be static, yes. None of the writers
and extractors I looked at currently do this, so if I find time I will do that
svuorela added a comment.
Looks good to me. A nice and simple way to drastically increase test coverage.
INLINE COMMENTS
> taglibwriter.cpp:22
> {
> QStringList types = {
> QStringLiteral("audio/mpeg"),
Unrelated. but consider making this static ?
REPOSITORY
R286
astippich added a dependent revision: D15714: add a string suffix to test data
and use for unicode testing of taglibwriter.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D15704
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov,
astippich updated this revision to Diff 42196.
astippich added a comment.
- don't change test strings for now
REPOSITORY
R286 KFileMetaData
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15704?vs=42174=42196
BRANCH
taglib_write_mimetypes
REVISION DETAIL
bruns added a comment.
In general this looks good, but I would like two changes:
1. Do the conversion to QTest first, and leave out the change for unicode
testing (e.g. `Title1` -> `Title €`)
2. Add a third column like "stringsuffix", and then add another test (row)
for each format.
astippich added a dependency: D15220: implement more basic tags for
taglibwriter.
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D15704
To: astippich, mgallien, bruns
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun,
ngraham, bruns,
astippich created this revision.
astippich added reviewers: mgallien, bruns.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.
REVISION SUMMARY
increase the test coverage by testing more mimetypes, and
11 matches
Mail list logo