move testing of common tags of test files to a new data-driven test for taglibextractor
ClosedPublic

Authored by astippich on Sep 28 2018, 9:51 PM.

Details

Summary

As a preparation for more test data, move all tags that
have a direct implementation by taglib itself to a common
test function. New tag formats can then be tested for the
most common tags, which is needed for a following
patch.

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.
astippich created this revision.Sep 28 2018, 9:51 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptSep 28 2018, 9:51 PM
Restricted Application added subscribers: Baloo, kde-frameworks-devel. · View Herald Transcript
astippich requested review of this revision.Sep 28 2018, 9:51 PM
bruns added a comment.Sep 30 2018, 2:20 AM

There are some more common tags with identical values, i.e. AlbumArtist, Composer and Lyrics - any reason you kept these?

astippich updated this revision to Diff 43028.Oct 7 2018, 12:28 PM
  • explicitly test for supported mimetype

There are some more common tags with identical values, i.e. AlbumArtist, Composer and Lyrics - any reason you kept these?

These are the tags directly supported by taglib (e.g. tag->artist() ) which don't require manual reading of tags. These are only the supported tags for the mime types tested in D15833

bruns added a comment.Oct 8 2018, 6:49 PM

There are some more common tags with identical values, i.e. AlbumArtist, Composer and Lyrics - any reason you kept these?

These are the tags directly supported by taglib (e.g. tag->artist() ) which don't require manual reading of tags. These are only the supported tags for the mime types tested in D15833

Sorry, I don't get it yet ...

Is there anything preventing this to work:

SimpleExtractionResult result(testFilePath(fileName), mimeType);
plugin.extract(&result);
QCOMPARE(result.properties().value(Property::AlbumArtist), QVariant(QStringLiteral("Album Artist")));

As far as I can see, this is identical code for all tests, and supported by all formats.

Or are these not supported by some of the four new formats added in D15833?

Sorry, I should have written this more clearly, but you got it right. It's for the new formats. For the most common tags, taglib directly provides the implementation for all its supported tag formats and one can call e.g. tag->artist(). For the others, one has to implement support manually, e.g. query the tag type and then do tag->find("Album Artist") and then read it.
So for any new formats we get the ones ones which are implemented by taglib for free. The others have to be manually added and thus are not implemented for the new formats.
I will update the summary a little bit.

astippich edited the summary of this revision. (Show Details)Oct 9 2018, 6:08 AM
bruns accepted this revision.Oct 9 2018, 9:55 PM

Thanks for the explanation. Looks good then, just update the summary.

You could add another column to the tests, "hasFullImplementation", and do an Q_EXPECT_FAIL if not set. But thats for another patch ...

This revision is now accepted and ready to land.Oct 9 2018, 9:55 PM
astippich edited the summary of this revision. (Show Details)Oct 10 2018, 4:20 PM
This revision was automatically updated to reflect the committed changes.