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

CommentFileSizeAuthor
#37 3156998-37.patch21.05 KBalexpott
#35 3156998-35.patch21.06 KBalexpott
#35 32-35-interdiff.txt2.14 KBalexpott
#32 3156998-32.patch21.18 KBalexpott
#32 30-32-interdiff.txt2.3 KBalexpott
#30 3156998-29.patch21.17 KBalexpott
#30 26-29-interdiff.txt1.46 KBalexpott
#26 3156998-26.patch20.88 KBalexpott
#26 25-26-interdiff.txt1.09 KBalexpott
#25 3156998-25.patch20.9 KBalexpott
#25 23-25-interdiff.txt2.8 KBalexpott
#23 3156998-23.patch21.02 KBalexpott
#23 22-23-interdiff.txt669 bytesalexpott
#22 3156998-22.patch20.95 KBalexpott
#22 21-22-interdiff.txt833 bytesalexpott
#21 3156998-21.patch21.03 KBalexpott
#21 18-21-interdiff.txt1.01 KBalexpott
#18 3156998-18.patch21.37 KBalexpott
#18 16-18-interdiff.txt3.38 KBalexpott
#16 3156998-16.patch21.37 KBalexpott
#16 13-16-interdiff.txt646 bytesalexpott
#13 3156998-13.patch21.18 KBalexpott
#13 3156998-13.test-only.patch1.94 KBalexpott
#13 11-13-interdiff.txt1.17 KBalexpott
#11 3156998-11.patch20.76 KBalexpott
#11 10-11-interdiff.txt4.11 KBalexpott
#10 3156998-10.patch19.62 KBalexpott
#10 3156998-10.test-only.patch1.52 KBalexpott
#9 3156998-9.patch18.1 KBalexpott
#9 8-9-interdiff.txt13.76 KBalexpott
#8 3156998-8.patch12 KBalexpott
#8 6-8-interdiff.txt7.16 KBalexpott
#6 3156998-6.patch8.91 KBalexpott
#6 4-6-interdiff.txt3.2 KBalexpott
#4 3156998-4.patch7.71 KBalexpott
#2 3156998-2.patch1.16 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
1.16 KB

This 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.

alexpott’s picture

This is a very tricky issue. So many competing requirements.

Here are some details:

  • The Symfony deprecation error handler is registered during autoloading of files. There's nothing we can do to come before it because the autoloader is triggered by the PHPUnit executable.
  • We need to register our error handler in \Drupal\Tests\Listeners\DeprecationListenerTrait::deprecationStartTest before \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::startTest() because we need to come before the @expectedDeprecation error handler
  • We need to remove our error handler after \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::endTest because we need to be able to skip deprecations triggered by the debugclassloader.
alexpott’s picture

So 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.

Status: Needs review » Needs work

The last submitted patch, 4: 3156998-4.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
8.91 KB

Oh 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.

Status: Needs review » Needs work

The last submitted patch, 6: 3156998-6.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.16 KB
12 KB

I think this might be green. We might have to choose our poison here.

alexpott’s picture

Okay 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.

  1. They've implemented the ability to add expected deprecations (yay!)
  2. In doing the above they've made it a bit easier for us to wrap the Symfony Test listener.
alexpott’s picture

And here's a test that proves #9 fixes the problem.

alexpott’s picture

Now 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

alexpott’s picture

