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

Branch

nometa (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

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
173	Please remove failMessage and use directly the expected value in each addRow. I fail to see what we gain by using this variable.
231	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
231	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
231	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
231	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 ↗	(On Diff #31934)	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
240 ↗	(On Diff #31934)	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

240 ↗

(On Diff #31934)

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
A	M	(bin)	autotests/samplefiles/no-meta/test.flac
A	M	(bin)	autotests/samplefiles/no-meta/test.m4a
A	M	(bin)	autotests/samplefiles/no-meta/test.mp3
A	M	(bin)	autotests/samplefiles/no-meta/test.mpc
A	M	(bin)	autotests/samplefiles/no-meta/test.ogg
A	M	(bin)	autotests/samplefiles/no-meta/test.opus
M			autotests/taglibextractortest.h (7 lines)
M			autotests/taglibextractortest.cpp (89 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
6db8787cd70f	bfb51e33d7fa	e0d4b12ef234	Michael Heidelbach	Simple plugin variable	Apr 12 2018, 12:19 PM
e0d4b12ef234	462758c7e0ef	56050787a119	Michael Heidelbach	Remove comment from mpc	Apr 7 2018, 5:17 PM
56050787a119	cfef8d8d047b	b9159870a81e	Michael Heidelbach	taglibextractortest: Add test for files without metadata	Apr 7 2018, 3:41 PM
b9159870a81e	9e8bd0e15ff9	a25f89b22468	Michael Heidelbach	Add no metadata audio sample files	Apr 7 2018, 2:32 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 31968

autotests/samplefiles/no-meta/test.flac

autotests/samplefiles/no-meta/test.m4a

autotests/samplefiles/no-meta/test.mp3

autotests/samplefiles/no-meta/test.mpc

autotests/samplefiles/no-meta/test.ogg

autotests/samplefiles/no-meta/test.opus

autotests/taglibextractortest.h

autotests/taglibextractortest.cpp

taglibextractortest: Add test for files with empty metadataClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 31968

autotests/samplefiles/no-meta/test.flac

autotests/samplefiles/no-meta/test.m4a

autotests/samplefiles/no-meta/test.mp3

autotests/samplefiles/no-meta/test.mpc

autotests/samplefiles/no-meta/test.ogg

autotests/samplefiles/no-meta/test.opus

autotests/taglibextractortest.h

autotests/taglibextractortest.cpp

taglibextractortest: Add test for files with empty metadata
ClosedPublic
Actions

Revision Contents
Changeset List