Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency

Removing the bridge is not as horrible as I thought; however, at the moment there are still too many prep issues from 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active that would need to be solved before we can see what to do to try and reimplement deprecation capturing.

With last commit in the MR we can see how PHPUnit itself will report calls to deprecated paths natively, i.e. without the bridge:

PHPUnit 10.5.9 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.2.15
    Configuration: /builds/issue/drupal-3417066/core/phpunit.xml.dist
    
    D...EE...D.D                                                      12 / 12
    (100%)
    
    Time: 00:03.231, Memory: 12.00 MB
    
    There were 2 errors:
    
    1)
    Drupal\KernelTests\Core\Database\ConnectionTest::testPerTablePrefixOption
    Error: Call to undefined method
    Drupal\KernelTests\Core\Database\ConnectionTest::expectError()
    
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:128
    
    2) Drupal\KernelTests\Core\Database\ConnectionTest::testPrefixArrayOption
    Error: Call to undefined method
    Drupal\KernelTests\Core\Database\ConnectionTest::expectError()
    
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:142
    
    --
    
    3 tests triggered 3 deprecations:
    
    1)
    /builds/issue/drupal-3417066/vendor/symfony/deprecation-contracts/function.php:25
    Since symfony/dependency-injection 6.4:
    "Symfony\Component\DependencyInjection\ContainerAwareTrait" is deprecated,
    use dependency injection instead.
    
    Triggered by:
    
    * Drupal\KernelTests\Core\Database\ConnectionTest::testConnectionRouting
     
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:18
    
    2)
    /builds/issue/drupal-3417066/core/lib/Drupal/Core/Database/Connection.php:506
    Drupal\Core\Database\Connection::getUnprefixedTablesMap() is deprecated in
    drupal:10.0.0 and is removed from drupal:11.0.0. There is no replacement.
    See https://www.drupal.org/node/3257198
    
    Triggered by:
    
    *
    Drupal\KernelTests\Core\Database\ConnectionTest::testDeprecatedGetUnprefixedTablesMap
     
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:180
    
    3)
    /builds/issue/drupal-3417066/core/lib/Drupal/Core/Database/Connection.php:489
    Drupal\Core\Database\Connection::tablePrefix() is deprecated in
    drupal:10.1.0 and is removed from drupal:11.0.0. Instead, you should just
    use Connection::getPrefix(). See https://www.drupal.org/node/3260849
    
    Triggered by:
    
    *
    Drupal\KernelTests\Core\Database\ConnectionTest::testDeprecatedTablePrefix
     
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:197
    
    ERRORS!
    Tests: 12, Assertions: 20, Errors: 2, Deprecations: 3.

Let's see if replacing @group legacy annotation with the #[IgnoreDeprecations] attribute we can replace the legacy legacy (pun intended)

Yes, that works. Adding the #[IgnoreDeprecations] attribute to the two test methods testDeprecatedGetUnprefixedTablesMap() and testDeprecatedTablePrefix(), now they are no longer reported.

However, it looks like PHPStan-PHPUnit is reporting calls to deprecated methods in such marked tests; opened https://github.com/phpstan/phpstan-phpunit/issues/201 upstream to fix that. For now, I'm keeping both the @group legacy annotation and the new attribute.

PHPUnit 10.5.9 by Sebastian Bergmann and contributors.
    
    Runtime:       PHP 8.2.15
    Configuration: /builds/issue/drupal-3417066/core/phpunit.xml.dist
    
    D...EE......                                                      12 / 12
    (100%)
    
    Time: 00:03.911, Memory: 12.00 MB
    
    There were 2 errors:
    
    1)
    Drupal\KernelTests\Core\Database\ConnectionTest::testPerTablePrefixOption
    Error: Call to undefined method
    Drupal\KernelTests\Core\Database\ConnectionTest::expectError()
    
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:129
    
    2) Drupal\KernelTests\Core\Database\ConnectionTest::testPrefixArrayOption
    Error: Call to undefined method
    Drupal\KernelTests\Core\Database\ConnectionTest::expectError()
    
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:143
    
    --
    
    1 test triggered 1 deprecation:
    
    1)
    /builds/issue/drupal-3417066/vendor/symfony/deprecation-contracts/function.php:25
    Since symfony/dependency-injection 6.4:
    "Symfony\Component\DependencyInjection\ContainerAwareTrait" is deprecated,
    use dependency injection instead.
    
    Triggered by:
    
    * Drupal\KernelTests\Core\Database\ConnectionTest::testConnectionRouting
     
    /builds/issue/drupal-3417066/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php:19
    
    ERRORS!
    Tests: 12, Assertions: 20, Errors: 2, Deprecations: 1.

