Page MenuHomePhabricator

phpunit splitter fails with obscure error message when a data provider is invalid
Closed, ResolvedPublic

Description

Seen in r1088394,21 but reproducible with the following dummy test:

<?php

class MyTest extends MediaWikiIntegrationTestCase {
	/**
	 * @dataProvider provideFoo
	 */
	public function testFoo() {
		$this->assertTrue( true );
	}

	public static function provideFoo() {
		$invalid = ThisDoesNotExist::FOO;
		return [ [] ];
	}
}

And running:

$ composer phpunit:prepare-parallel:extensions
> MediaWiki\Composer\PhpUnitSplitter\PhpUnitXmlManager::listTestsNotice

Running `phpunit --list-tests-xml` to get a list of expected tests ... 

> phpunit '--list-tests-xml=tests-list-extensions.xml' '--testsuite=extensions'
Using PHP 8.1.20
Running with MediaWiki settings because there might be integration tests
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Wrote list of tests that would have been run to tests-list-extensions.xml
> MediaWiki\Composer\PhpUnitSplitter\PhpUnitXmlManager::splitTestsListExtensions
Script MediaWiki\Composer\PhpUnitSplitter\PhpUnitXmlManager::splitTestsListExtensions handling the phpunit:prepare-parallel:extensions event terminated with an exception

In PhpUnitXmlManager.php line 87:
                                                                 
  Could not find file for class PHPUnit\Framework\ErrorTestCase  
                                                                 

phpunit:prepare-parallel:extensions [--dev] [--no-dev] [--] [<args>...]

The error message gives no hint of what the problem is, or where it is, or anything searchable:

Could not find file for class PHPUnit\Framework\ErrorTestCase

This is particularly problematic because this is the only feedback given by CI to the developer when parallel PHPUnit is enabled (i.e., all the other jobs pass, and all the relevant PHPUnit jobs fail with this message).

Event Timeline

I took a look at this. It's a bit involved because of the way phpunit deals with errors. When we list out the test cases, we run phpunit --list-tests-xml to generate a list of tests, and this quite happily runs to completion and puts something like the following in the generated XML for the bad test:

<testCaseClass name="PHPUnit\Framework\ErrorTestCase">
 <testCaseMethod name="Error" groups="DannyS712,Database"/>
 <testCaseMethod name="Error" groups="DannyS712,Database"/>
 <testCaseMethod name="Error" groups="DannyS712,Database"/>
 <testCaseMethod name="Error" groups="DannyS712,Database"/>
</testCaseClass>

Because that's the whole context that splitTestsListExtensions receives (reading the XML in a separate process), there's not a lot of extra information to provide. It looks like PHPUnit has more information internally about which file generates the error (since 9.5.10 - we use 9.6.19), but the implementation of the XmlTestListRenderer on the 9.6 branch is pretty basic and doesn't serialise these attributes. In PHPUnit 10, the ErrorTestCase class has been removed - I haven't looked into it, but I imagine another approach is used there.

At a first glance, I would say that we're approaching the limits of hacks we can apply to PHPUnit 9 - with PHPUnit 10, all of this (hopefully) becomes redundant. PHPUnit 9 is no longer receiving Bugfixes and will die before long.

What we can certainly do is make the error message here a bit more explanatory - that's low-hanging fruit. Besides that I wonder if this isn't another invitation to bump T345481 and T358394 up the priority list.

Thank you for looking into this!

Because that's the whole context that splitTestsListExtensions receives (reading the XML in a separate process), there's not a lot of extra information to provide.

That's unfortunate. How difficult would it be to fallback to non-parallel PHPUnit when that exception is caught? If it isn't too hard, I guess that would suffice as a temporary fix.

Besides that I wonder if this isn't another invitation to bump T345481 and T358394 up the priority list.

I'd LOVE to see those tasks resolved. Unfortunately, there's a decent amount of work involved and I don't know how realistic that would be, unless someone is willing to formally take up that work as part of say team goals.

Hello, while working on T359238 we found an instance of this bug with patch #1091464.

Frustratingly, it looks like this issue has also been reported and resolved in PHPUnit 10+ https://github.com/sebastianbergmann/phpunit/issues/5908 . In later versions --list-tests-xml itself prints a list of errors when the dataprovider has an issue.

Change #1092201 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/core@master] Display message when PHPUnit dataprovider has error in parallel run

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

Change #1092238 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/core@master] Fall back to linear tests if parallel test suite generation fails

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

Change #1092241 had a related patch set uploaded (by Arthur taylor; author: Arthur taylor):

[mediawiki/core@master] [DNM] Broken test case to test error logging

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

Change #1092241 abandoned by Arthur taylor:

[mediawiki/core@master] [DNM] Broken test case to test error logging

Reason:

Wrong repo - needs to be an "extensions"-suite test

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

Change #1092201 merged by jenkins-bot:

[mediawiki/core@master] Display message when PHPUnit dataprovider has error in parallel run

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

Change #1092238 merged by jenkins-bot:

[mediawiki/core@master] Fall back to linear tests if parallel test suite generation fails

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