🇮🇹Italy @mondrake

mondrake → created an issue.

mondrake → created an issue.

Looks good to me, thanks - I'll leave RTBC to someone that sees this from the outside, I'm too much an insider here :)

A comment added inline.

#18 this issue is about moving AWAY from ::commitAllOnShutdown() (and in general from committing during object destruction which has drawbacks).

IMHO, ::commitAllOnShutdown() should only be transitional towards such goal.

Added draft CR.

MR merged with 📌 TestDiscovery expects @group annotations, fails with #[Group()] attributes Active to check with TestDiscovery suppoorting #[Group] attributes.

Tests are green now.

Maybe related to 🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review ?

I think the --process-isolation argument can be removed, too, since the final solution in core was to enable process isolation on the base test classes. Maybe this could go in docs as the --fail-on-deprecation argument, but should not be necessary unless in edge cases, truly.

@fgm see this for the template adjustments: #3444792: Prepare for PHPUnit 10 → .

Tests need fixing, but I think the concept is reviewable.

Getting rid of custom code and just relying on PHPUnit to discover tests is a bit too far and actually PHPUnit itself only recently changed format for --list-tests-xml output, and modified test list filtering options, in PHPUnit 11.1. So better wait to be on that before trying it.

For now just extended TestDiscovery to look into attributes of test classes, falling back to annotations if they do not exist (like PHPUnit itself is doing).

mondrake → created an issue.

The @medium annotations can no longer be associated to methods, as attributes.
Only to classes.

mondrake → created an issue.

Currently developing a Rector script for this in https://github.com/mondrake/d8-unit/blob/test-rector/rector.php

There are just few more outside of Drupal/Tests, we can do them all.

mondrake → created an issue.

mondrake → created an issue.

I agree with #9 btw, and think in core we should just remove the annotations, and document the possible double standard for contrib.

Note sure about #11. It doesn't seem to be an issue in the project I referenced. Maybe if the class is missing yes, me might have a problem with PHPStan reporting it, but runtime the attribute is just handled like a comment?

AFAICS we cannot ignore deprecations thrown by PHPUnit itself, anymore. PHPUnit is not using trigger_error for deprecations, but it's own event system. But we could apparently have both annotations and attributes and being compatible with PHPUnit all the way from 9 to 11 - see for instance https://github.com/FileEye/MimeMap/blob/master/tests/src/TypeTest.php#L7... in a separate project. AFAICS PHPUnit will throw a deprecation when no attributes exists AND annotation exist; if the test has attributes, PHPUnit 11 will rely on those and not check about existing annotations.

Given test results, the only big thing to get is onto PHPUnit 11 is only the replacement of test annotations with PHP attributes, that throw thousands of deprecations. Actual errors and failures are just a few (functional ones seem actually error-free)

Filed 📌 [meta] Define a Rector rule to convert test annotations to attributes Active . See you there!

mondrake → created an issue.

Note that the Performance Test is throwing deprecations, even if the setup is such that the test job does not fail. That's because the deprecation handler that replaced Symfony's in PHPUnit 10 is stricter with deprecations thrown by the DebugClassloader. In this case it's the OpenTelemetry classes that are not typehinted.

PHPUnit 10.5.20 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.6
Configuration: /builds/project/drupal/core/phpunit.xml.dist

DDDDDDDD                                                            8 / 8 (100%)

HTML output was generated, 49 page(s).

Time: 12:00.036, Memory: 186.00 MB

8 tests triggered 8 deprecations:

1) /builds/project/drupal/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "ArrayAccess::offsetGet()" might add "mixed" as a native return type declaration in the future. Do the same in implementation "OpenTelemetry\SDK\Common\Attribute\AttributesBuilder" now to avoid errors or add an explicit @return annotation to suppress this message.
Triggered by:
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testFrontPageAuthenticatedWarmCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php:32
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageColdCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php:26
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageCoolCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php:79
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryFrontPagePerformanceTest::testFrontPageHotCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryFrontPagePerformanceTest.php:43
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageColdCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:26
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:67
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageHotCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:43
* Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache
  /builds/project/drupal/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:85
2) ....

In this case if this were a test run via run-tests.sh, it would fail. But here it's run via PHPUnit CLI without the --fail-on-deprecation command line argument, so deprecations are displayed but do not lead to test failure. If desirable, the indirect deprecation can be silenced like in https://git.drupalcode.org/project/drupal/-/blob/11.x/core/.deprecation-...