So far:

WE CAN

1) setup PHPUnit to fail and report calls to deprecated paths in normal tests;
2) likewise setup tests to ignore all deprecations to run tests with deprecated code (like today with @group legacy;

WE CAN NOT

3) check a specific deprecation was thrown (à la expectDeprecation());
4) selectively ignore deprecations (à la .deprecation-ignore.txt file of patterns).

https://github.com/sebastianbergmann/phpunit/pull/5605 looks like it might help with #8.3.

The above mention PR is merged for PHPUnit 11.0, which scared the heck out of me because it sounded far, far away.
Then I saw the planned release date: PHPUnit 11 is planned for February 2, 2024 (https://github.com/sebastianbergmann/phpunit/milestone/46)

Yeah, seeing that milestone earlier this week spurred me into action, because the further we fall behind the harder it will be to catch up. Maybe we will just have to skip PHPUnit 10 and go straight to PHPUnit 11 in Drupal 11.

for the record - interesting alternative by PHP-CS-Fixer, https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7578

The PHPUnit Unit job of the pipeline fails after a 1 hr timeout, https://git.drupalcode.org/issue/drupal-3417066/-/jobs/700189

There's some tests that get into infinite loops.

I will add a chunk from the MR in the parent meta 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active that changes PhpUnitTestRunner to use Symfony Process instead of exec() to run the PHPUnit executable. That allows to set a timeout and fail the subprocess - so to find out the culprit and avoid hours of idle bot.

#9 yes that will go in the direction of #8.3 but it gets things even more complicated:

1. If that's for PHPUnit 11 only, then we go back to the problem of getting all dependencies supporting PHPUnit 11. Prophecy took 10 months to release a PHPUnit 10 compatibile version.
2. We're already behind on prep issues for PHPUnit 10, jumping to PHPUnit 11 straight means we add even more issues (e.g. static data providers are removed, not just deprecated) to solve before being able to bump.
3. It does not solve the need to be able to get a replacement for expectError() and expectWarning()... probably it makes trying that even harder
4. it does not solve #8.4, again making it probably even harder

I start to think our best course of action would be to see if we can bypass PHPUnit's error handler in each and every test, and replace with one that would integrate expectations for triggered E_* errors (for point 3 here), manage a list of accepted deprecations (for #8.4), and dispatch deprecations events only when necessary.

Posting here something built on the approach of #12.

Now Drupal\Tests\Component\Diff\Engine\DiffOpTest::testReverse passes, i.e. expectError works. Also Drupal\KernelTests\Core\Database\ConnectionTest::testPerTablePrefixOption passes, after changing expectError() with expectException(\AssertionError::class). While that makes the test more accurate, overall is not good though, since ContainerAwareTrait deprecation should still make the test fail (in this context, when deprecation ignores are not supported), but it's not.

Try adding --stop-on-deprecation to PHPUnit's command line options.

Addressed #8.4.

The idea is to have an error handler shim on top of PHPUnit's one. Each and every Drupal test replaces PHPUnit's error handler with the shim at setUp(), and restores it at tearDown(). The shim 'captures' any logic that we do want to override, and falls back to PHPUnit's handler otherwise.

Currently:

WE HAVE

1) setup PHPUnit to fail and report calls to deprecated paths in normal tests;
2) setup tests to ignore all deprecations via #[IgnoreDeprecations]to run tests with deprecated code;
3) a solution to fill-gap removed expectError*(), expectWarning*() methods that are removed in PHPUnit 10; implemented expectError();
4) a solution to selectively ignore deprecations based on the .deprecation-ignore.txt file of patterns.

TODO

1) replacement for expectDeprecation() to check specific deprecations were thrown;
2) allow BC @group legacyannotation to ignore all deprecations when #[IgnoreDeprecations] is not yet applied;
3) implement fill-gaps for remaining expectError*(), expectWarning*() methods that are removed in PHPUnit 10;

Implemented #17.6. However, this likely requires traversing the inheritance hierarchy to pick inherithed docblocks.

Implemented #17.5.

This is mostly done actually. Surely we miss #17.7 and a lot of tuning, but for now my purpose was to see if we can do without the bridge, and I think we can say we can so.

There's a lot of prep issues to do to clean up the path to get this in, see the parent meta. So while we wait, and if Symfony upstream does not come up with a more general solution, I think we can park this for when the time comes.

