D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns added a comment.


  In D16490#352199 , @astippich 
wrote:
  
  > In D16490#352109 , @bruns wrote:
  >
  > > In D16490#351935 , @astippich 
wrote:
  > >
  > > > In D16490#351799 , @bruns 
wrote:
  > > >
  > > > > In D16490#351662 , 
@astippich wrote:
  > > > >
  > > > > > Only one minor thing: please also check that the mimetype is in the 
list of supported mimetypes
  > > > >
  > > > >
  > > > > This can actually happen and is completely valid, due to mimetype 
inheritance.
  > > > >
  > > > > So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...
  > > >
  > > >
  > > > Hmmm, I don't understand. When I change the code to return an empty 
stringlist of supported mimetypes for the xmlextractor, the tests still pass.
  > > >  This should imho be covered by the tests.
  > >
  > >
  > > This is one level above these tests. The surrounding code ensures the 
right extractor is called for each file, see 
`ExtractorCollection::fetchExtractors(...)`.
  >
  >
  > Right, and if e.g. the list of supported mimetypes is empty, the 
corresponding extractor will never be selected because ExtractorCollection 
doesn't know that the mimetype is supported by this extractor.
  >  Hence we should ensure and test imho that the list of supported mimetypes 
provided to the ExtractorCollection is correct for this extractor. I'm not 
calling for testing that the right extractor is selected.
  
  
  The unit tests do not use ExtractorCollection, because they test the 
extractors, not ExtractorCollection. The extractor unit tests explicitly pass 
the mime type to the extractor.
  
  We don't want to double the checks.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Alexander Stippich]

astippich added a comment.


  In D16490#352109 , @bruns wrote:
  
  > In D16490#351935 , @astippich 
wrote:
  >
  > > In D16490#351799 , @bruns 
wrote:
  > >
  > > > In D16490#351662 , 
@astippich wrote:
  > > >
  > > > > Only one minor thing: please also check that the mimetype is in the 
list of supported mimetypes
  > > >
  > > >
  > > > This can actually happen and is completely valid, due to mimetype 
inheritance.
  > > >
  > > > So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...
  > >
  > >
  > > Hmmm, I don't understand. When I change the code to return an empty 
stringlist of supported mimetypes for the xmlextractor, the tests still pass.
  > >  This should imho be covered by the tests.
  >
  >
  > This is one level above these tests. The surrounding code ensures the right 
extractor is called for each file, see 
`ExtractorCollection::fetchExtractors(...)`.
  
  
  Right, and if e.g. the list of supported mimetypes is empty, the 
corresponding extractor will never be selected because ExtractorCollection 
doesn't know that the mimetype is supported by this extractor.
  Hence we should ensure and test imho that the list of supported mimetypes 
provided to the ExtractorCollection is correct for this extractor. I'm not 
calling for testing that the right extractor is selected.
  
  > These are unit tests. The test itself is responsible to call an extractor 
with a suitable file and a matching mimetype.
  > 
  > What you are calling for are system tests.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns added a comment.


  In D16490#351935 , @astippich 
wrote:
  
  > In D16490#351799 , @bruns wrote:
  >
  > > In D16490#351662 , @astippich 
wrote:
  > >
  > > > Only one minor thing: please also check that the mimetype is in the 
list of supported mimetypes
  > >
  > >
  > > This can actually happen and is completely valid, due to mimetype 
inheritance.
  > >
  > > So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...
  >
  >
  > Hmmm, I don't understand. When I change the code to return an empty 
stringlist of supported mimetypes for the xmlextractor, the tests still pass.
  >  This should imho be covered by the tests.
  
  
  This is one level above these tests. The surrounding code ensures the right 
extractor is called for each file, see 
`ExtractorCollection::fetchExtractors(...)`.
  
  These are unit tests. The test itself is responsible to call an extractor 
with a suitable file and a matching mimetype.
  
  What you are calling for are system tests.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Alexander Stippich]

astippich added a comment.


  In D16490#351799 , @bruns wrote:
  
  > In D16490#351662 , @astippich 
wrote:
  >
  > > Only one minor thing: please also check that the mimetype is in the list 
of supported mimetypes
  >
  >
  > This can actually happen and is completely valid, due to mimetype 
inheritance.
  >
  > So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...
  
  
  Hmmm, I don't understand. When I change the code to return an empty 
stringlist of supported mimetypes for the xmlextractor, the tests still pass.
  This should imho be covered by the tests.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added a comment.


  In D16490#351662 , @astippich 
wrote:
  
  > Only one minor thing: please also check that the mimetype is in the list of 
supported mimetypes
  
  
  This can actually happen and is completely valid, due to mimetype inheritance.
  
  So the check would be `for supported in supportedMimetypes { if 
QMimeType(input->mimeType()).inherits(supported) return true; }; return false`. 
But this is already done from the calling code ...

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Alexander Stippich]

astippich added a comment.


  Only one minor thing: please also check that the mimetype is in the list of 
supported mimetypes

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns marked an inline comment as done.
bruns added inline comments.

INLINE COMMENTS

> lbeltrame wrote in xmlextractortest.cpp:38
> IIRC you shouldn't use `QDir::separator()`.  See 
> http://agateau.com/2015/qdir-separator-considered-harmful/

Thx, fixed here and elsewhere (D16505 )

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns updated this revision to Diff 44409.
bruns added a comment.


  Replace QDir::Separator with "/"

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16490?vs=44375=44409

BRANCH
  xml_extractor

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/samplefiles/test_with_metadata.svg
  autotests/xmlextractortest.cpp
  autotests/xmlextractortest.h

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Luca Beltrame]

lbeltrame added a comment.


  Generally looks OK to me, one note on `QDir::separator()` usage.

INLINE COMMENTS

> xmlextractortest.cpp:38
> +{
> +return QLatin1String(INDEXER_TESTS_SAMPLE_FILES_PATH) + 
> QDir::separator() + fileName;
> +}

IIRC you shouldn't use `QDir::separator()`.  See 
http://agateau.com/2015/qdir-separator-considered-harmful/

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns added a dependency: D16489: [KFileMetaData] Add extractor for generic XML 
and SVG.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

bruns edited the test plan for this revision.
bruns added reviewers: Frameworks, astippich.

REPOSITORY
  R286 KFileMetaData

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

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

D16490: [KFileMetaData] Add unittest for XML extractor

[Stefan Brüns]

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

REPOSITORY
  R286 KFileMetaData

BRANCH
  xml_extractor

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/samplefiles/test_with_metadata.svg
  autotests/xmlextractortest.cpp
  autotests/xmlextractortest.h

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