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

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:52920c0003d6: move testing of common tags of test files 
to a new data-driven test for… (authored by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15825?vs=43028&id=43324

REVISION DETAIL
  https://phabricator.kde.org/D15825

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib_extractor_tests

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Stefan Brüns]

bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib_extractor_tests

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich added a comment.


  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.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Stefan Brüns]

bruns added a comment.


  In D15825#338218 , @astippich 
wrote:
  
  > In D15825#334087 , @bruns wrote:
  >
  > > 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 ?

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich added a comment.


  In D15825#334087 , @bruns wrote:
  
  > 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 

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich updated this revision to Diff 43028.
astippich added a comment.


  - explicitly test for supported mimetype

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15825?vs=42529&id=43028

BRANCH
  refactor_taglib_extractor_tests

REVISION DETAIL
  https://phabricator.kde.org/D15825

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Stefan Brüns]

bruns added a comment.


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

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich added a dependent revision: D15833: extend test coverage to all 
supported mimetypes for taglibextractor.

REPOSITORY
  R286 KFileMetaData

REVISION DETAIL
  https://phabricator.kde.org/D15825

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

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

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: bruns, svuorela.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  as a preparation for more test data, move all tags that
  are directly provided by taglib to a common test function

REPOSITORY
  R286 KFileMetaData

BRANCH
  refactor_taglib_extractor_tests

REVISION DETAIL
  https://phabricator.kde.org/D15825

AFFECTED FILES
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h

To: astippich, bruns, svuorela
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams