PHPUnit 10 behaves differently when invoked outside web root

Created on 7 May 2024, 2 months ago
Updated 2 July 2024, 17 days ago

Problem/Motivation

When using a drupal/recommended-project setup, composer.json exists outside of the web root where Drupal core lives. In PHPUnit 9 you could run PHPUnit tests from the project root, for example:

$ ./vendor/bin/phpunit -c web/core/phpunit.xml.dist web/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php 
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Access\AccessManagerTest
..................                                                18 / 18 (100%)

With PHPUnit 10 this fails to find the test:

PHPUnit 10.5.20 by Sebastian Bergmann and contributors.

Test file "web/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php" not found

You can omit the web/ directory and it passes, but this means shell autocompletion is broken:

$ ./vendor/bin/phpunit -c web/core/phpunit.xml.dist core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
PHPUnit 10.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.6
Configuration: /home/michael/www/phpunit10/web/core/phpunit.xml.dist

..................                                                18 / 18 (100%)

Or you can cd in to the web/ directory first.

$ cd web
$ ../vendor/bin/phpunit -c core/phpunit.xml.dist core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
PHPUnit 10.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.6
Configuration: /home/michael/www/phpunit10/web/core/phpunit.xml.dist

..................                                                18 / 18 (100%)

Steps to reproduce

Testing this on drupal/core-recommended:11.0.x-dev because 11.x-dev hasn't updated yet.

$ composer create-project drupal/recommended-project:11.0.x-dev@dev .
$ ./vendor/bin/phpunit -c web/core/phpunit.xml.dist web/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
$ ./vendor/bin/phpunit -c web/core/phpunit.xml.dist core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
$ cd web/
$ ../vendor/bin/phpunit -c core/phpunit.xml.dist core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php

Proposed resolution

Move chdir call from core/tests/bootstrap.php to test base classes, right after they set $this->root.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ› Bug report
Status

Fixed

Version

11.0 ๐Ÿ”ฅ

Component
PHPUnitย  โ†’

Last updated about 1 hour ago

Created by

๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mstrelan
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    FWIW if you run tests via PhpStorm you're not affected by this as it use absolute file paths.

  • Pipeline finished with Failed
    2 months ago
    Total: 471s
    #166856
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan
  • Pipeline finished with Canceled
    2 months ago
    Total: 647s
    #166885
  • Pipeline finished with Failed
    2 months ago
    Total: 526s
    #166887
  • Pipeline finished with Success
    2 months ago
    Total: 573s
    #166898
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    I don't understand what has changed here, the chdir() was added a while back in ๐Ÿ› InfoParser returns an empty array if passed a non-existing file Fixed so why is PHPUnit 9 behaviour different?

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    Guessing PHPUnit 10 handles bootstrap differently, maybe it loads the file earlier now.

    FWIW the docs for bootstrap say:

    For common use cases, this script should not do more than register an autoloader so that PHP can find the tested units of code.

    If I get a chance I'll try track down the change in PHPUnit.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexpott ๐Ÿ‡ช๐Ÿ‡บ๐ŸŒ

    Missing build tests - but they might mess with the working directory even more.

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    Re: #5

    In PHPUnit 9 I put a breakpoint in \PHPUnit\TextUI\Command::handleArguments. Stepping throw I can see that $this->arguments['test'] = realpath($arguments->argument()); converts the relative path to absolute much before $this->handleBootstrap() is called in the same method.

    In PHPUnit 10 I put a breakpoint in core/tests/bootstrap.php and in \PHPUnit\TextUI\Configuration\TestSuiteBuilder::build. The bootstrap file is loaded first, invoked from \PHPUnit\TextUI\Application::run, then TestSuiteBuilder::build calls realpath($cliArgument) which fails.

    Re: #7

    Build tests are currently passing and seem to have their own logic for how to find Drupal. They also weren't touched in ๐Ÿ› InfoParser returns an empty array if passed a non-existing file Fixed and don't have an existing $root property the others have. I'd be inclined to leave them out.

  • Status changed to RTBC 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mark_fullmer Tucson

    I was able to reproduce the problem running core/contrib tests with Drupal 11.x and PHPUnit 10.

    I've tested this MR with PHPUnit 10 and can confirm that it resolves the issue with the webroot. Tests execute as expected, and pass. Marking this as RTBC!

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    I was able to install and test the following contrib modules against the patch, using core 11.0.x-dev:

    • Migrate Plus
    • Migrate Tools
    • dynamic_entity_reference
    • svg_image_field
    • Advanced email validation

    All install and test results are available on the MR comments. No more testing to be done from my end.

    • catch โ†’ committed 827dd969 on 11.0.x
      Issue #3445847 by mstrelan: PHPUnit 10 behaves differently when invoked...
    • catch โ†’ committed 1eef08b4 on 11.x
      Issue #3445847 by mstrelan: PHPUnit 10 behaves differently when invoked...
  • Status changed to Fixed about 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    I'm having similar problems just running tests from the core directory with ../vendor/bin/phpunit tests/whatever have to use core/tests/whatever instead which breaks terminal autocomplete. This is working well so I think we should go ahead here. If phpunit hadn't intentionally deprecated so many things we've been relying on in phpunit 10 would have suggested a bit more investigation and an upstream bug report, but not sure what that would get us here.

    Committed/pushed to 11.x and cherry-picked to 11.0.x, thanks!

    • catch โ†’ committed a36d0757 on 11.0.x
      Revert "Issue #3445847 by mstrelan: PHPUnit 10 behaves differently when...
    • catch โ†’ committed ca4e4a81 on 11.x
      Revert "Issue #3445847 by mstrelan: PHPUnit 10 behaves differently when...
  • Status changed to Needs work about 2 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Nope this broke HEAD, might be new tests that need an update. https://git.drupalcode.org/project/drupal/-/pipelines/179231

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    I see three failures.

    1. Core test job is failing on test RecipeQuickStartTest @ docroot/web/core/tests/Drupal/Tests/Core/Recipe/RecipeQuickStartTest.php.

    Curious that locally I see it getting skipped instead of attempted and failed.

    2. Build job is failing with

        Drupal\BuildTests\Command\GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled
        Fake git used by process.
        Failed asserting that 0 matches expected 127.
        
        /builds/project/drupal/core/tests/Drupal/BuildTests/Command/GenerateThemeTest.php:291
    

    3. As for nightwatch it is reporting it is failing to install the database during one of the tests:

    Failed to connect to database. The database engine reports the following message: <em class="placeholder">SQLSTATE[HY000]: General error: 5 database is locked</em>.<ul><li>Does the database file exist?</li><li>Does web server have permission to write to the database file?</li>Does the web server have permission to write to the directory the database file should be created in?
    

    The command is

    โ”‚       Command failed: php ./scripts/test-site.php install  --install-profile "nightwatch_testing"  --base-url http://localhost/subdirectory --db-url sqlite://localhost//builds/project/drupal/sites/default/files/db.sqlite?module=sqlite --json      
    

    Under the [Tests/Toolbar Api Test] Test Suite

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    Running the core unit test with--debugshows PHPUnit decides to bounce on the test while doing the setup

    - Drupal\Tests\Core\Recipe\RecipeQuickStartTest::setUp
    Test Prepared (Drupal\Tests\Core\Recipe\RecipeQuickStartTest::testQuickStartRecipeCommand)
    Test Skipped (Drupal\Tests\Core\Recipe\RecipeQuickStartTest::testQuickStartRecipeCommand)
    After Test Method Called (Drupal\Tests\Core\Recipe\RecipeQuickStartTest::tearDown)
    After Test Method Finished:
    

    I'm none the wiser as to the exact root cause so I'll be debugging it. I am on the same version of PHPUnit reported on the build log:

    PHPUnit Started (PHPUnit 10.5.20 using PHP 8.3.0 (cli) on Linux)
    
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    The nightwatch one is a random fail, SQLite doesn't work perfectly under the sorts of loads we put it under in the test suite, "database is locked" is a symptom of this and the test will likely pass on the next run.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    @longwave, thanks for the context.

    For the SQLite unit test, I am observing that it is path related.

    Changing

        $install_command = [
          $this->php,
          'core/scripts/drupal',
    

    to web/core/scripts/drupal does allow the install step to resume (as opposed to causing it to skip right then and there).

    Once the install proceeds Drupal site is installed but fails in configuration with error Error: Class "SimpleXMLElement" not found in . That last one might or might not be environmentally related. Fun XD

  • Status changed to Needs review about 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    First attempt ever at submitting a patch to core. I don't have a fork to which issue MRs from. Please take it easy on me :)

  • Pipeline finished with Failed
    about 2 months ago
    Total: 317s
    #179593
  • Pipeline finished with Failed
    about 2 months ago
    Total: 287s
    #179595
  • Pipeline finished with Success
    about 2 months ago
    Total: 571s
    #179597
  • Pipeline finished with Success
    about 2 months ago
    Total: 577s
    #179608
  • Pipeline finished with Success
    about 2 months ago
    Total: 512s
    #179648
  • Pipeline finished with Success
    about 2 months ago
    Total: 508s
    #179666
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    @Alexander Allen thanks for investigating this and creating the MR.

    In future, instead of creating a new MR, please rebase the existing MR and commit to that, so we can see what has changed. I've generated an interdiff below for committers to see.

    I'm not sure we need the additional assertion but I can see how it helps. But you're creating a new local variable but then not using it in $install_command. Also don't need {} around $script.

    $ interdiff 7952.diff 8153.diff 
    only in patch2:
    unchanged:
    --- a/core/tests/Drupal/Tests/Core/Recipe/RecipeQuickStartTest.php
    +++ b/core/tests/Drupal/Tests/Core/Recipe/RecipeQuickStartTest.php
    @@ -92,14 +92,17 @@ public function testQuickStartRecipeCommand(): void {
         // Install a site using the standard recipe to ensure the one time login
         // link generation works.
     
    +    $script = $this->root . '/core/scripts/drupal';
         $install_command = [
           $this->php,
    -      'core/scripts/drupal',
    +      $this->root . '/core/scripts/drupal',
           'quick-start',
           'core/recipes/standard',
           "--site-name='Test site {$this->testDb->getDatabasePrefix()}'",
           '--suppress-login',
         ];
    +    $this->assertFileExists($script, "Install script is found in {$script}");
    +
         $process = new Process($install_command, NULL, ['DRUPAL_DEV_SITE_PATH' => $this->testDb->getTestSitePath()]);
         $process->setTimeout(500);
         $process->start();
    @@ -123,7 +126,7 @@ public function testQuickStartRecipeCommand(): void {
     
         // Generate a cookie so we can make a request against the installed site.
         define('DRUPAL_TEST_IN_CHILD_SITE', FALSE);
    -    chmod($this->testDb->getTestSitePath(), 0755);
    +    chmod($this->root . '/' . $this->testDb->getTestSitePath(), 0755);
         $cookieJar = CookieJar::fromArray([
           'SIMPLETEST_USER_AGENT' => drupal_generate_test_ua($this->testDb->getDatabasePrefix()),
         ], '127.0.0.1');
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Alexander Allen Bushwick, Brooklyn

    I noticed the unused variable, and caught it when reviewing the MR. Then I overwrote the fix when changing the MR from 11.0.x to 11.x, didn't notice it that time. Thanks for pointing it out.

    Wasn't sure about pushing to existing MR branch, thanks for the guidance.

  • Pipeline finished with Success
    about 2 months ago
    Total: 584s
    #179812
  • Pipeline finished with Success
    about 2 months ago
    Total: 558s
    #179818
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone New Zealand

    There are two MRs here both on 11.x and with different diff stats. Which one should be reviewed? The issue summary should have details on the difference.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    mstrelan โ†’ changed the visibility of the branch 3445847-phpunit-10-chdir-retry to hidden.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    mstrelan โ†’ changed the visibility of the branch 3445847-phpunit-10-chdir-retry to active.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    mstrelan โ†’ changed the visibility of the branch 3445847-phpunit-10-chdir to hidden.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia mstrelan

    @quietone I've hidden the original branch that was previously committed. The remaining MR is for review. As mentioned in an earlier comment we should have continued on with the one MR, but Alexander Allen was not aware of this. Please review MR 8153.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Removing issue summary tag as there is only 1 MR up now.

  • Pipeline finished with Success
    about 1 month ago
    Total: 654s
    #201303
  • Status changed to RTBC about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    I reviewed the changes between the two MRs; basically, there was an additional test base class added for recipe quickstart things between the original MR and the commit. Everything else is the same.

    Given the obvious necessity and that this was already committed once aside from the above, I'm also comfortable committing it despite RTBCing it myself.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    Saving credits.

    • xjm โ†’ committed b064d944 on 11.x
      Issue #3445847 by Alexander Allen, mstrelan, catch, xjm, longwave,...
    • xjm โ†’ committed 9c045e0c on 11.0.x
      Issue #3445847 by Alexander Allen, mstrelan, catch, xjm, longwave,...
  • Status changed to Fixed about 1 month ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States xjm

    Committed to 11.x and cherry-picked to 11.0.x. Thanks everyone!

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    Noticed that writing test.bbb to the filesystem was causing issues here so created a follow up ๐Ÿ“Œ FileSaveUploadTest should not write to the app root Needs review

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

Production build 0.69.0 2024