Thank you @mondrake this looks great so far. It looks like Symfony might abandon the bridge themselves so if they are do we are well prepared, and even if they port some functionality there is likely to be more here that we still need to use.

Filed 📌 Use Symfony Process to spawn subprocesses in PhpUnitTestRunner Needs review and 📌 [meta] Replace calls to ::expectError*() and ::expectWarning*() Active to anticipate some fixes if possible and unload them from here.

Added reference from 🌱 [meta] Add compatibility for the latest major and minor versions of dependencies to Drupal 10 Active since this is one of a few dependencies that are a major version behind.

Since we can be pretty sure the bridge won't be ported to phpunit 10, even if it eventually gets ported direct to 11, this can probably be unpostponed?

Even if there will be a future symfony/phpunit-bridge version working with PHPUnit 11, the deprecation reporting features are probably gone forever, https://github.com/symfony/symfony/issues/49069#issuecomment-1969518232.

It seems to me in Drupal there are no other purposes for the bridge than deprecations, so probably dropping it and filling the gap ourselves until all of Drupal deprecation tracking needs are met natively in PHPUnit is the only option ATM. The MR here does that.

1. PHPUnit 11 could cover most of the deprecation needs - capture and report deprecations, skip deprecations in tests marked appropriately, expect deprecations. The only thing apparently missing is support for the 'ignore file'. However, PHPUnit 11 is going to require quite some additional work on top of the already quite large effort for PHPUnit 10.

2. The biggest headache at the moment on the MR is getting it to work with DebugClassloader for FunctionalTests, its interaction with the middleware is mindboggling to me

Other than that, yes reviews will be appreciated on the MR now; just mind the test failure will most likely be fixed not here but in other issues of the PHPUnit 10 meta.

The biggest headache for me here has gone - the error handlers we are adding at bootstrap and at each test execution were uncontrolledly reverted by the TestKernel in Functional tests. Now there’s a check that restores the handler only when another one was actually set.

Unit tests don’t go green at the moment only because of the expectError*/expectWarning* methods removal, and for the deprecations related to the remaining non-static data providers methods for tests. Kernel and Functional have some othe failures.

Build tests and Nightwatch tests are green!

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

/rebase

GitLabBot found a few errors.

Extracted the TestSuites removal into 📌 Remove deprecated PHPUnit test suites Active

I think the clue to at least some of the kernel test failures is:

Drupal\KernelTests\KernelTestBaseTest::testSetUpDoesNotLeak
Failed asserting that an array does not have the key 'destroy-me'.

A global set in a previous test now leaks through to the subsequent test. This appears to be the problem with ArgumentDateTimeTest as well because when run individually with --filter the test cases work on their own, it's only when the entire class is run that things fail.

Adding @runTestsInSeparateProcesses fixes a lot of these, so I sprinkled that in a number of places and the problems have gone away, adding a @todo to look at why we need this in the first place. There's still some other fails I don't understand but this clears out quite a number of them.

Adding @runTestsInSeparateProcesses fixes a lot of these

This plus a follow-up might be enough to unblock the update.

Ahh KTB and BTB previously disabled all this by default:

  /**
   * {@inheritdoc}
   *
   * Browser tests are run in separate processes to prevent collisions between
   * code that may be loaded by tests.
   */
  protected $runTestInSeparateProcess = TRUE;

  /**
   * {@inheritdoc}
   */
  protected $preserveGlobalState = FALSE;

These properties are gone in PHPUnit 10, you have to explicitly annotate tests.

#38 great catch, so that’s explaining a lot of what I was not understanding…

I can't find a way of enabling these by default from the base class any more. As with everything else in PHPUnit 10 it goes to great lengths to prevent anything about test setup from being overridden.

There are public methods on the test case:

    /**
     * @internal This method is not covered by the backward compatibility promise for PHPUnit
     */
    final public function setRunTestInSeparateProcess(bool $runTestInSeparateProcess): void

But it's too late to run this from setUp(), the test has already started by that point!

Perhaps we just have to annotate all individual tests that need this, it looks like it's not that many, and write a change record explaining this to anyone else who runs into the problem.

There is a new set of attributes that can be used to indicate process isolation at different levels - https://docs.phpunit.de/en/10.5/attributes.html#test-isolation

we should probably look at these instead of the annotations.

Not sure whether setting attributes to e.g a base BTB or KTB class would mean tehy extend to child classes - if they do then we could replicate the concept set by the properties currently.

or maybe we need to set the processIsolation attribute in the phunit.xml file.

That would prevent the need to add the annotation/attribute on the test code of all test files.

