Page MenuHomePhabricator

Add a PHPUnit group to skip test on gated CI runs
Open, Needs TriagePublic

Description

Create a PHPUnit group in mediawiki/core, add a new testsuite that would exclude the group in gated runs and then switch the wmf-quibble jobs to use that new suite.

Background
Several extensions[1] that should be added to the extension-gated CI runs have very time consuming tests that would increase run time tremendously. Therefor these extensions are not added to the gated runs, potentially risking that things break unnoticed.

See also the discussion roughly starting here T125050#5120646

[1] MediaWiki-extensions-Scribunto, CirrusSearch or the FileImporter from Move-Files-To-Commons

Event Timeline

Restricted Application added projects: TCB-Team, Discovery-Search. · View Herald TranscriptJun 5 2019, 9:34 AM
hashar awarded a token.Jun 5 2019, 5:18 PM

Is this waiting for anything other than naming to be settled?

Can we use the standard @small/@medium/@large tags (https://phpunit.de/manual/6.5/en/appendixes.annotations.html) that PHPUnit already supports?

Can we use the standard @small/@medium/@large tags (https://phpunit.de/manual/6.5/en/appendixes.annotations.html) that PHPUnit already supports?

I don't think (ab)using the semantics of those would be a great idea. Some isolated unit tests might be very fast but still unnecessary to run repeatedly, and full-stack system integration tests despite being slow are the value we get from testing code together…

hashar added a comment.Jun 5 2019, 6:43 PM

...

  • Then there are the 1000+ tests in tests/phpunit/engines/LuaCommon, which are run for both LuaSandbox and LuaStandalone to ensure things work under both environments.
    • The run for LuaSandbox takes around 34 seconds on my laptop, and the run for LuaStandalone takes around 163 seconds.

...

And some other similar candidates:

CirrusSearch\SearcherTest::testSearchText
MathCoverageTest::testCoverage

Taking Wikibase as an example, CI eventually injects CirrusSearch, Math and Scribunto (among others). Since we run tests/phpunit/phpunit.php --testsuite extensions then every tests get run which is not so nice. In T125050 we started talking about adding a way to flag tests as being standalone / only for a given extension. Or in other term, that they are not integration tests between extensions.

Given a patch send to Wikibase, we certainly want to run all of its tests. I believe that can be done by simply passing the extension path as an argument: tests/phpunit/phpunit.php --testsuite extensions -- extensions/Wikibase. I am not sure whether the structure or parser tests would be properly included though.

Then we will want to find out whether we want other extensions tests to be:

blacklisted

Exclude tests marked as being standalone / not integration tests. Eg: tests/phpunit/phpunit.php --testsuite extensions --exclude-group standalone (or whatever name we settle on).

There is a risk a test is actually depending on some other extension and thus might suddenly break when the other extension is modified. But I guess we can trust developers best judgment there. All other tests are still run so we get ample coverage and prevent breakages.

whitelist

Run only tests explicitly flagged as being integration tests. Eg tests/phpunit/phpunit.php --testsuite extensions --group integration. That would by far the fastest since no more tests would be run. But I don't think we are well equipped to find out whether a test is actually not relying on any other code. So I think it is rather risky. The advantage of whitelisting with @group integration is that we run no tests by default.

A third way would be a mixture of both system. So that potentially a test from extension Wikibase could be flagged as being an integration test for Scribunto. As such it would only be run for patchsets proposed for those two repositories, but not for a patch proposed to another extension which might happen to depend on both Wikibase and Scribunto.

That potentially could be added as @group statement so that in Wikibase, an integration test with Scribunto would use @group Scribunto. Then CI would:

  • on a patch to Wikibase
    • clone its dependencies and reverse dependencies
    • run all Wikibase tests via phpunit.php extensions/Wikibase
    • run all tests marked @group Wikibase or @group integration
  • on a patch to Scribunto
    • clone its dependencies
    • run all Scribunto tests via phpunit.php extensions/Scribunto
    • run all tests marked @group Scribunto or @group integration

Note that Scribunto does not have Wikibase defined as a dependency. But the wmf-quibble-* jobs have a hardcoded list of extensions being cloned in which includes both, thus a patchset to Scribunto would actually get Wikibase tests marked @group Scribunto.

For the other CI jobs (the one that get dependencies injected), we would need to inject the reverse dependencies. Right now Scribunto only have SyntaxHighlight_GeSHi injected and thus those jobs would not catch a breakage of Wikibase (but the wmf-quible job above do).


I am more than happy to see people jumping on this issue. I have been struggling with it with no good way to even write down a proper problem statement :-\ My status quo so far has been to just run everything.

greg removed a subscriber: greg.Jun 6 2019, 6:41 PM

OK, let's move this forward with a decision; the "blacklist" model, for simplicity:

  • we'll create a new group, "standalone";
  • we'll make quibble run phpunit with --group standalone for all patches to that repo, and --exclude-group standalone for all others; and
  • we'll have authors manually move tests to the new group, starting with the big ones in Scribunto, CirrusSearch, and Math that have already been discussed, with a plea to wikitech-l for people to find and add more.

Sound OK?