Page MenuHomePhabricator

Do not hit actual urls during phpunit tests
Closed, ResolvedPublic

Description

Some phpunit tests are hitting actual external urls. This is undesirable because test behavior and performance may be inconsistent depending on actual internet conditions and the moment-by-moment state of external sites. It also prohibits successfully running phpunit tests in environments where external calls are not possible.

This was confirmed for UploadFromUrlTest by executing unit tests locally with internet enabled (they passed) then running them again with internet disabled (they failed).

It is possible that other test classes are similarly affected. The first step in addressing this task should be to inventory existing tests and determine the extent of this behavior. If it is found that a significant number of tests are affected, it may be desirable to file each affected test class as a subtask.

Related Objects

Event Timeline

Change 626208 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] UploadFromUrlTest: don't make a real request

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

Change 626348 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] PHPUnit: prevent HTTP requests

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

Change 626208 merged by jenkins-bot:
[mediawiki/core@master] UploadFromUrlTest: don't make a real request

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

daniel subscribed.

Preventing HTTP requests causes several tests to fail. The following tests in extensions appear to be attempting an HTTP request:

  • /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/InterwikiResolverTest.php:443
  • /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/Profile/SearchProfileServiceFactoryTest.php:249
  • /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/RequestLoggerTest.php:166
  • /workspace/src/extensions/Echo/tests/phpunit/integration/Push/NotificationServiceClientTest.php:12

The following test failures in core can be considered artifacts and should be easy enough to fix:

  • /workspace/src/tests/phpunit/includes/http/GuzzleHttpRequestTest.php:61
  • /workspace/src/tests/phpunit/includes/MediaWikiServicesTest.php:294

Change 628936 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Introduce MockHttpTrait

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

Change 628938 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/CirrusSearch@master] Use MockHttpTrait

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

Change 628939 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Echo@master] Use MockHttpTrait

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

Change 629208 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/MobileFrontend@master] McsContentProviderTest: avoid real HTTP request

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

Change 629221 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Wikibase@master] FederatedPropertiesTestTrait: Use mock HTTP

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

Change 629208 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] McsContentProviderTest: avoid real HTTP request

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

Change 628936 merged by jenkins-bot:
[mediawiki/core@master] Introduce MockHttpTrait

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

Change 629221 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] FederatedPropertiesTestTrait: Use mock HTTP

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

Change 628938 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Use MockHttpTrait

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

Change 628939 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use MockHttpTrait

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

Change 630906 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/Flow@master] Reset spam blacklist cache for tests

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

Change 630907 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/extensions/SpamBlacklist@master] Ensure instance cache does not interfere with tests.

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

Change 630907 merged by jenkins-bot:
[mediawiki/extensions/SpamBlacklist@master] Ensure instance cache does not interfere with tests.

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

Change 630906 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Reset spam blacklist cache for tests

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

Change 626348 merged by jenkins-bot:
[mediawiki/core@master] PHPUnit: prevent HTTP requests

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

Change 632656 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/WikibaseQualityConstraints@master] DelegatingConstraintCheckerTest: Use mock HTTP

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

Change 632656 merged by jenkins-bot:
[mediawiki/extensions/WikibaseQualityConstraints@master] DelegatingConstraintCheckerTest: Use mock HTTP

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

Clarakosi subscribed.

Marking as resolved but please reopen if that is not correct

Change 632820 had a related patch set uploaded (by Urbanecm; owner: Kosta Harlan):
[mediawiki/core@master] Revert "PHPUnit: prevent HTTP requests"

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

Urbanecm subscribed.

Seems to cause T262443, reopening as such.

Change 632820 merged by jenkins-bot:
[mediawiki/core@master] Revert "PHPUnit: prevent HTTP requests"

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

Change 632825 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Revert "Revert "PHPUnit: prevent HTTP requests""

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

Change 632825 merged by jenkins-bot:
[mediawiki/core@master] Re-apply: "PHPUnit: prevent HTTP requests"

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

Seems to cause T262443, reopening as such.

That links here... what do it cause? And was it resolved with the patch above?

Grr, sorry. I meant the GE failing tests issue, it should be all okay now. Thanks!

Change 636505 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] PHPUnit tests: prevent real Guzzle requests from tests.

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

Change 636505 merged by jenkins-bot:
[mediawiki/core@master] PHPUnit tests: prevent real Guzzle requests from tests.

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

This interacts badly with EventLogging/EventBus. See, for example, T270801: Integration tests die with "HTTP requests to <url> blocked. Use MockHttpTrait." when EventBus extension is installed, in which the creation of a revision in MediaWikiIntegrationTestCase->addCoreDBData causes the test suite to crash because EventBus attempts to send an event on PageSaveComplete.

Is the intent to force any test that somehow triggers an event to be updated to use MockHttpTrait? That seems like overkill to me. (Also, IMO it's made unreasonably difficult by T265662: MockHttpTrait fails the whole test suite instead of individual test).

Wouldn't it be better to have the MediaWikiIntegrationTestCase class use a mock HTTP request factory by default, rather than forcing many tests to be updated on a test-by-test basis?

Is the intent to force any test that somehow triggers an event to be updated to use MockHttpTrait? That seems like overkill to me. (Also, IMO it's made unreasonably difficult by T265662: MockHttpTrait fails the whole test suite instead of individual test).

This is an effect of having a default behavior that should be disabled during testing, because it affects other systems. Only the extension itself can know what a sensible default would be in the context of unit tests. It should register a hook to apply such a default, or make the default depend on MW_PNPUNIT_TEST. Mathoid has a similar problem.

Wouldn't it be better to have the MediaWikiIntegrationTestCase class use a mock HTTP request factory by default, rather than forcing many tests to be updated on a test-by-test basis?

It does - the default behavior of that mock HTTP request factory is to make the test fail. What else should it do? Provide empty responses? That will also make the tests fail, but with stranger errors.

More extensions that need fixing:

  • MultiMaps

Change 703186 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/Flow@master] phpunit: Prevent network requests

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

Change 703186 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] phpunit: Prevent network requests

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

@daniel: Hi, all related patches in Gerrit have been merged. Can this task be resolved (via Add Action...Change Status in the dropdown menu), or is there more to do in this task? Asking as you are set as task assignee. Thanks in advance!