But, it makes ALL test run in isolation 😕, including unit ones. Maybe the attribute could be set on test suite level in the file, but run-tests.sh does not use test suites defined there.

Unfortunately the alternative is, I am afraid, to add attribute/annotation to ALL test files: true we have to fix just a few here, but again in run-tests.sh each test file is run in its own PHPUnit process because it’s the script itself spawning the subprocesses, while if we run a group or a suite from PHPUnit CLI, with KTB and BTB it’s one PHPUnit process, and if not specified to run in isolation we’ll be doomed. Practically all custom/contrib tests will be broken.

Another alternative (?): one phpunit.xml for unit tests - w NO process isolation, another one for all other test types - w process isolation.

We could try upstream to see if they would merge annotations/attributes from parent classes, but the answer is probably no, as I imagine it can get quite complicated when overriding parent attributes in child tests

or else, would it possible to call the method in #40 from a method marked with #[BeforeClass]?

(sorry I’m afk at the moment, just throwing ideas I cannot verify)

we will definitely need to test manually running tests groups with PHPUnit CLI

process isolation can also be specified on the PHPUnit CLI command line, https://docs.phpunit.de/en/10.5/textui.html#isolation

IMHO this is the best solution given our run-test.sh script and the current setup where all KTB, BTB and WTB tests are run in isolation. We can tweak our internal runner to be aware of the suite a specific test file is part of, then pass --process-isolation to the spawned PHPUnit call ONLY if the test is a Unit one.

Then:
1) the attribute would only have to be used for Unit tests that need to run in process isolation too
2) we document that running in process isolation with PHPUnit CLI requires changes to the command line

Process isolation isn't the problem with some of the remaining tests. While trying to debug SecurityAdvisoryTest I found some really strange things going on.

At the cron run at the end of the installer, when security advisories are enabled (usually in tests they are disabled), an HTTP request is made to updates.drupal.org. Because we are in a test, the Guzzle middleware calls drupal_generate_test_ua(). This generates a warning, because at that point the sites directory is not writeable so the .htkey file is not written.

In PHPUnit 9, this warning is converted to an exception by PHPUnit, which is then caught in the Guzzle middleware and I think the request is ignored, and the static $key is not set. In PHPUnit 10, the warning is not trapped, so the key is not written to disk, but it is stored in the static variable.

On the subsequent page load, in PHPUnit 9, the key is correctly generated and stored because the sites directory is now writeable. In PHPUnit 10, because the static is set but the .htkey file doesn't exist, the user agent checker can't match it and throws an early 403.

Not quite sure what the fix is, but using warnings converted to exceptions by PHPUnit and then caught by Guzzle to alter code flow is surely not the right way of doing things; hopefully any fix is backportable so we can make all this work more simply in both versions.

Remaining failures:

Unit tests:

• 📌 Deprecate trigger_error in FileStorage::createDirectory() to allow replacing calls to :::expectWarning*() in Drupal\Tests\Component\PhpStorage\FileStorageTest RTBC

Kernel tests:

• 📌 Replace catch of PHPUnit\Framework\Error\Warning in MarkupInterfaceComparatorTest Needs review
• 📌 Stop triggering a warning in MySQL's TransactionManager rollback RTBC

Functional tests:

• SuperUserAccessInstallTest calls getProvidedData() which is gone in PHPUnit 10, needs issue
• SecurityAdvisoryTest fails, needs debugging

FunctionalJavascript tests:

• StandardPerformanceTest has additional queries to key_value_expire for some reason, needs debugging

Unsure if it's worth backporting the @runInSeparateTests bits to make this MR smaller and keep the branches in sync, although one day soon we will want to convert all these annotations to attributes.

At the cron run at the end of the installer, when security advisories are enabled (usually in tests they are disabled), an HTTP request is made to updates.drupal.org.

We should skip that test and open an issue to refactor it, tests aren't supposed to call back to Drupal.org at all, I thought we'd fixed all of them, but apparently we hadn't. We could do something like change the update module endpoint to hit a fixture file or something instead. This doesn't necessarily help the test failure at all, but might as well tackle on its own.

It is only in the test that explicitly covers the advisories feature. And I will have to trace it again to be sure but I think the request didn't actually happen in PHPUnit 9 - but only because file_put_contents triggers a warning which is converted to an exception that's caught inside Guzzle.

Sounds like that is related to what I was fighting with in #3383487-14: Add CronSubscriberInterface so that services can execute cron tasks directly → as well?

