Follow-up to #2105583: Add some sane strictness to phpunit tests to catch risky tests

Problem/Motivation

Not sure how this isn't already causing problems for global state change detection but its probably a quirk of how phpunit runs. The composer class loader is set to a global in the phpunit bootstrap.php. Somehow all relevant classes seem to be loaded or no changes happen for what ever reason during the core phpunit test runs. However in [#2105593] klausi identified that in some cases it is possible for contrib to trigger autoloading that modifies the contents of the classloader's internal structures.

Proposed resolution

Don't set the class loader as a global and run the related code within a function.

API changes

Not sure its really an API change but the global loader object will be removed. The existence of the loader global was incidental to how the code was written and not a feature/api and this is testing the change should be acceptable. Additionally, fetching the class loader should use composer's API not this global for correctness and accuracy.

CommentFileSizeAuthor
#10 bootstrap_php_loader-2631478-10.patch2.96 KBneclimdul
#10 bootstrap_php_loader-2631478-10,interdiff.txt865 bytesneclimdul
#7 2631478-7.interdiff.txt874 bytesneclimdul
#7 bootstrap_php_loader-2631478-7.patch2.9 KBneclimdul
#2 bootstrap_php_loader-2631478-2.patch2.9 KBneclimdul
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Active » Needs review
FileSize
bootstrap_php_loader-2631478-2.patch2.9 KB

Patch.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented 
+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -235,8 +235,7 @@ protected function bootEnvironment() {
-    $this->classLoader = $GLOBALS['loader'];
+    $this->classLoader = require $this->root . '/autoload.php';

+++ b/core/tests/bootstrap.php
@@ -87,22 +87,40 @@ function drupal_phpunit_get_extension_namespaces($dirs) {
-$loader = require __DIR__ . '/../../autoload.php';

I agree with this change, but I'm pretty sure the original code was designed that way to make tests faster. Whether it's worth it to break isolation is doutbtful, AFAIC.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Yeah I thought about doing it differently because there are a couple alternatives but I think this is probably ok.

1) We could call the composer method directly but that only works because we hard code the autoloader-suffix which I've been told we want to remove. After that the class name is unpredictable so we can't hard code it.
2) We could store a static in the bootload.php populate method and return the classloader each time its called.
3) We force everything to exclude the loader from global watches.

1 & 2 Are going to be more expensive then accessing a super global because its a function call. I'm not sure its really any different then the require in practice.
3 is what we're trying to fix because it isn't not really a reasonable requirement for tests.

So I settled with the require change to KTB because its just the way composer is designed to be used and the least likely to cause problems at any point in the future.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Yeah practically everything else which happens during a test is way more costly, so let's not worry about that.

+1

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work

I can confirm that this fixes the risky tests in Rules, thanks a lot!

Super minor nitpicks:

  1. +++ b/core/tests/bootstrap.php
@@ -87,22 +87,40 @@ function drupal_phpunit_get_extension_namespaces($dirs) {
+ * We run this in a function to avoid settings the class loader to a global

    ".. to avoid setting the ..." 

  2. +++ b/core/tests/bootstrap.php
@@ -87,22 +87,40 @@ function drupal_phpunit_get_extension_namespaces($dirs) {
+  /** @var  \Composer\Autoload\ClassLoader() $loader */

    one space too much after the @var

    EDIT: and there should be no parenthesis after the class name.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Status: Needs work » Needs review
FileSize
bootstrap_php_loader-2631478-7.patch2.9 KB
2631478-7.interdiff.txt874 bytes

fixed.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

I'm calling this RTBC even though the tests are still running.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -235,8 +235,7 @@ protected function bootEnvironment() {
+    $this->classLoader = require $this->root . '/autoload.php';

This really should be injected, but that's out of scope here.

Yeah practically everything else which happens during a test is way more costly, so let's not worry about that.

The namespace scan happens once per class anyway on the testbot, because each test is a separate process.

Calling it RTBC but the testbot is still free to disagree.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch as a volunteer commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/bootstrap.php
@@ -87,22 +87,40 @@ function drupal_phpunit_get_extension_namespaces($dirs) {
+ * Do initial class loader populate classes.

This isn't a sentence. Not able to think of a quick suggestion.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
FileSize
bootstrap_php_loader-2631478-10,interdiff.txt865 bytes
bootstrap_php_loader-2631478-10.patch2.96 KB

Wow... to think that made sense in my head at one point.

Touched up the following statement to clarify where the false positive is as well.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Reviewed & tested by the community

Wow... to think that made sense in my head at one point.

Yeah that happens to me ALL the time.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed ea3f8b0 and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 87cded8 on 8.1.x 
    Issue #2631478 by neclimdul: bootstrap.php loader can show as changed...

  • alexpott committed ea3f8b0 on 
    Issue #2631478 by neclimdul: bootstrap.php loader can show as changed...

Status: Fixed » Closed (fixed)

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