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
Details
- Reviewers
mgallien michaelh - Group Reviewers
Frameworks Baloo
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
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.
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. |
Please fix the issues.
src/extractors/taglibextractor.cpp | ||
---|---|---|
363 | Is it really needed to push directly a list ? | |
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 |
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. | |
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. | |
393 | It depends on how you define 'releaseYear' if you think of it as year published negative dates can be ruled out. ;-) |
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?
src/extractors/taglibextractor.cpp | ||
---|---|---|
389 | For CDs, there are hidden tracks, IIRC these have negative numbers. |