PHPUnit 8 support for the bridge: SymfonyTestsListenerForV7 implements TestListener that is deprecated, we need to use TestHook instead

[nicolas-grekas]

SymfonyTestsListenerForV7 implements TestListener that is deprecated, we need to use TestHook instead

/cc @greg0ire maybe?

[greg0ire]

I will give it a try :)

[greg0ire]

First hurdle: it's unclear to me what the migration path is for

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Lines 112 to 179 in c1bca29

	public function startTestSuite($suite)
	{
	$suiteName = $suite->getName();
	$this->testsWithWarnings = array();
	foreach ($suite->tests() as $test) {
	if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)) {
	continue;
	}
	if (null === Test::getPreserveGlobalStateSettings(\get_class($test), $test->getName(false))) {
	$test->setPreserveGlobalState(false);
	}
	}
	if (-1 === $this->state) {
	echo "Testing $suiteName\n";
	$this->state = 0;
	if (!class_exists('Doctrine\Common\Annotations\AnnotationRegistry', false) && class_exists('Doctrine\Common\Annotations\AnnotationRegistry')) {
	if (method_exists('Doctrine\Common\Annotations\AnnotationRegistry', 'registerUniqueLoader')) {
	AnnotationRegistry::registerUniqueLoader('class_exists');
	} else {
	AnnotationRegistry::registerLoader('class_exists');
	}
	}
	if ($this->skippedFile = getenv('SYMFONY_PHPUNIT_SKIPPED_TESTS')) {
	$this->state = 1;
	if (file_exists($this->skippedFile)) {
	$this->state = 2;
	if (!$this->wasSkipped = require $this->skippedFile) {
	echo "All tests already ran successfully.\n";
	$suite->setTests(array());
	}
	}
	}
	$testSuites = array($suite);
	for ($i = 0; isset($testSuites[$i]); ++$i) {
	foreach ($testSuites[$i]->tests() as $test) {
	if ($test instanceof TestSuite) {
	if (!class_exists($test->getName(), false)) {
	$testSuites[] = $test;
	continue;
	}
	$groups = Test::getGroups($test->getName());
	if (\in_array('time-sensitive', $groups, true)) {
	ClockMock::register($test->getName());
	}
	if (\in_array('dns-sensitive', $groups, true)) {
	DnsMock::register($test->getName());
	}
	}
	}
	}
	} elseif (2 === $this->state) {
	$skipped = array();
	foreach ($suite->tests() as $test) {
	if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)
	|| isset($this->wasSkipped[$suiteName]['*'])
	|| isset($this->wasSkipped[$suiteName][$test->getName()])) {
	$skipped[] = $test;
	}
	}
	$suite->setTests($skipped);
	}
	}

, since BeforeFirstTestHook, which seems to be the best candidate for this, does not have access to the current suite.

Its only method is not even called here: https://github.com/sebastianbergmann/phpunit/blob/154de22df1fda1d67758ab1c417603df7b290809/src/Runner/Hook/TestListenerAdapter.php#L130-L132, but directly in the test runner: https://github.com/sebastianbergmann/phpunit/blob/41c75847e0a7e7df2e0eba6b6edf4d1b99da3103/src/TextUI/TestRunner.php#L535-L539

[greg0ire]

I created this PR in order to clarify things: sebastianbergmann/phpunit#3800

[greg0ire]

Ok so it looks like we should also stop relying on test suites entirely, and find another way to get all tests.

[greg0ire]

So why do we rely on test suites?

1. For starters, we use them to get all tests and disable preserveGlobalState on them, which is a saner default than having it enabled by default. (see [Bridge\PhpUnit] Disable broken auto-require mechanism of phpunit #25032):

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Lines 117 to 124 in b9ba0c5

	foreach ($suite->tests() as $test) {
	if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)) {
	continue;
	}
	if (null === Test::getPreserveGlobalStateSettings(\get_class($test), $test->getName(false))) {
	$test->setPreserveGlobalState(false);
	}
	}

2. It's then used to print the test suite name:

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Line 127 in b9ba0c5

echo "Testing $suiteName\n";

3. Then, it is used to achieve the feature of replaying skipped tests by replacing the tests with an empty array if the file is present and empty (meaning the test suite was run, and there were no skipped tests):

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Lines 138 to 149 in b9ba0c5

	if ($this->skippedFile = getenv('SYMFONY_PHPUNIT_SKIPPED_TESTS')) {
	$this->state = 1;
	if (file_exists($this->skippedFile)) {
	$this->state = 2;
	if (!$this->wasSkipped = require $this->skippedFile) {
	echo "All tests already ran successfully.\n";
	$suite->setTests(array());
	}
	}
	}

Or by reading the file, and replacing the suite's tests with the tests found in the file:

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Lines 168 to 178 in b9ba0c5

	} elseif (2 === $this->state) {
	$skipped = array();
	foreach ($suite->tests() as $test) {
	if (!($test instanceof \PHPUnit\Framework\TestCase || $test instanceof TestCase)
	|| isset($this->wasSkipped[$suiteName]['*'])
	|| isset($this->wasSkipped[$suiteName][$test->getName()])) {
	$skipped[] = $test;
	}
	}
	$suite->setTests($skipped);
	}

