8000 Check that we are not using features deprecated in PHPUnit 8 · Issue #30055 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Check that we are not using features deprecated in PHPUnit 8 #30055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
javiereguiluz opened this issue Feb 1, 2019 · 16 comments
Closed

Check that we are not using features deprecated in PHPUnit 8 #30055

javiereguiluz opened this issue Feb 1, 2019 · 16 comments

Comments

@javiereguiluz
Copy link
Member

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
Copy link
Contributor
ro0NL commented Feb 1, 2019 •
edited
Loading

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
Copy li 8000 nk
Contributor

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
Copy link
Contributor

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

@Pierstoval
Copy link
Contributor
Pierstoval commented Feb 3, 2019 •
edited
Loading

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
Copy link
Member
derrabus commented Feb 4, 2019

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
Copy link
Member
derrabus commented Feb 4, 2019

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
Copy link
Member
xabbuh commented Feb 4, 2019

see also #30071

@derrabus
Copy link
Member
derrabus commented Feb 4, 2019

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
Copy link
Member
derrabus commented Feb 4, 2019

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
Copy link
Member

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
Copy link
Member
derrabus commented Feb 4, 2019

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
Copy link
Member

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

@derrabus
Copy link
Member
derrabus commented Feb 4, 2019

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

@alexander-schranz
Copy link
Contributor
alexander-schranz commented Feb 5, 2019 •
edited
Loading

@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 alexander-schranz mentioned this issue Feb 5, 2019
Merged
@alexander-schranz
Copy link
Contributor
alexander-schranz commented Feb 6, 2019 •
edited
Loading

@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

@ro0NL ro0NL mentioned this issue Feb 25, 2019
Closed
@stof stof mentioned this issue Apr 2, 2019
Closed
nicolas-grekas added a commit that referenced this issue Apr 8, 2019
@nicolas-grekas
bug #30085 Fix TestRunner compatibility to PhpUnit 8 (alexander-schranz)
d7a8b3d 
This PR was squashed before being merged into the 3.4 branch (closes #30085).

Discussion
----------

Fix TestRunner compatibility to PhpUnit 8

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | related to: #30055
| License       | MIT
| Doc PR        | -

Modify the installed phpunit version to be compatibility with the symfony custom TestRunner. This is sure not the best way but maybe currently the fastest way to support PhpUnit 8. The hack should be removed as soon as there is another way to implement a custom Runner.

Commits
-------

a0c66a3 Fix TestRunner compatibility to PhpUnit 8
@javiereguiluz
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
495A
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@javiereguiluz @jverdeyen @nicolas-grekas @ro0NL @derrabus @alexander-schranz @xabbuh @Pierstoval
0