Issue summary: View changes
  1. +++ b/core/phpunit.xml.dist
    @@ -57,9 +57,6 @@
    -    <!-- The Symfony deprecation listener has to come after the Drupal listener -->
    -    <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener">
    -    </listener>
    

    This 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.

  2. +++ /dev/null
    @@ -1,24 +0,0 @@
    -/**
    - * Listens to PHPUnit test runs.
    - *
    - * @internal
    - */
    -class AfterSymfonyListener implements TestListener {
    

    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.

  3. Updated the issue summary.
    +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
    @@ -36,76 +38,9 @@ protected function addExpectedDeprecationMessage($message) {
    -  /**
    -   * Gets the SymfonyTestsListenerTrait.
    -   *
    -   * @return \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait|null
    -   *   The SymfonyTestsListenerTrait object or NULL is a Symfony test listener
    -   *   is not present.
    -   */
    -  private function getSymfonyTestListenerTrait() {
    -    $test_result_object = $this->getTestResultObject();
    -    $reflection_class = new \ReflectionClass($test_result_object);
    -    $reflection_property = $reflection_class->getProperty('listeners');
    -    $reflection_property->setAccessible(TRUE);
    -    $listeners = $reflection_property->getValue($test_result_object);
    -    foreach ($listeners as $listener) {
    -      if ($listener instanceof SymfonyTestsListener || $listener instanceof LegacySymfonyTestsListener) {
    -        $reflection_class = new \ReflectionClass($listener);
    -        $reflection_property = $reflection_class->getProperty('trait');
    -        $reflection_property->setAccessible(TRUE);
    -        return $reflection_property->getValue($listener);
    -      }
    

    This is lovely to see go. Very ugly how we had to use reflection to access Symfony's internals.

alexpott’s picture

Ah wow the fail case is even more specific there needs to be a data provider involved.

alexpott’s picture

Issue summary: View changes

The last submitted patch, 13: 3156998-13.test-only.patch, failed testing. View results

alexpott’s picture

I did the composer update using composer 2.0 by mistake - so

+++ b/composer.lock
@@ -7188,5 +7191,5 @@
-    "plugin-api-version": "1.1.0"
+    "plugin-api-version": "2.0.0"

was changed... it doesn't matter much but it's better to be correct.

mondrake’s picture

Nice cleanup, independently from the PHP8 support.

Nit, we could add return typehints to newly introduced functions:

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeRequiresTest.php
@@ -0,0 +1,39 @@
+  public function testWillNeverRun() {
...
+  public function providerTestWillNeverRun() {

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php
@@ -26,4 +26,14 @@ public function testDeprecatedFunction() {
+  public function testWillNeverRun() {

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -208,37 +203,41 @@ public static function getSkippedDeprecations() {
+  protected function registerErrorHandler() {
...
+  protected function removeErrorHandler() {
alexpott’s picture

registerErrorHandler 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.

alexpott’s picture

Oh and @mondrake thanks the review! It'd be great if you wanted to become a test sub-system maintainer btw :)

mondrake’s picture

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -208,37 +203,41 @@ public static function getSkippedDeprecations() {
+        // @todo long and winding comment.

I guess this should be unwinded :) before RTBC...

alexpott’s picture

Ah that should no longer be necessary.

alexpott’s picture

Proper revert of that change.

alexpott’s picture

Some docs fixes.

alexpott’s picture

Status: Needs review » Needs work

Found a bug in expected deprecations from inside an isolated test in the symfony code. Working around it.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
20.9 KB

So 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.

alexpott’s picture

From working on the SF issue (https://github.com/symfony/symfony/pull/37515) worked out how to get the assertion count correct.

mondrake’s picture

In 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.

mondrake’s picture

@alexpott do you deem the patch here complete, or you still have todos?

Couple of more nits

  1. +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php
    @@ -26,4 +26,14 @@ public function testDeprecatedFunction() {
    +  /**
    +   * Tests the @requires annotation.
    +   *
    +   * @requires extension will_hopefully_never_exist
    +   */
    +  public function testWillNeverRun(): void {
    

    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.

  2. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
    @@ -34,40 +31,13 @@ protected function addExpectedDeprecationMessage($message) {
    +   * @see \Symfony\Bridge\PhpUnit\Legacy\ExpectDeprecationTraitForV8_4::expectDeprecation()
    

    This seems to be locked down to a specific PHPUnit 8 release, will a PHPUnit 9 one have a different signature?

  3. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
    @@ -80,31 +50,15 @@ public function expectedDeprecations(array $messages) {
    +      // Copy code from ExpectDeprecationTraitForV8_4::expectDeprecation().
    

    same

mondrake’s picture

Re. #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.

alexpott’s picture

Fixed #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/37515

And 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.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, RTBC for me. Draft CR and release note snippet are there, deprecation follow up filed.

Nits:

  1. +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeRequiresTest.php
    @@ -0,0 +1,42 @@
    + * This test will skipped and should not cause the test suite to fail.
    

    This test will be skipped...

  2. +++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php
    @@ -26,4 +26,17 @@ public function testDeprecatedFunction() {
    +   * This test method will skipped and should not cause the test suite to fail.
    

    same

  3. +++ b/core/tests/Drupal/Tests/Listeners/DrupalListener.php
    @@ -33,12 +43,46 @@ class DrupalListener implements TestListener {
    +   * The wrapped symfony test listener.
    

    Symfony with capital S

  4. +++ b/core/tests/Drupal/Tests/Listeners/DrupalListener.php
    @@ -59,9 +103,30 @@ public function startTest(Test $test): void {
    +    // The Drupal error handler has to be removed after the symfony error
    

    same

alexpott’s picture

Addressing #31 - thanks!

jibran’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Bug Smash Initiative

Patch looks good to me. I just have one question and minor docs improvements suggestions.

  1. If you change * @requires extension yaml to * @requires extension nope

    Why would we do this?

  2. +++ b/core/tests/Drupal/Tests/Listeners/DrupalListener.php
    @@ -59,9 +103,30 @@ public function startTest(Test $test): void {
    +      $className = \get_class($test);
    ...
    +      if (\in_array('legacy', $groups, TRUE)) {
    

    We can remove \ here.

  3. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
    @@ -15,7 +11,8 @@
    + * @todo https://www.drupal.org/project/drupal/issues/3157434 Deprecate the
    + *   class and its methods.
    

    This is not a class. Let's add link at the end of the sentence.

  4. +++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
    @@ -34,40 +31,13 @@ protected function addExpectedDeprecationMessage($message) {
    +    // @todo Until https://github.com/symfony/symfony/pull/37515 is resolved
    

    This is merged.

mayurjadhav’s picture

Assigned: Unassigned » mayurjadhav
alexpott’s picture

Assigned: mayurjadhav » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
2.14 KB
21.06 KB

Thanks for the review @jibran.

  1. To simulate what happens if you don't have an extension that a test requires. So you can see how PHP 8 failed with it didn't have the yaml extension. Or locally it fails for me because I don't have the yaml extension turned on.
  2. Sure we can and I will but this is actually code copied from Symfony. Doing \built_in_method is actually a performance improvement because PHP can then skip checking whether Drupal\Tests\Listeners\in_array() exists.
  3. Changed to trait. Personally I prefer the link at the front of a @todo for two reasons. It's actually what is important and URL parsers work better because you're not dealing with the final fullstop which or may not be part of the URL.
  4. Fixed - yeah here we can rely on the @todo to deprecate / remove all of this.

Setting back to rtbc because #33 was only nits.

catch’s picture

Issue tags: +Needs reroll

Needs a reroll, haven't reviewed yet.

alexpott’s picture

#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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 3156998-37.patch, failed testing. View results

ravi.shankar’s picture

Issue tags: -Needs reroll

Removed needs reroll tag.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Random JS test fails...

  • catch committed 8af8df7 on 9.1.x
    Issue #3156998 by alexpott, mondrake, jibran: Using @requires...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8af8df7 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: +9.1.0 release notes