Problem/Motivation
If you change * @requires extension yaml
to * @requires extension nope
in \Drupal\Tests\Component\Serialization\YamlPeclTest and run the test you'll see:
OK, but incomplete, skipped, or risky tests!
Tests: 40, Assertions: 0, Skipped: 7.
THE ERROR HANDLER HAS CHANGED!
Other deprecation notices (2)
1x: The "Drupal\Tests\Listeners\DrupalListener" class implements "PHPUnit\Framework\TestListener" that is deprecated Use the `TestHook` interfaces instead.
1x: The "Drupal\Tests\Listeners\DrupalListener" class uses "PHPUnit\Framework\TestListenerDefaultImplementation" that is deprecated The `TestListener` interface is deprecated.
Process finished with exit code 1
The problem is the test is marked as failed because THE ERROR HANDLER HAS CHANGED!
is outputted from \Symfony\Bridge\PhpUnit\DeprecationErrorHandler::shutdown() and deprecations that should be skipped are listed. This happens when the error handler that's registered in \Drupal\Tests\Listeners\DeprecationListenerTrait::registerErrorHandler() is not working as expected.
Proposed resolution
- Upgrade symfony/phpunit-bridge
- Add test for @requires annotation on the test class level with a data provider
- Convert our test listener to leverage the new symfony/phpunit-bridge functionality.
Remaining tasks
User interface changes
None
API changes
The changes made here all made to classes marked @internal and concern how Drupal integrates with Symfony's deprecation reporting.
- symfony/phpunit-bridge upgraded to ^5.1 - this bridge is designed to be compatible with all versions of Symfony so there is no actual API change - just new additions that we leverage.
- \Drupal\Tests\Listeners\AfterSymfonyListener is removed. This class is @internal and only ever used by DrupalListener.
Data model changes
None
Release notes snippet
The dev-only dependency symfony/phpunit-bridge is upgraded to 5.1.2. The Symfony\Bridge\PhpUnit\SymfonyTestsListener
listener should be removed from any custom phpunit.xml files. For more information read the change record: https://www.drupal.org/node/3157433
Comment | File | Size | Author |
---|---|---|---|
#37 | 3156998-37.patch | 21.05 KB | alexpott |
#35 | 3156998-35.patch | 21.06 KB | alexpott |
#35 | 32-35-interdiff.txt | 2.14 KB | alexpott |
#32 | 3156998-32.patch | 21.18 KB | alexpott |
#32 | 30-32-interdiff.txt | 2.3 KB | alexpott |
Comments
Comment #2
alexpottThis fixes the
THE ERROR HANDLER HAS CHANGED!
but unfortunately it does not fix the problem but our deprecations which need skipping are not skipped.Let's see if this causes any other problems.
Comment #3
alexpottThis is a very tricky issue. So many competing requirements.
Here are some details:
Comment #4
alexpottSo this fixes the issue locally. It does it by removing the listener Symfony\Bridge\PhpUnit\SymfonyTestsListener and then re-implementing inside \Drupal\Tests\Listeners\DrupalListener so we can registered our skipping deprecations nice and early and remove it nice and late. And it can do this without having to reach inside the internals of PHPUnit - which doesn't always work. The problem is that data providers with an @requires extension don't get a test result object that we can futz with to registered the AfterSymfonyListener.
This will need a CR because anyone that has customised a phpunit.xml will need to remove some lines from it. I can't think yet of anyway to automate that or provide BC.
Comment #6
alexpottOh gosh the order that everything happens is so fragile. Here's a fix. Once I've got this green going to try to add copious documentation.
Comment #8
alexpottI think this might be green. We might have to choose our poison here.
Comment #9
alexpottOkay so in fact we don't have to choose our poison. We can update to the latest Symfony PHPUnit bridge. This gives us two things that makes everything a bit simpler.
Comment #10
alexpottAnd here's a test that proves #9 fixes the problem.
Comment #11
alexpottNow with promised documentation. Also filed the follow-up to complete the deprecation of \Drupal\Tests\Traits\ExpectDeprecationTrait - #3157434: Deprecate \Drupal\Tests\Traits\ExpectDeprecationTrait in favour of \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait
Comment #12
alexpottThis looks to be the disruptive part of the change. But actually deprecation and regular testing still pass if this listener is registered. It's just no longer necessary and will do unnecessary work if it is present.
I think it is okay to remove rather than go through deprecation as this as this class is marked as @internal, can only be autoloaded by tests, and was only used from DrupalListener.
And what's nice about the new approach is that all the error loader futzing takes place in DrupalListener so it's a bit easier to follow along.
This is lovely to see go. Very ugly how we had to use reflection to access Symfony's internals.
Comment #13
alexpottAh wow the fail case is even more specific there needs to be a data provider involved.
Comment #14
alexpottComment #16
alexpottI did the composer update using composer 2.0 by mistake - so
was changed... it doesn't matter much but it's better to be correct.
Comment #17
mondrakeNice cleanup, independently from the PHP8 support.
Nit, we could add return typehints to newly introduced functions:
Comment #18
alexpottregisterErrorHandler is pre-exsiting. So I'll not change that. I've removed a parameter but that's fine. This is all @internal and test infra so imo we can add typehints whenever but better safe than sorry.
I've also investigated the expectDeprecation and risky test situation. Found a bug in the Symfony code and opened https://github.com/symfony/symfony/pull/37515. For now the $this->addToAssertionCount(1) is okay. It's the correct fix. It does mean that that number of assertions on tests that use $this->expectDeprecation() and are not run in isolation will report an elevated number of assertions but I think that's a very small price to pay. Tests still fail when there are no assertions of any kind.
Comment #19
alexpottOh and @mondrake thanks the review! It'd be great if you wanted to become a test sub-system maintainer btw :)
Comment #20
mondrakeI guess this should be unwinded :) before RTBC...
Comment #21
alexpottAh that should no longer be necessary.
Comment #22
alexpottProper revert of that change.
Comment #23
alexpottSome docs fixes.
Comment #24
alexpottFound a bug in expected deprecations from inside an isolated test in the symfony code. Working around it.
Comment #25
alexpottSo the Symfony code has a bug that expectDeprecation does not work in isolated tests. Fortunately we have the plumbing all there to make it work.
This can be tested by altering \Drupal\Tests\ExpectDeprecationTest::testExpectDeprecationInIsolation() so that $this->addExpectedDeprecationMessage('Test isolated deprecation'); is expecting a different message. #23 but the patch attached here will not. I'm going to try to fix this upstream in https://github.com/symfony/symfony/pull/37515 but considering we're leveraging existing plumbing I don't think that's a reason to not proceed here.
Comment #26
alexpottFrom working on the SF issue (https://github.com/symfony/symfony/pull/37515) worked out how to get the assertion count correct.
Comment #27
mondrakeIn https://www.drupal.org/project/drupal/issues/3113423#comment-13737715, currently testing the patch here in combination with #3127141-55: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3 to check any impact when upgrading to PHPUnit 9.
Comment #28
mondrake@alexpott do you deem the patch here complete, or you still have todos?
Couple of more nits
I think here (and in the other similar test) a few words in the doc, of what is expected, would help. In fact AFAICS, we expect that the missing extension will lead to the test not being run at all, so the class-level deprecation that would theoritically fire as soon as the autloader will load the class as part of the test execution, will not be triggered.
This seems to be locked down to a specific PHPUnit 8 release, will a PHPUnit 9 one have a different signature?
same
Comment #29
mondrakeRe. #27, couple of failures there, related to method signatures apparently. Something to be dealt with in #3127141: Use PHPUnit 9 for PHP 7.4+, while keeping support for PHPUnit 8.4 in PHP 7.3 IMO.
Comment #30
alexpottFixed #28
Re 2 and 3. I don't think it will change for PHPUnit 9 but that's not the point. We're copying the internals. We could point to ExpectDeprecationTraitBeforeV8_4 it'd make no difference. And pointing to \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait would only confuse people more. Also eventually all this will be removed in #3157434: Deprecate \Drupal\Tests\Traits\ExpectDeprecationTrait in favour of \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait and we'll be using
\Symfony\Bridge\PhpUnit\ExpectDeprecationTrait
and$this->expectDeprecation()
(from Symfony). But that can't happen until they fix https://github.com/symfony/symfony/pull/37515And yep I consider this patch correct. Test fail and pass as expected. If we want to add coverage of failing tests we need to rewrite test discovery and allow .phpt type tests (see https://github.com/symfony/symfony/pull/37515/files#diff-99e462bbc782cd4... for how they would work). Which will be a lot of work.
Comment #31
mondrakeThank you, RTBC for me. Draft CR and release note snippet are there, deprecation follow up filed.
Nits:
This test will be skipped...
same
Symfony with capital S
same
Comment #32
alexpottAddressing #31 - thanks!
Comment #33
jibranPatch looks good to me. I just have one question and minor docs improvements suggestions.
Why would we do this?
We can remove \ here.
This is not a class. Let's add link at the end of the sentence.
This is merged.
Comment #34
mayurjadhav CreditAttribution: mayurjadhav at Srijan | A Material+ Company commentedComment #35
alexpottThanks for the review @jibran.
Setting back to rtbc because #33 was only nits.
Comment #36
catchNeeds a reroll, haven't reviewed yet.
Comment #37
alexpott#2619482: Convert all get_called_class()/get_class() to static:: caused the conflict. We no longer need the class name in core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
Comment #39
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedRemoved needs reroll tag.
Comment #40
alexpottRandom JS test fails...
Comment #42
catchCommitted 8af8df7 and pushed to 9.1.x. Thanks!
Comment #44
xjm