Check that we are not using features deprecated in PHPUnit 8

[javiereguiluz]

Description
PHPUnit 8 has just been released. In addition to new features, the following has been deprecated and will be removed in PHPUnit 9:

• assertInternalType() and assertNotInternalType()
• assertArraySubset()
• Annotation(s) for expecting exceptions
• Assertions (and helper methods) that operate on (non-public) attributes
• Optional parameters of assertEquals() and assertNotEquals()
• TestListener interface
• Optional parameters of assertContains() and assertNotContains() as well as using these methods with string haystacks

Full details at https://phpunit.de/announcements/phpunit-8.html

Let's review our tests to make sure we don't use these. Thanks.

[ro0NL]

First reports running my own test suite, related to simple-phpunit:

Fatal error: Class Symfony\Bridge\PhpUnit\Legacy\TestRunnerForV7
may not inherit from final class (PHPUnit\TextUI\TestRunner)
in ./vendor/symfony/phpunit-bridge/Legacy/TestRunnerForV7.php on line 22

Fatal error: Declaration of Symfony\Component\Form\Test\FormIntegrationTestCase::setUp()
must be compatible with PHPUnit\Framework\TestCase::setUp(): void
in ./vendor/symfony/form/Test/FormIntegrationTestCase.php on line 57

[Pierstoval]

Yep, tearDown() and some other methods need a : void return type, so Symfony will need to update this too in order to be compatible 🤔

[jverdeyen]

@Pierstoval with the exception of downgrading, is there a workaround for this? Any idea?

[Pierstoval]

Well, with this thing about variance & co, it seems we can already add the void return type without breaking existing code: https://3v4l.org/7eIlr

class A {
    protected function tearDown()
    {
    }
}

class B extends A {
    protected function tearDown(): void // Works
    {
    }
}

[derrabus]

Yes, the return type can already be added to existing tests. However, we cannot do this in the 3.4 branch as we need to support php 5 there. The consequence is that the Symfony 3.4 test suite will be incompatible with PHPUnit 8 forever. But this is fine, I guess.

On the other hand, if we fix this in 4.2/master only, changes to the 3.4 test suite will cause more conflicts when merging them to 4.2/master.

[derrabus]

PHPUnit Bridge uses the deprecated TestListener interface. That interface has been replaced by at set of TestHook interfaces. This also relates to the error that @ro0NL reported because the bridge overrides the now-final TestRunner class in order to interact with the registered SymfonyTestListener instance.

[xabbuh]

see also #30071

[derrabus]

Yes, the return type can already be added to existing tests. However, we cannot do this in the 3.4 branch as we need to support php 5 there. The consequence is that the Symfony 3.4 test suite will be incompatible with PHPUnit 8 forever. But this is fine, I guess.

Alternatively: We could rename all setUp() and tearDown() methods and annotate them with @before and @after. We wouldn't need to add : void in that case.

[derrabus]

Regarding assertContains and assertNotContains with string haystacks: We do this in Symfony\Bridge\PhpUnit\Tests\CoverageListenerTest for instance. Unfortunately, the replacements were added with PHPUnit 8, so without breaking PHPUnit 5/6/7 compatibility we can only fix this by…

• … duplicating the new assertStringContainsString() method inside the Symfony codebase.
• ... using a more verbose assertNotFalse(strpos(…)) assertion.

[nicolas-grekas]

I think we should focus on the bridge only for now as that's the blocking part - as a bug fix if possible.
About the test suite, that's of a much lower priority to me.

[derrabus]

I think we should focus on the bridge only for now as that's the blocking part

The blocking part would be that we cannot extend PHPUnit's test runner anymore. That class is final now. If I simply alias Symfony's test runner to PHPUnit's default one, my project test suites work again.

I must admit, I don't fully understand the purpose of that extended test runner, so I'm having a hard time finding a replacement for it.

[nicolas-grekas]

Its purpose is to automatically register the SymfonyTestsListener without the need to explicitly list it in phpunit.xml files.

[derrabus]

Can't we do this in the Command class somehow and use the vanilla runner? 🤔

[alexander-schranz]

@javiereguiluz

Maybe helpfull I used the following regex to upgrade our phpunit tests to 6.5 some time ago:

setExpectedException
expectException

\n(     \* )@expectExceptionMessage (.*?)(\n)([\s\S^?]+?)(\{)
$3$4$5\n        \$this->expectExceptionMessage(\'$2\');

\n(     \* )@expectException (.*?)(\n)([\s\S^?]+?)(\{)
$3$4$5\n        \$this->expectException($2::class);

Some other regex can be found here: sulu/sulu#3931

[alexander-schranz]

@nicolas-grekas @derrabus not the best way but as the bridge itself installed a modified version of the phpunit we could just remove the final 🙈 : #30085

[javiereguiluz]

Let's close this because it's not very actionable and because we're already working continuously on our test suite, so we won't miss a single change or deprecation. Thanks.