Let’s see what happens if we add --process-isolation to ALL test runs. Then if that’s OK we could revert the annotations and find a way to pass to the runner the request to add the flag or not depending on the test class being tested.

TestDiscovery::getPhpunitTestSuite can help determine that.

The Unit test failures now are good IMHO, because they are related to Unit tests suddenly running in isolation. The removal of the annotation from LayoutTest shows also that now Kernel test run in isolation regardless of the annotation.

Next would be to conditionally run in isolation or not depending on the test suite - so to run in isolation all tests except the Unit ones.

I have an idea how to do that, but afk at the moment. If nobody beats me at that, I will be able to try that sometime in the next days.

Still wondering if we need to run all (non-unit) tests in isolation. If PHPUnit don't want us to do this by default, should we try to override it? Is this really going to be a big problem for contrib? If they see failures they can try adding @runTestsInSeparateProcess?

In the meantime I opened 🐛 TestHttpClientMiddleware should prevent requests to other hosts Needs work which fixes SecurityAdvisoryTest on both branches by explicitly throwing an exception when the advisories code tries to contact updates.drupal.org, instead of relying on the warning-to-exception side effect in PHPUnit 9.

@mondrake re "conditionally run in isolation or not depending on the test suite" - this will work in GitLab CI but what about users running tests locally? It'll be confusing to run a functional test on the CLI, that works in GitLab CI, and have it fail with a strange error because it requires isolation. But we also don't want to enable isolation for all tests because, as shown, some unit tests fail when that is enabled. Unless you have a plan to be able to do this from outside of PhpUnitTestRunner?

Other option is 🐛 Tests should not do requests to updates.drupal.org Needs work

@andypost I had not seen that, but that predates the security advisory feature which is the problem here, although perhaps there are other problems too that are hidden by this.

#55 I think exploring running tests not in isolation should be a follow up, if possible. There’s already so much change for contrib/custom, if we can avoid a change in behavior here I think we should try.

#56 I do not know what the future will look like for process isolation, but to me all signals are in the direction this could become phpunit’s default (static dataproviders are in this sense).

In Drupal, today the exception is Unit tests - all other tests run in isolation and there’s a good sense to it IMHO considering all the setup done in Kernel and Functional tests.

So my suggestion would be to tell everyone contrib/custom to run test from the CLI with —process-isolation, and fix any unit test failures. It’s a pity there’s no toggle in the attributes to force no isolation at test or class level even if set on the command line or the config xml; maybe we could ask that upstream but I doubt it will be supported, and in any case it would likely not be available before PHPUnit 12, so we need to find the best trade off possible here.

working on some cleanup + docs

Upgrade PHPUnit to version 10 and consider dropping the Symfony PHPUnit-bridge dependency due to compatibility issues and potential future changes in Symfony's support for PHPUnit. As the current tech gap in our testing suite, especially with PHPUnit 11 released, necessitates action.

composer require --dev phpunit/phpunit ^10
composer remove --dev symfony/phpunit-bridge

@Pravesh_Poonia your comment just appears to be an automated summary of the issue, I am not sure what you are trying to say?

Debugged the StandardPerformanceTest changes with @catch, where there is suddenly a new query to key_value_expire. The root cause is also 🐛 TestHttpClientMiddleware should prevent requests to other hosts Needs work .

In PHPUnit 9, the announcement feed fetcher runs during cron at the end of the installer. TestHttpClientMiddleware tries to write .htkey to the sites directory (which is not writeable), file_put_contents() throws a warning and PHPUnit converts this to an exception, which means the HTTP request is considered failed and so the announcement feed is never written to the expirable key-value store. Later, the form cache tries to read from the expirable key-value store, which doesn't exist, so the query is not logged by PerformanceDataCollector.

In PHPUnit 10, file_put_contents() still fails but the warning is ignored and execution continues. The HTTP request succeeds, the announcements are read and put into the key-value store. Later, the form cache tries to read from the table which now exists, so the query is logged.

We only need two more issues down to get this green:

🐛 TestHttpClientMiddleware should prevent requests to other hosts Needs work

and

📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC

@mondrake is 🐛 TestHttpClientMiddleware should prevent requests to other hosts Needs work strictly necessary for PHPUnit 10? I feel like it's a whole can of worms that I would prefer not to open just right now - we should still fix it but after the upgrade if possible.

@longwave re #67 I thought that one would be necessary to solve the last remaining failure in the test jobs?

I just had to figure out how to fix SecurityAdvisoryTest given that the external HTTP request at the end of the installer now succeeds where it (accidentally) did not in PHPUnit 9; we can figure out how to fix this properly later.

