also test for value types in taglibextractortest and fix errors
AbandonedPublic
Actions

Authored by astippich on Mar 15 2018, 6:55 PM.

Details

Reviewers

mgallien
michaelh

Group Reviewers

Frameworks
Baloo

Summary

taglib tests never tested for the expected value type. add this to the tests and fix the resulting errors. also add some more metadata to the samplefiles

Test Plan

Thorough testing with baloo and all users of KFileMetaData

Diff Detail

Repository

R286 KFileMetaData

Branch

taglib_test

Lint

No Linters Available

Unit

No Unit Test Coverage

astippich created this revision.Mar 15 2018, 6:55 PM

Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMar 15 2018, 6:55 PM

Restricted Application added a subscriber: Frameworks. · View Herald Transcript

astippich requested review of this revision.Mar 15 2018, 6:55 PM

astippich added reviewers: Frameworks, Baloo.Mar 15 2018, 6:55 PM

Just in case you didn't use data-driven tests on purpose: https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

In D11365#227241, @michaelh wrote:

Just in case you didn't use data-driven tests on purpose: https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

Thanks for the hint, that's new for me. Anyways, there are subtle differences for each file and there will be more in the future, so I think it is better to leave it unchanged.

astippich added reviewers: mgallien, michaelh.Mar 18 2018, 7:51 PM

any feedback on this? I would like to land this and then get going with D10803 again

It is very hard for me understand what you are actually testing. Please (re-)consider to organize like below (or similar). It would be closer to consumer's point of view. What do I get when I ask kfilemetadata for the artist of test.m4a?

void TagLibExtractorTest::testArtist_data()
{
    QTest::addColumn<QString>("path");
    QTest::addColumn<QString>("mime");
    QTest::addColumn<Property::Property>("prop");
    QTest::addColumn<QString>("value");
    QTest::addColumn<QStringList>("valueList");
    QTest::addColumn<QVariant::Type>("type");

    QTest::addRow("m4a")
        << QFINDTESTDATA("samplefiles/test.m4a")
        << QStringLiteral( "audio/mp4")
        << Property::Artist
        << QStringLiteral("Artist")
        << QStringList{QStringLiteral("Artist")}
        << PropertyInfo(Property::Artist).valueType()
        ;

... more files
}

void  TagLibExtractorTest::testArtist()
{
    QFETCH(QString, path);
    QFETCH(QString, mime);
    QFETCH(KFileMetaData::Property::Property, prop);
    QFETCH(QString, value);
    QFETCH(QStringList, valueList);
    QFETCH(QVariant::Type, type);
    QScopedPointer<ExtractorPlugin> plugin(new TagLibExtractor(this));
    SimpleExtractionResult extracted(path, mime);
    plugin->extract(&extracted);
    QCOMPARE(extracted.properties().value(prop).type(), type);
    QCOMPARE(extracted.properties().value(prop), value);
    QCOMPARE(extracted.properties().value(prop), valueList);

}

src/propertyinfo.cpp
98	Looks unrelated to me.

It's just testing each file for all available properties, one after another. This revision just adds missing bits to the existing test. Refactoring should be done separately imho

src/propertyinfo.cpp
98	It's not, composer should really be treated the same as e.g. artist or lyricist, as it can have multiple entries.

astippich mentioned this in D10803: handle more tags in taglibextractor.Mar 27 2018, 7:08 PM

Please fix the issues.

src/extractors/taglibextractor.cpp
363	Is it really needed to push directly a list ? You are ignoring the fact that the original developer intended add to be called once for each value. I would prefer to keep doing that until we can be absolutely sure that nothing breaks if we change that.
370	idem
374	idem
385	idem
389	Are you sure we need this cast ? I do not think we will ever need to have negative track numbers added. The original type is unsigned int and should exactly convey the fact that we do not expect negative numbers.
393	idem

This revision now requires changes to proceed.Mar 28 2018, 3:47 PM

astippich added inline comments.Mar 28 2018, 7:24 PM

src/extractors/taglibextractor.cpp
363	That was actually the point for this. In propertyinfo, it says that this property is a stringlist, but it was actually never one, just a simple string. For multiple values, there will be multiple entries in the property map with a single string. This is not considered within Elisa at least. We could also go the other way and adjust the properties to match the behavior. @michaelh agreed that stringlists are easier to handle
389	I changed it so it matches its associated value type, I could also change the type in propertyinfo to uint
393	This one is a litte bit more tricky, as the year property could theoretically also be negative (B.C.). Not really relevant for music, but maybe for written documents, and still only very rarily :)

This patch should be split.

Test more properties
Change return types of ...

Also 'fix errors' in the title is misleading because currently kfilemetadata works well.

src/extractors/taglibextractor.cpp
363	You have to ensure baloo searching still works, when string lists contains more than 1 items. I think it will break. Generally IMO returning a string list is the natural thing to do here. On the other hand: Why is `PropertyInfo` announcing the value type when calling `.type()` on a QVariant would be sufficient. There must be a reason for using `PropertyMap` and giving a type hint in `PropertyInfo`. Unless somebody knows the reason this will need some investigation and a concerted action if we choose to change it. I've put D10694 on hold because of that.
393	It depends on how you define 'releaseYear' if you think of it as year published negative dates can be ruled out. ;-)

Since we're struggling with the same issues: T8196 and D10694

In D11365#236274, @michaelh wrote:

This patch should be split.

Test more properties

Change return types of ...

Also 'fix errors' in the title is misleading because currently kfilemetadata works well.

Sure, I can do that if it is not a problem that the new tests do not pass (temporarily). It is probably the best idea to create thorough tests for the way we'd like KFileMetaData/taglib to work, as they are lacking in several ways (and hence created the confusion I have had) . Then we can start fixing the errors.
Can we reach a consent how it should behave in the end? e.g. should the result match the valueType in propertyinfo, should there be multiple properties with single strings or stringlists for multiple values? Any other concerns?
@mgallien, what's your opinion here?

astippich mentioned this in D12932: handle string lists as input.May 16 2018, 6:39 PM

bruns added a subscriber: bruns.May 16 2018, 8:19 PM

bruns added inline comments.

src/extractors/taglibextractor.cpp
389	For CDs, there are hidden tracks, IIRC these have negative numbers.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 16 2018, 8:19 PM

astippich mentioned this in D12950: add test which checks the property types.Jul 12 2018, 7:23 PM

astippich abandoned this revision.Nov 13 2018, 5:16 PM

Revision Contents
Changeset List

		Path
M	(bin)	autotests/samplefiles/test.flac
M	(bin)	autotests/samplefiles/test.m4a
M	(bin)	autotests/samplefiles/test.mp3
M	(bin)	autotests/samplefiles/test.mpc
M	(bin)	autotests/samplefiles/test.ogg
M	(bin)	autotests/samplefiles/test.opus
M		autotests/taglibextractortest.cpp (88 lines)
M		src/extractors/taglibextractor.cpp (49 lines)
M		src/propertyinfo.cpp (2 lines)

Commit	Tree	Parents	Author	Summary	Date
712b0e7536bb	d652e38071a6	a97308cf9ab8	Alexander Stippich	also test for value types in taglibextractortest and fix errors (Show More…)	Mar 15 2018, 5:26 PM