Details

Reviewers

mgallien
bruns

Group Reviewers

Baloo
Frameworks

Commits

R286:fd378a11e424: taglibextractortest: Add test for files with empty metadata
R286:1755f07e31f4: taglibextractortest: Add test for files with empty metadata

Summary

Indicates a bug in kfilemetadata delivering genre property when genre has no value.

Copy appropiate sample files to no-meta/
Remove tag data with picard or kid3
Create test (of course)

Test Plan

make test

Diff Detail

Repository

R286 KFileMetaData

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

michaelh created this revision.Apr 7 2018, 7:08 PM

Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptApr 7 2018, 7:08 PM

michaelh requested review of this revision.Apr 7 2018, 7:08 PM

michaelh added a dependent revision: D12029: taglibextractor: Fix empty genre bug.

I need more time. I will try to look at it today. By the way, the stack concept seems very useful. Thanks

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 rebasing hell.

michaelh edited the summary of this revision. (Show Details)Apr 11 2018, 12:39 PM

Thanks for this work. Fix the issues and we should be good to go.

autotests/taglibextractortest.cpp
233	Please remove failMessage and use directly the expected value in each addRow. I fail to see what we gain by using this variable.
291	Why not use a simple variable here ? TagLibExtractor plugin;

This revision now requires changes to proceed.Apr 11 2018, 9:11 PM

Apply suggested change

michaelh added inline comments.Apr 11 2018, 10:52 PM

autotests/taglibextractortest.cpp
291	This converts the enums to strings. That way it is easier to spot which properties are responsible for the failure.

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 useful. Thanks

Yes, it has its downsides though. It is easy to find oneself in a rebasing hell.

Squash all commits you want as one Phabricator revision using git rebase -i
Create a branch for you stack on the first commit
Submit the commit with arc diff HEAD^
Cherry-pick the next commit

Repeat 3. and 4. until finished

Updating a commit/revision:

git rebase -i, select 'e'dit for the commit
edit, git add, git commit --amend
arc diff --preview HEAD^
git rebase --continue

bruns added inline comments.Apr 11 2018, 11:00 PM

autotests/taglibextractortest.cpp
291	What he likely meant: `TagLibExtractor plugin{this};` instead of `QScopedPointer<ExtractorPlugin> plugin(new TagLibExtractor(this));`

michaelh added inline comments.Apr 12 2018, 12:13 PM

autotests/taglibextractortest.cpp
291	Well,... In that case: D12145

Simple plugin variable

michaelh marked an inline comment as done.Apr 12 2018, 12:18 PM

Thanks

This revision is now accepted and ready to land.Apr 12 2018, 3:52 PM

michaelh mentioned this in D12145: autotests: Do not use QScopedPointer for plugins.Apr 12 2018, 3:57 PM

Closed by commit R286:fd378a11e424: taglibextractortest: Add test for files with empty metadata (authored by michaelh). · Explain WhyApr 12 2018, 4:01 PM

This revision was automatically updated to reflect the committed changes.

Untested, but looks good to me otherwise.

autotests/taglibextractortest.cpp
42	Nitpick - needs some indentation

Sample files are broken, for no obvious reason. https://build.kde.org/view/Frameworks/job/Frameworks%20kfilemetadata%20kf5-qt5%20SUSEQt5.10/lastBuild/

This revision is now accepted and ready to land.Apr 12 2018, 6:15 PM

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.<ext>

mpc with kid3-qt remove all tags and add ' ' as genre

Expect all test to fail
Add debug output

michaelh requested review of this revision.Apr 12 2018, 6:21 PM

bruns added inline comments.Apr 12 2018, 7:39 PM

autotests/taglibextractortest.cpp
300	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() = ... auto missingKeys() = ... if (excessKeys().size()) { QWARN("Excess properties: " + excessKeys.join(", ")); if (!failMessage.isEmpty()) QEXPECT_FAIL(...) QCOMPARE(excessKeys.size(), 0); } if (missingKeys().size()) { ... }

autotests/taglibextractortest.cpp

300

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() = ...
auto missingKeys() = ...

if (excessKeys().size()) {
    QWARN("Excess properties: " + excessKeys.join(", "));
    if (!failMessage.isEmpty())
        QEXPECT_FAIL(...)
    QCOMPARE(excessKeys.size(), 0);
}

if (missingKeys().size()) {
    ...
}

Optimize failure messages

Ok, lets see Jenkins opinion ...

This revision is now accepted and ready to land.Apr 13 2018, 12:31 AM

michaelh mentioned this in T8506: Arcanist doesn't apply patches correctly in certain circumstances..Apr 13 2018, 10:09 AM

Rebased

Closed by commit R286:1755f07e31f4: taglibextractortest: Add test for files with empty metadata (authored by michaelh). · Explain WhyApr 13 2018, 10:35 AM

This revision was automatically updated to reflect the committed changes.

		Path
M	(bin)	autotests/samplefiles/no-meta/test.flac
M	(bin)	autotests/samplefiles/no-meta/test.m4a
M	(bin)	autotests/samplefiles/no-meta/test.mp3
M	(bin)	autotests/samplefiles/no-meta/test.ogg
M	(bin)	autotests/samplefiles/no-meta/test.opus
M		autotests/taglibextractortest.cpp (24 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	31616	a25f89b		Apr 7 2018, 7:00 PM	★	★
Diff 2	31934	a25f89b	- Apply suggested change	Apr 11 2018, 10:47 PM	★	★
Diff 3	31968	a25f89b	- Simple plugin variable	Apr 12 2018, 12:17 PM	★	★
Diff 4	31975	7f9de32	R286:fd378a11e4243d72d5d634d0209119be813adcb0	Apr 12 2018, 4:04 PM	★	★
Diff 5	31989	7f9de32	- Recreate sample files and adapt tests	Apr 12 2018, 6:20 PM	★	★
Diff 6	32009	7f9de32	- Optimize failure messages	Apr 12 2018, 8:14 PM	★	★
Diff 7	32048	fd378a1	- Rebased	Apr 13 2018, 10:34 AM	★	★
Diff 8	32049	fd378a1	R286:1755f07e31f4835d777c740d36e6a49a5e07bf39	Apr 13 2018, 10:38 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
0c6e8f9cd7d4	1c670f205005	251b19a5774c	Michael Heidelbach	taglibextractortest: Recreate sample files and adapt tests (Show More…)	Apr 12 2018, 6:11 PM
251b19a5774c	a6d3cd645782	fd378a11e424	Michael Heidelbach	taglibextractortest: Add test for files without metadata	Apr 7 2018, 3:41 PM

	Status	Author	Revision
	Closed	michaelh	D12029 taglibextractor: Fix empty genre bug
	Closed	michaelh	D12028 taglibextractortest: Add test for files with empty metadata

taglibextractortest: Add test for files with empty metadata
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 32049

autotests/samplefiles/no-meta/test.flac

autotests/samplefiles/no-meta/test.m4a

autotests/samplefiles/no-meta/test.mp3

autotests/samplefiles/no-meta/test.ogg

autotests/samplefiles/no-meta/test.opus

autotests/taglibextractortest.cpp

taglibextractortest: Add test for files with empty metadataClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 32049

autotests/samplefiles/no-meta/test.flac

autotests/samplefiles/no-meta/test.m4a

autotests/samplefiles/no-meta/test.mp3

autotests/samplefiles/no-meta/test.ogg

autotests/samplefiles/no-meta/test.opus

autotests/taglibextractortest.cpp

taglibextractortest: Add test for files with empty metadata
ClosedPublic
Actions

Revision Contents
Changeset List