Page MenuHomePhabricator

expectOutputString() seems to be broken for Maintenance script tests
Closed, ResolvedPublic

Event Timeline

Reedy renamed this task from MaintenanceBaseTestCase::expectOutputString() seems to be broken to expectOutputString() seems to be broken for Maintenance script tests.Tue, Jun 2, 4:24 PM
Reedy triaged this task as High priority.

This could potentially be an Upstream issue in PHPUnit...

The problem is that MaintenanceBaseTestCase sets a default expectOutputRegex and using both expectation functions is not supported (setting the default was moved for PHPUnit in 01de75b5145802406742586c76f0cacf413b3ac7).

So the regex part always win. Upstream could warn when both are used, but the fault seems in ourer base class.

PHPUnit 9:

// Perform assertion on output.
if (!isset($e)) {
    try {
        if ($this->outputExpectedRegex !== null) {
            $this->assertMatchesRegularExpression($this->outputExpectedRegex, $this->output);
        } elseif ($this->outputExpectedString !== null) {
            $this->assertEquals($this->outputExpectedString, $this->output);
        }
    } catch (Throwable $_e) {
        $e = $_e;
    }
}

So the fix is to keep a track of when either method is called and if they are then don't call ::expectOutputRegex?

I'm sure we could override ::expectOutputRegex and ::expectOutputString to do this in addition to us throwing if both are called?

Change #1297086 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/core@master] Prevent calls to both expectOutputString and expectOutputRegex

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

Change #1297091 had a related patch set uploaded (by Dreamy Jazz; author: Dreamy Jazz):

[mediawiki/extensions/CheckUser@master] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297091 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@master] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297139 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/CentralAuth@master] PopulateHomeDBTest: Fixup test assertion

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

Change #1296633 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/extensions/OATHAuth@master] tests: Update tests using expectOutputString()

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

Change #1297132 merged by jenkins-bot:

[mediawiki/extensions/CheckUser@REL1_46] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297135 merged by Reedy:

[mediawiki/extensions/CheckUser@REL1_43] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297134 merged by Reedy:

[mediawiki/extensions/CheckUser@REL1_44] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297133 merged by Reedy:

[mediawiki/extensions/CheckUser@REL1_45] Fix broken expectation in PopulateCheckUserTableTest.php

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

Change #1297139 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] PopulateHomeDB: Break early if we get no rows

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

Change #1297086 merged by jenkins-bot:

[mediawiki/core@master] Prevent calls to both expectOutputString and expectOutputRegex

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

Change #1296633 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] tests: Update tests using expectOutputString()

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

Do we want to keep this open for making this work for PHPUnit 10?

Krinkle subscribed.

Reported at https://github.com/sebastianbergmann/phpunit/issues/6700.

Do we want to keep this open for making this work for PHPUnit 10?

Looks like PHPUnit 10.3 renames the internal hasExpectationOnOutput to expectsOutput (upstream commit in 10.x), which is straight-forward enough that we can handle that as part of the MediaWiki change that upgrades PHPUnit.

More recently the now-private remnants of hasExpectationOnOutput to another class (commit in 14.x-dev), but that doesn't affect us.

This is already fixed upstream by adding a phpunit warning for v12 and v13.

The suggested expectsOutput is marked @internal to phpunit, but is the best short time solution to handle this situation. Added notice to T328919 to track the necessary adjustments.