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

CommentFileSizeAuthor
#22 issue3445847-reroll.patch5.91 KBalexander allen

Issue fork drupal-3445847

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

mstrelan commented

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

mstrelan opened merge request !7952

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

mstrelan’s picture

mstrelan commented

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue tags: +PHPUnit 10
mstrelan’s picture

mstrelan commented

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 #3391776: InfoParser returns an empty array if passed a non-existing file and don't have an existing $root property the others have. I'd be inclined to leave them out.

mark_fullmer’s picture

mark_fullmer
he/him
Primary language English
Location Tucson
commented
Status: Needs review » Reviewed & tested by the community

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!

alexander allen’s picture

alexander allen commented

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...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.0.x-dev
Status: Reviewed & tested by the community » Fixed

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...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

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

alexander allen’s picture

alexander allen commented

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

alexander allen’s picture

alexander allen commented

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)
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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.

alexander allen’s picture

alexander allen commented

@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).

Edit: I was missing simplexml, fixed that. Now I see failure in line chmod($this->testDb->getTestSitePath(), 0755);, which makes sense because it is path related.

Wish me luck figuring out a patch, if @mstrelan doesn't beat me to it (I'm sure he will haha)

alexander allen’s picture

alexander allen commented
Status: Needs work » Needs review
StatusFileSize
new issue3445847-reroll.patch5.91 KB
  • First attempt ever at submitting a patch to core.
  • I don't have a fork to which issue MRs from. Fixed, tests running.
  • Please take it easy on me :)

Alexander Allen opened merge request !8153

mstrelan’s picture

mstrelan commented

@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');
alexander allen’s picture

alexander allen commented

I've fixed the unused variable on on MR 8153 and pushed the change.

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

quietone’s picture

quietone commented
Issue tags: +Needs issue summary update

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.

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

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

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

mstrelan’s picture

mstrelan commented

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

smustgrave’s picture

smustgrave commented
Issue tags: -Needs issue summary update

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

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

xjm
she/her
Primary language English
commented

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,...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Related issues: +#3455183: FileSaveUploadTest should not write to the app root

Noticed that writing test.bbb to the filesystem was causing issues here so created a follow up #3455183: FileSaveUploadTest should not write to the app root

xjm closed merge request !7952

xjm closed merge request !8153

Status: Fixed » Closed (fixed)

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