Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency

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, hopefully we can add backward compatibility shims for things like this again.

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.

Added reference from #3267879: [meta] Add compatibility for the latest major and minor versions of dependencies to Drupal 10 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?

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.

GitLabBot found a few errors.

Extracted the TestSuites removal into #3443732: Remove deprecated PHPUnit test suites

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.

Similarly HelpTopicSearchTest::testUninstall works fine on its own but when run with the other tests in the class it fails with "You have requested a non-existent service "locale.config_manager"."

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.

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. As far as I can tell attributes and annotations are only read from the test class directly, parent classes are not taken into account.

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.

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

But, given there are not many failures left here, maybe the answer is just to explicitly declare it where needed as we have already.

I don't think two phpunit.xml files is workable, it will be painful to remember to use the right one when running tests locally.

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 first real page load in the test, in PHPUnit 9, the key is now 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:

• #3427173: Replace calls to :::expectWarning*() in Drupal\Tests\Component\PhpStorage\FileStorageTest

Kernel tests:

• #3440044: Replace catch of PHPUnit\Framework\Error\Warning in MarkupInterfaceComparatorTest
• #3440043: Stop triggering a warning in MySQL's TransactionManager rollback

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?

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 #3443894: TestHttpClientMiddleware should prevent requests to other hosts 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 #3095755: Tests should not do requests to updates.drupal.org

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

sorry, commented by mistake

@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 #3443894: TestHttpClientMiddleware should prevent requests to other hosts.

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 never executed 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.

@mondrake is #3443894: TestHttpClientMiddleware should prevent requests to other hosts 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.

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.

I think we could remove the test coverage for #3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule 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.

(I find it amusing but also a testament to the work done here that we have three PhpUnitBridgeTest classes and they pass with no changes even though the original bridge is removed)

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, also updated the change record with a note about process isolation. 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

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

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.

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? nope doesn't work according to stuff above.

This is really painful.

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

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

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

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

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?

This means that whether we use the constructor or the attribute to run the tests in isolation, I don't think the HtmlOutputLogger will work as intended.

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

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

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.

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?

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

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

Thanks for reworking the output logger, the solution looks good. 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!

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 with absolute path
./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.php. 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 #3445211: Composer tests fail because of invalid Drupal version

Shouldn't this have been removed from phpunit.xml.dist?

    <!-- To disable deprecation testing completely uncomment the next line. -->
    <!-- <env name="SYMFONY_DEPRECATIONS_HELPER" value="disabled"/> -->

Also, with Symfony PHPUnit Bridge, you could get deprecations to show a backtrace in tests. I don't think there's a way to do that with plain PHPUnit.

Filed #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency for phpunit.xml.dist.

In another issue @mondrake asked for a change to the instructions for tests to the 'how to deprecate' document. I thought they were a bit out of scope so didn't do the work there.

To address the point I searched and found that this issue is the one that added the ability to use "#[IgnoreDeprecations]". It would have made sense to do the doc updates as part of this issue. Hopefully, on the next PHPUnit update issue it can be tagged for documentation update so that can happen, if needed.

I have updated how to deprecate.