Page MenuHomePhabricator

Isolate FileImporter integration test from external web services
Open, LowPublic1 Story Points

Description

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.

Event Timeline

awight created this task.May 11 2019, 8:20 PM
thiemowmde triaged this task as Low priority.May 14 2019, 1:33 PM
thiemowmde added a project: WMDE-QWERTY-Team.
thiemowmde added subscribers: thiemowmde, WMDE-Fisch.

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.

  • All the test might need to do is to replace the FileImporterHttpRequestExecutor service with a mock.

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"

https://gerrit.wikimedia.org/r/511697

Change 511697 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Mark SpecialImportFileIntegrationTest as "medium"

https://gerrit.wikimedia.org/r/511697

WMDE-Fisch set the point value for this task to 5.Jun 5 2019, 9:02 AM

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.
Andrew-WMDE moved this task from Sprint Backlog to Doing on the WMDE-QWERTY-Sprint-2019-05-29 board.

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

https://gerrit.wikimedia.org/r/514541

WMDE-Fisch added a subscriber: Andrew-WMDE.
WMDE-Fisch changed the point value for this task from 5 to 1.Jun 12 2019, 1:29 PM

Change 514541 merged by jenkins-bot:
[mediawiki/extensions/FileImporter@master] Isolate FileImporter integration test from external web services

https://gerrit.wikimedia.org/r/514541