The last piece of code using test suites is that one:

symfony/src/Symfony/Bridge/PhpUnit/Legacy/SymfonyTestsListenerTrait.php

Lines 151 to 167 in b9ba0c5

	for ($i = 0; isset($testSuites[$i]); ++$i) {
	foreach ($testSuites[$i]->tests() as $test) {
	if ($test instanceof TestSuite) {
	if (!class_exists($test->getName(), false)) {
	$testSuites[] = $test;
	continue;
	}
	$groups = Test::getGroups($test->getName());
	if (\in_array('time-sensitive', $groups, true)) {
	ClockMock::register($test->getName());
	}
	if (\in_array('dns-sensitive', $groups, true)) {
	DnsMock::register($test->getName());
	}
	}
	}
	}

But I don't understand it… phpunit test suites can have test suites in their test suites? Can somebody explain?

EDIT: I got an explanation from Nicolas Grekas: apparently, the sub-test suite can be used when there are data providers. The use case of this code block is to inject ClockMock functions in the namespace before it is used.

[greg0ire]

Sebastian Bergmann wrote:

The concept of a "test suite" is one that should, and will, eventually vanish. Therefore there is no equivalent in the new API.

Yet test suite related methods are not marked as deprecated yet, so I think I can ignore this for the moment and just work on following the deprecations for the other methods.

EDIT: I noticed I need to do the something similar but simpler for CoverageTestListener, so I started working on a CoverageHook class… then I noticed the new signature: public function executeBeforeTest(string $test): void. So apparently we only have access to a string, no Test instance… not sure what to do with this string 🤦‍♂️

The main discussion happens here: sebastianbergmann/phpunit#3390

[nicolas-grekas]

ping @epdenouden

[nicolas-grekas]

For reference, the doc has some reminders about the features: https://symfony.com/doc/current/components/phpunit_bridge.html

[rbaarsma]

Not sure if somebody made a PR already, but the solution is to use extensions (possible from phpunit >= 7.1) instead of listeners.

The CHANGELOG should reflect that the phpunit.xml.dist should be changed to use the new feature.

The code would be something like this:

class CoverageExtension implements BeforeTestHook
{
    public function __construct(callable $sutFqcnResolver = null, $warningOnSutNotFound = false)
    {
        $this->trait = new CoverageListenerTrait($sutFqcnResolver, $warningOnSutNotFound);
    }

    public function executeBeforeTest(string $test): void
    {
        $this->trait->startTest($test);
    }
}

And the phpunit.xml.dist would be to remove the listener and instead add an extension:

    <extensions>
        <extension class="Symfony\Bridge\PhpUnit\Legacy\CoverageExtension" />
    </extensions>

Note that CoverageExtension is just one of the phpunit-bridge extensions. The same system applies to other existing extensions.

I could write the code, but don't want to interfere with existing work on this.

[greg0ire]

@rbaarsma your solution will not work for the more complicated SymfonyTestListener I'm afraid, but you can still make a PR just for the CoverageListener I guess 👍

[rbaarsma]

@greg0ire after some more examination and attempted solution, I come to the same conclusion as you already said before. The string $test given is not enough to cover the use-case. I assumed this was the test class, but it can be SelfDescribing and then be really only a string.

My conclusion is that the current hook implementation is not enough to migrate these classes. Perhaps we should contact @sebastianbergmann to discuss what his idea is with the current hook implementation and how we are supposed to use the minimum input to cover the use-cases of symfony, such as access to the annotations. testResultObject, status, and failures.

Also the SymfonyTestListener uses a globalListenerDisabled function that does not seem to have a hook.

[epdenouden]

Hello everyone!
including @greg0ire @nicolas-grekas @sebastianbergmann @localheinz and @theseer

At the SymfonyCon in Amsterdam last weekend we decided on refactoring both in PHPUnit and the Symfony test collection, which will remove the need for a bridge altogether. PHPUnit will get new features that will bring much of the bridge functionality to its core, whereas any missing Symfony-specific requirements will be handled with a proper PHPUnit extension.

The upcoming event-based logging system in PHPUnit allows for much better logging and analysis of -you guessed it!- events happening during testing. Thanks to the work by @localheinz and @theseer we will be able to report many new events in much more detail.

@sebastianbergmann and I discussed this with @greg0ire and @nicolas-grekas last Saturday and I will write up a list of what is needed where and how to approach this.

Also, we propose a communal bonfire during a conference next year to burn all the legacy code.

[derrabus]

On PHPUnit 9, our TestListener is broken.

Nevermind, that's actually the test listener of the polyfills package. I should've read the error message more carefully. 🙈

[carsonbot]

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

[carsonbot]

Hello? This issue is about to be closed if nobody replies.

[carsonbot]

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!