make && ./xmlextractortest
Details
- Reviewers
astippich - Group Reviewers
Frameworks - Commits
- R286:dbb34ea5039c: [XmlExtractor] Add unittest for XML extractor
Diff Detail
- Repository
- R286 KFileMetaData
- Branch
- xml_extractor
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 4302 Build 4320: arc lint + arc unit
Generally looks OK to me, one note on QDir::separator() usage.
autotests/xmlextractortest.cpp | ||
---|---|---|
38 ↗ | (On Diff #44375) | IIRC you shouldn't use QDir::separator(). See http://agateau.com/2015/qdir-separator-considered-harmful/ |
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.
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.
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.
I understand that ExtractorCollection is not used, and that the extractor gets the mimetype directly set in the test.
But the list retrieved via XmlExtractor::mimetypes() is part of the extractor itself, isn't it? So imho that should be tested here.
All I'm asking for is something similar to https://phabricator.kde.org/source/kfilemetadata/browse/master/autotests/embeddedimagedatatest.cpp$48
For very specialized media types, this works out ok, but this is not correct in general:
Assume you have a filetype which is a specialization of some other, supported mime type. This specialization is not in the mimetypes list, but can still be handled by the extractor.
For XML, you will not be able to add every specialized mimetype. SVG, which is a specialization, is listed specifically because it has specific supporting code, but every other XML variant can be handled by the generic code.
This causes the Qt5.9 build to fail
https://build.kde.org/job/Frameworks/job/kfilemetadata/job/kf5-qt5%20SUSEQt5.9/workflow-stage/