The tests in SpecialImportFileIntegrationTest.php contact external services, which is extremely naughty of us. These should probably be rewritten to use static response fixtures, or split into more focused unit tests. The requests are responsible for approximately 50% of the run time of our extension's entire test suite--it probably doesn't add up to much on a normal day, just a few seconds, but if the network or service is being temperamental we could add dramatically to the overall time required to run full MediaWiki test suites. There's also the risk of false positives if the service is down, or the randomly chosen test files have been changed from what we're expecting. We can trust that the API contract is trustworthy, so making these requests doesn't add any value to our test suite.
Description
Details
Related Objects
- Mentioned In
- T225068: Add a PHPUnit group to skip test on gated CI runs
rEFLI07660053717c: Isolate FileImporter integration test from external web services
rEFLId4ceefbc1a61: Isolate FileImporter integration test from external web services
rEFLI228735adf47c: Isolate FileImporter integration test from external web services
rEFLIc91381d54fb9: Isolate FileImporter integration test from external web services
rEFLI120a51d2d778: Isolate FileImporter integration test from external web services
rEFLId2cb13d60dc8: Isolate FileImporter integration test from external web services
rEFLId5e4144586e2: Mark SpecialImportFileIntegrationTest as "medium"
T222802: Add FileImporter to gated extensions
Event Timeline
One of the purposes of this test is to cover the ServiceWiring. This means the test should not be isolated so much that it does not cover this code any more.
I can see two possible solutions:
- All the test might need to do is to replace the FileImporterHttpRequestExecutor service with a mock.
- Another option is to not replace this service, but wrap it in a facade that does nothing but replacing the factory it internally uses via HttpRequestExecutor::overrideRequestFactory. This does have the advantage that all FileImporterHttpRequestExecutor is still part of the integration test. The disadvantage is that the mock for this will be quite a bit more complicated.
Both ideas sound surprisingly trivial to me, but I'm sure I missed something.
I like this approach, in the past I've gotten a lot of value out of simply recording API responses and bundling as test fixtures.
Another option is to not replace this service, but wrap it in a facade that does nothing but replacing the factory it internally uses via HttpRequestExecutor::overrideRequestFactory. This does have the advantage that all FileImporterHttpRequestExecutor is still part of the integration test. The disadvantage is that the mock for this will be quite a bit more complicated.
A separate unit test on FileImporterHttpRequestExecutor which asserts that it's calling the HttpRequestExecutor as expected should be equivalent.
Change 511697 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/FileImporter@master] Mark SpecialImportFileIntegrationTest as "medium"
Change 511697 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Mark SpecialImportFileIntegrationTest as "medium"
Briefly discussed and estimated with the team.
- We do not want to split the test, but replace the API call with a mocked one.
- We suggest to store the JSON responses from the actual API calls in a series of files (e.g. in the existing tests/phpunit/res/ folder), and use these files in the mock.
Change 514541 had a related patch set uploaded (by Andrew-WMDE; owner: Andrew-WMDE):
[mediawiki/extensions/FileImporter@master] Isolate FileImporter integration test from external web services
Change 514541 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Isolate FileImporter integration test from external web services