D12028: taglibextractortest: Add test for files with empty metadata

This revision was automatically updated to reflect the committed changes. Closed by commit R286:1755f07e31f4: taglibextractortest: Add test for files with empty metadata (authored by michaelh). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=3204

D12028: taglibextractortest: Add test for files with empty metadata

michaelh updated this revision to Diff 32048. michaelh added a comment. - Rebased REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=32009&id=32048 BRANCH nometa REVISION DETAIL https://phabricator.kde.org/D12028 AFFECTED FILES autotests

D12028: taglibextractortest: Add test for files with empty metadata

bruns accepted this revision. bruns added a comment. This revision is now accepted and ready to land. Ok, lets see Jenkins opinion ... REPOSITORY R286 KFileMetaData BRANCH nometa REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks, bruns Cc:

D12028: taglibextractortest: Add test for files with empty metadata

michaelh updated this revision to Diff 32009. michaelh added a comment. - Optimize failure messages REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=31989&id=32009 BRANCH nometa REVISION DETAIL https://phabricator.kde.org/D12028 AFFECTED

D12028: taglibextractortest: Add test for files with empty metadata

bruns added inline comments. INLINE COMMENTS > taglibextractortest.cpp:300 > +} > +QCOMPARE(resultKeys, expectedKeys); > +} Unfortunately QCOMPARE does not do a deep compare if sizes mismatch. To get a better output in case the test fails, you can do something like: auto excessKeys()

D12028: taglibextractortest: Add test for files with empty metadata

michaelh requested review of this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: bruns, astippich, ashaposhnikov, michaelh, spoorun, ngraham, alexeymin

D12028: taglibextractortest: Add test for files with empty metadata

michaelh updated this revision to Diff 31989. michaelh added a comment. - Recreate sample files and adapt tests for mp3, m4a, ogg, opus, flac recreate files with `$ ffmpeg -f lavfi -i anullsrc=channel_layout=stereo:sample_rate=44100 -t 1 test.` for mpc with kid3-qt remove all tags and add '

D12028: taglibextractortest: Add test for files with empty metadata

michaelh reopened this revision. michaelh added a comment. This revision is now accepted and ready to land. Sample files are borked. https://build.kde.org/view/Frameworks/job/Frameworks%20kfilemetadata%20kf5-qt5%20SUSEQt5.10/lastBuild/ REPOSITORY R286 KFileMetaData REVISION DETAIL https:

D12028: taglibextractortest: Add test for files with empty metadata

bruns added a comment. Untested, but looks good to me otherwise. INLINE COMMENTS > taglibextractortest.cpp:42 > > +const QStringList TagLibExtractorTest::propertyEnumNames(const > QList& keys) const > +{ Nitpick - needs some indentation REPOSITORY R286 KFileMetaData REVISION DETAIL

D12028: taglibextractortest: Add test for files with empty metadata

This revision was automatically updated to reflect the committed changes. Closed by commit R286:fd378a11e424: taglibextractortest: Add test for files with empty metadata (authored by michaelh). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=3196

D12028: taglibextractortest: Add test for files with empty metadata

mgallien accepted this revision. mgallien added a comment. This revision is now accepted and ready to land. Thanks REPOSITORY R286 KFileMetaData BRANCH nometa (branched from master) REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: brun

D12028: taglibextractortest: Add test for files with empty metadata

michaelh marked an inline comment as done. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: bruns, astippich, ashaposhnikov, michaelh, spoorun, ngraham, alexeymin

D12028: taglibextractortest: Add test for files with empty metadata

michaelh updated this revision to Diff 31968. michaelh added a comment. - Simple plugin variable REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=31934&id=31968 BRANCH nometa (branched from master) REVISION DETAIL https://phabricator.kde.

D12028: taglibextractortest: Add test for files with empty metadata

michaelh added inline comments. INLINE COMMENTS > bruns wrote in taglibextractortest.cpp:231 > What he likely meant: > `TagLibExtractor plugin{this};` > instead of > `QScopedPointer plugin(new TagLibExtractor(this));` Well,... In that case: D12145 REPOSITORY

D12028: taglibextractortest: Add test for files with empty metadata

bruns added inline comments. INLINE COMMENTS > michaelh wrote in taglibextractortest.cpp:231 > This converts the enums to strings. That way it is easier to spot which > properties are responsible for the failure. What he likely meant: `TagLibExtractor plugin{this};` instead of `QScopedPointer p

D12028: taglibextractortest: Add test for files with empty metadata

bruns added a comment. In D12028#244257 , @michaelh wrote: > In D12028#244243 , @mgallien wrote: > > > I need more time. I will try to look at it today. By the way, the stack concept seems very usef

D12028: taglibextractortest: Add test for files with empty metadata

michaelh added inline comments. INLINE COMMENTS > mgallien wrote in taglibextractortest.cpp:231 > Why not use a simple variable here ? > TagLibExtractor plugin; This converts the enums to strings. That way it is easier to spot which properties are responsible for the failure. REPOSITORY R286

D12028: taglibextractortest: Add test for files with empty metadata

michaelh updated this revision to Diff 31934. michaelh added a comment. - Apply suggested change REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D12028?vs=31616&id=31934 BRANCH nometa (branched from master) REVISION DETAIL https://phabricator.kde.

D12028: taglibextractortest: Add test for files with empty metadata

mgallien requested changes to this revision. mgallien added a comment. This revision now requires changes to proceed. Thanks for this work. Fix the issues and we should be good to go. INLINE COMMENTS > taglibextractortest.cpp:173 > +const auto testpasses = QString(); > +QString failMe

D12028: taglibextractortest: Add test for files with empty metadata

michaelh edited the summary of this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: astippich, ashaposhnikov, michaelh, spoorun, ngraham, bruns, alexeymin

D12028: taglibextractortest: Add test for files with empty metadata

michaelh added a comment. In D12028#244243 , @mgallien wrote: > I need more time. I will try to look at it today. By the way, the stack concept seems very useful. Thanks Yes, it has its downsides though. It is easy to find oneself in a re

D12028: taglibextractortest: Add test for files with empty metadata

mgallien added a comment. I need more time. I will try to look at it today. By the way, the stack concept seems very useful. Thanks REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: astippich, ashaposhnikov,

D12028: taglibextractortest: Add test for files with empty metadata

michaelh added a dependent revision: D12029: taglibextractor: Fix empty genre bug. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D12028 To: michaelh, mgallien, #baloo, #frameworks Cc: astippich, ashaposhnikov, michaelh, spoorun, ngraham, bruns, alexeymin

D12028: taglibextractortest: Add test for files with empty metadata

michaelh created this revision. michaelh added reviewers: mgallien, Baloo, Frameworks. Restricted Application added projects: Frameworks, Baloo. michaelh requested review of this revision. REVISION SUMMARY Indicates a bug in `kfilemetadata` delivering genre property when genre has no value. TE