Cool! First time full green test run!

We only need to sort out what to do with 📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC then - we may even just postpone that to after this, if we accept to temporarily lose the coverage: it’s only checking that component tests do not extend from any of Drupal core test base classes, which in the total economy of this journey it’s very minor.

I think we could remove the test coverage for 📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC and then keep that issue to add the phpstan rule. The test isn't even testing functionality, just the organisation of our test coverage, so can live without it for a while.

Agree with @catch that we can defer that until later, that feature is not critical and it is better if we can land this sooner instead.

There are still some @todo docblocks that should be written here, also some old references to the Symfony bridge that need cleaning up;

core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:                      // \Symfony\Bridge\PhpUnit\DeprecationErrorHandler::collectDeprecations().

core/tests/Drupal/FunctionalTests/Core/Test/PhpUnitBridgeTest.php: * Tests Drupal's integration with Symfony PHPUnit Bridge.

On #72.

I think 📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC is now as good as we can for now, the new rule is active, and we can just defer having proper testing of the rule itself - which is even better than #70 because we do not lose coverage of what we are removing.

Addressed #72.

Drupal\Tests\Core\Test\PhpUnitBridgeRequiresTest is probably borked now, as PHPUnit 10 only consider @require extension relevant to PHP extensions, not Drupal's concept of extension. Shall we remove it?

Yeah I think PhpUnitBridgeRequiresTest can go, if we break it again somehow we can add another test then but it seems kinda pointless to keep around.

A couple more comments to review/clean up:

core/tests/Drupal/Tests/Core/Test/PhpUnitBridgeTest.php
33:   * Tests the @requires annotation in conjunction with DrupalListener.
39:   * @see \Drupal\Tests\Listeners\DrupalListener

core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php
128:   * URLs to has been created by \Drupal\Tests\Listeners\HtmlOutputPrinter.

Addressed #76: deleted PhpUnitBridgeRequiresTest, deleted the test in PhpUnitBridgeTest (which is a sister test of PhpUnitBridgeRequiresTest), deleted the comment in BrowserHtmlDebugTrait as it just duplicates the method comment anyway.

Updated IS with the proposed solution. I think this is ready for review.

Left a review, mostly nit-picks on grammar with suggestions and one question about what I _think_ is some lost functionality

Applied suggestions and replied to inline comments.

This looks good to me, thanks for pointing me at 📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC - mammoth effort here, well done.

I think we need to add to core/phpunit.xml.dist also paths to themes and profiles tests in the <testsuites> section.

Tweaked the test suites: added unit, kernel, functional and JS test for profiles and themes, and as far as I know build tests only make sense for core so removed module paths for those.

Couple of glitches I think

I think the process isolation changes are problematic. run-tests should not be deciding this. Can't we add the attribute on the base classes?

@alexpott it appears not, the properties for that are gone and the only way appears to be attributes/annotations, which don't support inheritance.

The problem with our approach of doing it in run tests is that when people run tests from PHPStorm they are going to be using PHPUnit directly. So it you run a class with multiple methods from PHPStorm and it is a kernel test you are going to get possible side effects.

ATM I really don't see any other way apart from adding the attribute or annotation to every test. I guess the annotation would be make it easier on the upgrade path... ie. to be Drupal 10 and 11 compatible. But at some point PHPUnit will drop annotation support...

Maybe we can control this from the constructor, although the base constructor is tagged @internal.

#89 is going to work. Obviously I think it needs a big old comment as to why we're doing this but it is pragmatic for today and then we can consider opening an upstream issue and asking for something to help us not to do this.

So this worked better than I expected. Deprecations are only caught when a test ends up loading a different type of test (a unit test loads a kernel test class, etc) so if we can clean those up a bit this might be a viable solution.

Not sure I'm quite doing the right thing here but it seems to work locally. I don't think the <source> element was configured properly before:

https://docs.phpunit.de/en/10.5/error-handling.html#limiting-issues-to-y...

and the restrictDeprecations="true" makes the @internal warning go away, though I'm not sure what other side effects this might have:

https://docs.phpunit.de/en/10.5/configuration.html#the-restrictdeprecati...

Perhaps we are skipping other deprecations (e.g. in Symfony?) by doing this.

Actually yeah I think restrictDeprecations="true" is incorrect; the deprecation is triggered by vendor/symfony/error-handler/DebugClassLoader.php so we are ignoring all DebugClassLoader deprecations now, which is not what we want.

Isn’t the last change killing all DebugClassloader generated deprecations? That would be a pity. Maybe we just add the deprecation to the ignore file?

