Problem/Motivation

Process isolation in PHPUnit 10 must be specified on a single class level or as a global switch.

This causes issues because Drupal needs to run Unit tests NOT in isolation for performance reasons.

Proposed resolution

In this issue, explore adding #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests, through an automated Rector script.

How to update the MR

  • from a git branch of the MR
  • reset the branch to current 11.x: git reset 11.x --hard
  • install rector: composer require --dev rector/rector
  • revert composer changes: git checkout -- .
  • download the rector config file attached here as rector.php in the root directory, and git ignore it
  • run rector: vendor/bin/rector
  • run PHPCBF to reorder use import statements: vendor/bin/phpcbf --filter=GitModified --standard=Drupal --sniffs=SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses,PSR2.Namespaces.UseDeclaration,PSR2.Namespaces.NamespaceDeclaration,Drupal.Classes.UnusedUseStatement core
  • commit changes to branch
  • force push commit

With over 3k files touched, we should try to avoid making manual changes - rather tune the rector script.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 rector.php_.txt3.37 KBmondrake
#3 rector.php_.txt3.19 KBmondrake

Issue fork drupal-3445240

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

mondrake created an issue. See original summary.

mondrake opened merge request !7914

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new rector.php_.txt3.19 KB

Attached file is the Rector config file (rector.php) that conveniently has a Rule class to do what is in the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Postponed

So, chicken and egg - we can't do this until we have PHPUnit 10 in place, otherwise PHPStan complains about the fact that the attribute class does not exist.

andypost’s picture

andypost
he/him
Primary language Russian
commented

I bet it will affect split of tests for running in parallel to optimize pipeline

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3217904: [meta] Support PHPUnit 10 in Drupal 11
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new rector.php_.txt3.37 KB

Uploading latest version of the rector script

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Postponed » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented

Only manual changes:

  • one addition to PHPStan ignoreErrors, to be removed once PHPUnit 10 is in
  • fixes for two functional tests that were hardcoding the line number of their scripts in a check
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Closed (won't fix)

As noted over in #3417066-120: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency, we can't mix PHPUnit attributes and annotations, we will have to convert them all at once.

mondrake closed merge request !7914

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Closed (won't fix) » Active
Issue tags: +PHPUnit 12

I'm afraid we have to reopen this in light of PHPUnit 12 - that is making final the constructor of TestCase. We can no longer override it to set programmatically at the base class level.

The issue was reported upstream but no alternatives were suggested: https://github.com/issues/created?issue=sebastianbergmann%7Cphpunit%7C5838

So I think we need to

  • add the #[RunTestsInSeparateProcesses] attribute to all existing kernel and functional tests
  • add a PHPStan rule that will prevent new kernel and functional tests to miss the attribute
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Postponed

We need to convert all tests from annotation to attributes, first.

mondrake opened merge request !12669

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 11.0.x-dev » 11.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
commented
Parent issue: #3417066: Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency » #3535662: [meta] Convert test metadata from annotations to attributes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes

mondrake opened merge request !13195

mondrake changed the visibility of the branch 3445240-rule to hidden.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed #3546029: Ensure that #[RunTestsInSeparateProcesses] attribute is added to all Kernel tests that imho should happen first.

mondrake changed the visibility of the branch 3445240-metadata-check to hidden.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Add #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests » [meta] Add #[RunTestsInSeparateProcesses] attribute to all Kernel and Functional tests
Status: Postponed » Active
Issue tags: +PHPUnit 11
mondrake’s picture

mondrake
Location 🇮🇹
commented
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Reviewed & tested by the community

All children are in, I think we can mark this fixed too - leaving to RTBC for any input

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I think we're clear here, shame we had to do it, but glad that it is done!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Is there a PHPStan rule that can be used by downstream projects to ensure that tests always have the #[RunTestsInSeparateProcesses]?

I tried adapting the Rector code from #10, but it didn't work.

Still wrapping my head around the consequences of not including #[RunTestsInSeparateProcesses]... will tests break in PHPUnit 12? Or will they still run, but not in a separate process, and that might go unnoticed?

If the tests will outright break, then maybe a PHPStan rule isn't needed. I just want to make sure we don't miss something.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Yes, the Drupal\PHPStan\Rules\TestClassClassMetadata rule exactly implements that check.

Not including the attribute in PHPUnit 12 will mean the test will be run in the same runtime space of the PHPUnit framework; as long as only one test is executed, it will likely not have problems, but as soon as multiple tests are executed in the same testrun, Drupal global settings of each test method executed will leak into the next test and there will be fun.

m.stenta’s picture

m.stenta
he/him
Primary language English
commented

Thank you for the quick answer @mondrake!

Yes, the Drupal\PHPStan\Rules\TestClassClassMetadata rule exactly implements that check.

Sorry if this is off-topic, but is there guidance anywhere for using core PHPStan rules in contrib projects? I'll try to dig into understanding how the core GitLab runner works... but any pointers would be appreciated!

mondrake’s picture

mondrake
Location 🇮🇹
commented

#33 not that I am aware of. You may want to ask on Slack in #gitlab? Or file a policy issue even here in core. Good question!

mondrake closed merge request !12669

mondrake closed merge request !13195