D15704: increase test coverage of taglibwriter
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 SINCE LAST UPDATE https://phabricator.kde.org/D15704?vs=42519=42521 REVISION DETAIL https://phabricator.kde.org/D15704 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, mgallien, bruns Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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 https://phabricator.kde.org/D15704 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, mgallien, bruns Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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 IMHO it is
nicer to have some rule for ordering, instead of arbitrary order.
> taglibwriter.cpp:23
> QStringList types = {
> QStringLiteral("audio/mpeg"),
> QStringLiteral("audio/mpeg3"),
Can you also order these alphabetically based on the main mime type (aliases
immediate after)?
> taglibwriter.cpp:33
> +QStringLiteral("audio/x-opus+ogg"),
> +QStringLiteral("audio/wav"),
> +QStringLiteral("audio/x-aiff"),
audio/wav and the next three are not backed by tests AFAICS, can you add these
in a separate review, accompanied by tests files?
REPOSITORY
R286 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D15704
To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich,
spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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 for all at once and separately REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D15704 To: astippich, mgallien, bruns Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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 KFileMetaData
REVISION DETAIL
https://phabricator.kde.org/D15704
To: astippich, mgallien, bruns
Cc: svuorela, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich,
spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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 https://phabricator.kde.org/D15704 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D15704: increase test coverage of taglibwriter
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. `QStringLiteral("Title1")` then becomes
`QStringLiteral("Title1") + stringsuffix`
(2.) would go in a dependent review.
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, abrahams
D15704: increase test coverage of taglibwriter
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, abrahams
D15704: increase test coverage of taglibwriter
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 use some unicode characters TEST PLAN taglibwriter test pass REPOSITORY R286 KFileMetaData BRANCH taglib_write_mimetypes REVISION DETAIL https://phabricator.kde.org/D15704 AFFECTED FILES autotests/taglibwritertest.cpp autotests/taglibwritertest.h src/writers/taglibwriter.cpp To: astippich, mgallien, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