Great catch #89

@mondrake I tried adding the deprecation to the ignore file but it didn't get ignored, will try again tomorrow.

Added deprecation ignore

Thanks @mondrake - I see the error I made last night now, I forgot to escape the () in the deprecation.

Unfortunately, though, this broke the HTML output logger. Looks like the environment variables are no longer visibile in PHPUnit isolated subprocess and therefore the logger does not init. If I remove the BrowserTestBase constructor the failing test passes, but that means that we are not running in isolation. I'm afraid this path is taking us too much into PHPUnit internals, and that makes me shiver. I think the point here is that ALL tests could potentially run in isolation, but in run-test.sh we're omitting process isolation only for Unit ones for a matter of performance, the Unit test run job will last 15 minutes instead of 1. But in local dev, I see no reason not to run everything in isolation. Don't IDEs have a process isolation setting themselves when it comes to run tests https://www.jetbrains.com/help/phpstorm/run-debug-configuration-phpunit....? (not a user, sory)

I confirmed with xdebug that the environment variables are passed into the child process, and the HtmlOutputLogger appears to work, except that HtmlOutputLogger::testRunnerFinished() appears to not be called inside the isolated process, and so the verbose message is not printed. Will continue debugging...

How about writing a simple Rector rule that will add #[RunTestsInIsolation] to all classes extending KTB and BTB? Then we will be done, no hacking of internals, no fiddling with command line arguments. I've already started playing with a similar thing in 📌 Replace @dataProvider annotations with #[DataProvider()] attributes Postponed .

Only thing: since the result of such a rule would touch thousands of files, I would do it separately from here - still, I'd do this first so we can add attributes and not annotations (that are throwing PHPUnit deprecations in 11, and are removed in 12)

Filed 📌 Add #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests Needs review to explore #100

The issue is that the events fire in the parent process, and the logging happens in the child process. When process isolation is used the static $links never gets populated in the parent, and so the output is never printed. We need some way of communicating back from the child that output was printed - maybe we can use the counter file for this instead of a static variable?

Yeah… wonder why the test was passing before the change then…

Yeah… wonder why the test was passing before the change then…

Maybe the isolated process stuff wasn't quite working...

Given 📌 Add #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests Needs review is almost green, we might do that first (it touches 3200+ files!), and then come back here. That one takes away the need of both the CLI flag, AND of the hacking of process isolation in the constructor.

I still don't understand how that one acts differently from the constructor change here, not how they then again act differently when run-tests.sh sets the process isolation flag.

I have an hypotesis i.e. that the subprocess run by PHPUnit shares static info with the parent. This is different from run-tests.sh, whose process isolation is at OS level i.e. separate *nix process. That would explain why i.e. deprecation bridge works for Kernel tests. In that case, since the HTML logger is a singleton, the subprocess would accumulate links in the single instance even if opened by the parent. Somehow this works though only if the process isolation is set before (or separately) the TestCase construction. We would likely not have a public setRunTestInSeparateProcess() method if it was possible to do stuff just in the constructor, there is likely some other config that we are missing from the case of the annotated/attributed method.

Will revert a bit of the last changes and try to see if the logger works.

So my hypothesis in #107 is wrong. The HTML logger works on the subprocess but is not reported by the main process, per #102. Still a mystery for me why the test was passing earlier.

Per #102, I think it's right we should use a file instead of the static variable, which BTW was the original implementation from the removed listener. I had to go back a bit to find out. On that.

Now FunctionalTestDebugHtmlOutputTest passes, so hopefully we solved the problem. At this stage, I would suggest to do 📌 Add #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests Needs review first, then come back to this and hopefully see all the process isolation issue solved purely with test attributes.

Thanks! I realise what was happening before now:

When run-tests.sh was configuring process isolation, FunctionalTestDebugHtmlOutputTest runs FunctionalTestDebugHtmlOutputHelperTest separately - without isolation - so the trait works and it passes.

When isolation was configured in the constructor, it affects all tests, including FunctionalTestDebugHtmlOutputHelperTest, so the trait stopped working.

I am not sure we should add the attribute to all tests, because it means remembering to add it each time (and probably writing a test or PHPStan rule to enforce it) in core, contrib and custom tests. Surely it's better if we just configure this automatically, just like we did in PHPUnit 9 and below?

I am scared about doing that in the extended constructor - being marked @internal by PHPUnit I can see troubles in the future. But if that’s the way to get through, fine for now I suppose.

