Problem/Motivation

Steps to reproduce:

  1. rm core/tests/Drupal/Tests/Component/Utility/XssTest.php
  2. phpunit --testsuite unit

And you see failures in Drupal\Tests\Core\Routing\RedirectDestinationTest and Drupal\Tests\Core\Path\PathValidatorTest. This is because Drupal\Tests\Component\Utility\UrlHelperTest ends up with setting only http as a valid protocol. When XssTest is around this fixes this before the tests that are failing are run.

This is mess. Statics are awful for unit tests.

Proposed resolution

As of Drupal 10.1.x the issue does not occur - but that's probably because of test ordering which is something we should not be relying.

The best solution here is to run tests that mess with statics in a separate process using PHPUnit's annotations. Since we use run-tests.sh on DrupalCI it was never affected because it runs each PHPUnit test individually.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#23 2503063-2-23.patch3.23 KBalexpott
#23 19-23-interdiff.txt1.74 KBalexpott
#19 2503063-2-19.patch2.26 KBalexpott
#2 2503063.2.patch2.79 KBalexpott
#2 2503063.2.will-fail.patch22.73 KBalexpott
#1 2503063.1.patch862 bytesalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2503063.1.patch862 bytes

Well this fixes the problem and ensures that UrlHelper does not having protocols bleed between tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2503063.2.will-fail.patch22.73 KB
2503063.2.patch2.79 KB

I think we should add a PHPUnit side effect test so we catch this type of thing in d.o test runs.

The last submitted patch, 2: 2503063.2.will-fail.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented 
+++ b/core/modules/simpletest/src/Tests/PhpUnitSideEffectTest.php
@@ -0,0 +1,61 @@
+    // Insert the PHPUnit results into the simpletest table and update the
+    // result counter.

Yes but ... why? Well, because we are running tests manually. Should we explain that?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

So now I'm pondering if we shouldn't just remove run-tests.sh's ability to run PHPUnit tests and just put everything into this test. In the long run it's best to encourage people to run phpunit tests using phpunit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented

So now I'm pondering if we shouldn't just remove run-tests.sh's ability to run PHPUnit tests and just put everything into this test. In the long run it's best to encourage people to run phpunit tests using phpunit.

Honestly? I'd just love!

The question is what do we do with the other tests: BrowserTestBase and KernelTestBase in the future. Running them like that could be problematic due to the speed limit?
I guess we really need the new testbot infrastructure

Then there is also the question how do we adapt the existing simpletest UI. Should we just render shell strings and remove the checkbox for PHPUnit tests?

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Status: Needs review » Needs work

Patch no longer applies.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
2503063-2-19.patch2.26 KB

While the failure no longer happens - probably because test ordering has changed in some way I still think that making some sort of fix here it warranted.

The simplest fix is to mark tests which change the static as needing to run in a separate process.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

This came up as a daily Bug smash target, hence the reviving.

Checked and these 4 spots are the only tests that mess with UrlHelper::setAllowedProtocols, so that seems like a good fix.

Tried to reproduce the issue too, but my setup gets stuck at 49% while running all the unit tests ¯\_(ツ)_/¯

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2503063-2-19.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
19-23-interdiff.txt1.74 KB
2503063-2-23.patch3.23 KB

Interesting, we really shouldn't be setting expectations or using the container in a data provider.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at iO commented
Status: Needs review » Reviewed & tested by the community

Nice bit of hardening coming out of this then, looks good and comes back green

  • catch committed 476ad33 on 10.0.x 
    Issue #2503063 by alexpott, Lendude: Removing XssTest causes test...
  • catch committed a47bf76 on 10.1.x
    Issue #2503063 by alexpott, Lendude: Removing XssTest causes test...
  • catch committed dc2df52 on 9.4.x
    Issue #2503063 by alexpott, Lendude: Removing XssTest causes test...
  • catch committed c381d7c on 9.5.x
    Issue #2503063 by alexpott, Lendude: Removing XssTest causes test...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x and cherry-picked back through to 9.4.x, thanks!

Status: Fixed » Closed (fixed)

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