#11 is this won't fix then? RTBC to get a core committer decide.

mondrake → created an issue.

Note that WARNINGS and DEPRECATIONS in PHPUnit 10 do make the test job fail, but are not reported in the GitLab CI test summary.
You need to look at the raw log of the job failure to get details of the failure.

For example, I see in the MR failure here

PHPUnit 10.5.20 by Sebastian
Bergmann and contributors.

Runtime: PHP 8.3.6
Configuration: /builds/issue/drupal-3446164/core/phpunit.xml.dist

W 1 / 1
(100%)

HTML output was generated, 305 page(s).

Time: 01:06.504, Memory: 10.00 MB

1 test triggered 1 PHP warning:

1) /builds/issue/drupal-3446164/core/includes/install.core.inc:1805
Undefined array key "major"

Triggered by:

*
Drupal\FunctionalTests\Asset\AssetOptimizationTestUmami::testAssetAggregation

/builds/issue/drupal-3446164/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php:37

OK, but there were issues!
Tests: 1, Assertions: 240, Warnings: 1.

Is it possible to have a link to a failing pipeline? What is actually failing?

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.

#6 - ouch that's painful.. however we could keep annotations as well as attributes in the same test file, so for BC to have PHPUnit 9 compatiblity (using annotations) and PHPUnit 10/11 compatibility (using attributes) at the same time. See for example these tests in a library I maintain on GitHub: https://github.com/FileEye/MimeMap/blob/ba0b04e179976e7d6a487fdb166c5f1f...

However, we can't proceed by annotation. Do we need an issue to build a comprehensive Rector that would address all annotations and then use that to proceed by test suite/core-module?

Some comments inline.

Right!

Here I would like to check what is the error handler at that point: if it’s already set to PHPUnit’s one we probably do not need to create a nee instance.

Re-added setRunTestInSeparateProcess(TRUE) in base classes.

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

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

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

Great find in #112 BTW!

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.

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.

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.

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.

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

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.

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

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.

Uploading latest version of the rector script

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.

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

To run this:

• install rector via composer require -dev rector/rector
• run rector with the config file via vendor/bin/rector
• run PHPCBF via vendor/bin/phpcbf --filter=GitModified --standard=SlevomatCodingStandard --sniffs=SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses core (this reorders alphabetically the use imports for cleanliness
• your diff will be what to put in the MR

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

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

mondrake → created an issue.

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)

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)

Added deprecation ignore

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

Can you please make dataproviders static so we don’t mess up with PHPUnit 10 upgrade ?

Out of curiosity I tried run Rector with this config

<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\PHPUnit\AnnotationsToAttributes\Rector\ClassMethod\DataProviderAnnotationToAttributeRector;

return RectorConfig::configure()
    ->withPaths([
        __DIR__ . '/composer',
        __DIR__ . '/core',
    ])
    ->withSkipPath(
        __DIR__ . '/core/tests/Drupal/Tests/Component/Annotation/Doctrine/Fixtures',
    )
    ->withSkipPath(
        '*/ProxyClass/*',
    )
    ->withSkipPath(
        '*.api.php',
    )
    ->withRules([
        DataProviderAnnotationToAttributeRector::class,
    ])
    ->withImportNames(
        importDocBlockNames: false,
        importShortClasses: false,
        removeUnusedImports: false,
    );

and it produced the attached patch; it's not perfect but it should fix 95% of the cases. A bit annoying the added use import is not sorted alphabeticalyl, but I assume some PHPCS magic could happen to fix that as well.

mondrake → created an issue.

Parenting

Removing PHPUnit 10 tag, it's no longer strictly related to the PHPUnit upgrade.

This one will be eventually fixed in 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Needs work .

We're down to the 2 last issues to be committed, 📌 Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule RTBC and 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Needs work , then this one would be done - 3 years and 160 issues later :)

Couple of glitches I think

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

https://docs.phpunit.de/en/10.5/textui.html

I suggest to wait a sec... there are other changes in command line options that will pop-up: for example, the --verbose option is removed, and I think the --printer one too.

Applied suggestions and replied to inline comments.

Well, in core that depends on the value of the SYMFONY_DEPRECATIONS_HELPER environment variable. If that enables the deprecation handler, then unexpected deprecations lead to test failure. If we want to keep in sync with core, we probably need to match that behavior here.

... but it looks like I can't trigger the 'Test Key CDN' job.