I could also argue that adding the attribute now to all tests would be a sort of a baseline vs D9 where all tests were run in isolation. Doing that and leaving additions to be thoughtfully marked with the attribute (or not), would drive focused attention by developers. They would have options: add the attribute to the test, or add the command line option when running PHPUnit, or even add the configuration in the XML file if they have their own. I am not sure we need to baby sit here.

Great find in #112 BTW!

@mondrake we do need to enforce this for all kernel and functional tests. Drupal makes use of too many statics. And it is impossible for the test author to judge how their kernel or functional test will impact others run at the same time.

Plus with the attribute - this is not practical for contrib that wants to be D10 and D11 compatible - as the attribute does not exist in PHPUnit 9. We could recommend that the annotation is used for contrib that wants to be D10 compatible - but that feels like another complexity to deal with.

I think the constructor changes are fine. And I think we should open an upstream issue to allow process isolation to be set by an API for different test types. Isolation is a hard requirement for our kernel and functional tests.

Isolation is a hard requirement for our kernel and functional tests.

How about adding a PHPStan rule checking the attribute exists on those tests?

@mondrake that's what we would have to do - but in my opinion we should open something up against PHPUNit describing the situation and the why. I think this is a reasonable request. I think the constructor fix is okay - I'd even use reflection to fix this. FWIW this fix still works in PHPUnit 11 see https://github.com/sebastianbergmann/phpunit/blob/main/src/Framework/Tes... and https://github.com/sebastianbergmann/phpunit/blob/main/src/Framework/Tes...

Also the attribute is problematic - we should look for both the annotation or the attribute because PHPUnit 9 and 10 support - also we'd need to but the rule in mglaman's thing because it is not a core rule - it is a Drupal rule.

Started a section in the IS with a list of things to request upstream

Re-added setRunTestInSeparateProcess(TRUE) in base classes.

We can remove the attributes added for testing again here, but after that I think this is good to go.

Also it looks like we can't mix PHPUnit attributes and annotations, we will have to convert them all at once unfortunately:

        $metadata = $this->attributeReader->forMethod($className, $methodName);

        if (!$metadata->isEmpty()) {
            return $metadata;
        }

        return $this->annotationReader->forMethod($className, $methodName);

Addressed #120, as it's only undoing attributes added while we were working on the process isolation fixes I think this is OK - my RTBC covers @mondrake's changes since @larowlan previously marked this RTBC.

I wonder what we need to tell people to do to their phpunit.xml files? We're changing core/phpunit.xml.dist but many people will have a customised version - for projects or for core dev.

The good news is the with my current phpunit.xml for core tests run and I get a helpful message saying

./vendor/bin/phpunit  core/tests/Drupal/KernelTests/Core/Recipe/RollbackTest.php                                       Tue  7 May 11:21:55 2024
PHPUnit 10.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.6
Configuration: /Volumes/dev/sites/drupal8alt.dev/phpunit.xml

...                                                                 3 / 3 (100%)

Time: 00:12.809, Memory: 10.00 MB

There was 1 PHPUnit test runner deprecation:

1) Your XML configuration validates against a deprecated schema. Migrate your XML configuration using "--migrate-configuration"!

OK, but there were issues!
Tests: 3, Assertions: 24, Deprecations: 1.

Still it is going to be interesting switching between Drupal 11 and 10 development.

FWIW for core development I don't use a custom phpunit.xml, instead I override various environment variables in ddev, which works the same on both versions of PHPUnit.

Removed the process isolation section from the CR, as that is obsolete now.

Summarised #122-#124 in the change record.

Committed and pushed d4fa83453d to 11.x and 2d3e0bd3fb to 11.0.x. Thanks!

@longwave added content to the CR about updating the phpunit.xml files. Thanks!

mondrake → closed merge request !6326

This change broke relative path resolution

Stopped working (worked in 11-alpha)
./vendor/bin/phpunit -c ./web/core ./web/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php

Works
./vendor/bin/phpunit -c ./web/core $(pwd)/web/core/modules/user/tests/src/Unit/UserAccessControlHandlerTest.php

HEAD is failing and I used git bisect to find out where. And it is this issue. Unfortunately, it is not a simple revert, there are conflicts in bootstrap.inc. I am not sure what the usual process is but I am setting this to needs work and will make a new issue for the revert.

Only 11.x fail but 11.0.x does not https://git.drupalcode.org/project/drupal/-/pipelines?page=1&scope=all&r...

Setting back to fixed as #133/134 was solved in 🐛 Composer tests fail because of invalid Drupal version Fixed