🇮🇹Italy @mondrake

Working on the unit tests.

@jonathan1055 the way core executes individual PHPUnit testsuites is via run-tests.sh --types "$TESTSUITE" --all arguments.

But there is a BUT: there's a difference between the way testsuites are indicated in the .gitlab-ci.yml file and how they're defined in the phpunit.xml.dist testsuites section. run-tests.sh manages on the fly the conversion between e.g. PHPUnit-Unit (in GitLab CI) and unit (in phpunit.xml) and viceversa.

I do not remember what would happen if the --types "$TESTSUITE" --all has a $TESTSUITE value which is not one of the core phpunit's testsuites. Worth checking it, and if it fails, fix here to let fallback the conversion to the input value.

I think it would be worth deprecating --types and --all arguments and introduce sth along the lines of --phpunit-testsuite and --phpunit-group to match PHPUnit's --testsuite and --group arguments, but IMHO should be part of a follow-up issue.

As long as we can accept that these events are not dispatched when the event dispatcher service is not available - that is: 1) early in the request before the full container is available, and 2) during shutdown when the service can have already been destructed, this can proceed.

• 📌 [PP-1] Convert all transactions to explicit COMMIT Postponed would make this issue having some deprecated behaviour (i.e. including in the event information about the commit having occurred during destruct); however since that deprecation removal will be no sooner than D13 (2028?), I think it's worth keeping for now.
• 📌 Explore injecting the event dispatcher in the database service Active would make the event being dispatched also before full container availability - that could be a must-have if we want to move the post commit callback execution to an event-based solution.

Still needs some tests for commit/release/rollback failure events - not easy to create the context - so I will look into mocking.

But the biggest part here is already reviewable.

#6 would be great!! Thanks in advance

If we remove the three lines

   * @todo Uncomment new method parameter before drupal:12.0.0.
   * @see https://www.drupal.org/project/drupal/issues/3354672
   * phpcs:ignore Drupal.Commenting.FunctionComment.ParamNameNoMatch,Drupal.Commenting.DocComment.ParamGroup

from the MR and run phpcs on the file, we get

FILE: [...]/core/lib/Drupal/Core/Executable/ExecutableInterface.php
------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------
 15 | ERROR | Doc comment for parameter $object does not match actual variable name <undefined>
------------------------------------------------------------------------------------------------

Time: 168ms; Memory: 10MB

i.e. phpcs tells us the failing line is the one in the docblock, not the actual method declaration.

And, per https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignorin...,

You can [...] ignore a single line using the phpcs:ignore comment. If placed on a line by itself, this comment will ignore the line that the comment is on and the following line.

So, adding the ignore like this

  /**
   * Executes the plugin.
   *
   * @param object|null $object
   *   (Optional) An object to execute the plugin on/with.
   */
  // phpcs:ignore Drupal.Commenting.FunctionComment.ParamNameNoMatch,Drupal.Commenting.DocComment.ParamGroup
  public function execute(/* ?object $object = NULL */);

I checked it does not work.

Shouldn’t this have a deprecation @trigger_error anyway?

@damien laguerre would you kindly open a new issue for that, and link it here? This issue is closed and unlikely to be reopened.

Quite straightforward.

Filed 📌 Let run-tests.sh print db type and version in the initial output Active that would help in a case like this.

mondrake → created an issue.

This could use a good look by @andypost if the php/db combinations are now accurate - a lot of changes and rebases of late here.

Tweaking this.

Seems wasn't pushed to 11.x

mondrake → changed the visibility of the branch 3530105-with_group to hidden.

No it's a Drupal thing, not PHPUnit.

Drupal's PHPUnit bootstrap script activates DeprecationHandler, that introspects PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION in its ::getConfiguration() method. That method is invoked by PhpUnitTestRunner::runCommand that does this

    // If the deprecation handler bridge is active, we need to fail when there
    // are deprecations that get reported (i.e. not ignored or expected).
    $deprecationConfiguration = DeprecationHandler::getConfiguration();
    if ($deprecationConfiguration !== FALSE) {
      $command[] = '--fail-on-deprecation';
      if ($deprecationConfiguration['failOnPhpunitDeprecation']) {
        $command[] = '--fail-on-phpunit-deprecation';
      }
    }

that is, it adds (or not) --fail-on-phpunit-deprecation to the PHPUnit CLI sub-process spawned by run-tests.sh.

Re #105

Until we do that I don't think we can remove deprecated usages in the test themselves because they'd be needed for phpunit 10.

per se PHPUnit 10 can perfectly process attributes only, so if tests are run via PHPUnit CLI, no problem to have attributes only in contrib. See https://www.drupal.org/project/sophron → for example. The problem is if tests are run via the run-tests.sh script BEFORE Drupal 11.2: in that case the 'old' TestDiscovery process is in place, and that one is not capable of dealing with test classes that lack the @group annotation.

As per #3497431-107: Deprecate TestDiscovery test file scanning, use PHPUnit API instead → , you may opt-out from reporting PHPUnit runner deprecations (if they're too noisy for now), by setting the env variable PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION to FALSE.

There are a few issues already towards converting metadata from annotations to attributes, see related 📌 [meta] Define a Rector rule to convert test annotations to attributes Active , 📌 Convert test annotation to attributes in Drupal/Test/Component Active , 📌 Convert test annotations to attributes in Drupal/BuildTests Active . But they are currently children of the original 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed . It would be probably be the case now to open a new meta for the conversions (there are 000s of files to convert), and reparent those to it.

When it comes to the deprecations, with PHPUnit 11 we need to distinguish between SUT deprecations and PHPUnit runner deprecations - the former are the 'traditional' ones, the latter are internal to PHPUnit. Core, when running tests through run-tests.sh, currently DISABLES PHPUnit runner deprecations by setting the env variable PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION to FALSE. This leads to SUT deprecations being reported but no PHPUnit ones. 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active aims at introducing the same capability in GitLab Templates; contrib using run-tests.sh can anyway already use it if needed.

Lets try then 😀

This would be very very nice, but aside from @borisson_ comment above, I wonder whether we are ready for it.

If I am not mistaken, https://phpstan.org/user-guide/rule-levels, the actual checking of the array shapes' contents will start from PHPStan level 5 above; we are currently struggling with level 1...

So we may be introducing non-validated typing that could become a barrier rather that a facilitation to move to upper levels. Happy to be mistaken though.

@daffie thanks for the detailed review, it will take some time before I can get to it. In the meantime, please consider this is not the endgame, and consider reviewing this in combination with 📌 [PP-1] Allow using Table identifier value objects in database Query classes Postponed and 📌 [PP-1] Use Table identifier value objects instead of table name strings in database Schema classes Postponed that would further implement the changes occurring here.

For instance, when you talk about DX, the endgame would be using the new identifier object with its automatic conversion to string: for example,

$query = $comments . 'DELETE FROM {' . $this->connection->escapeTable($this->table) . '} ';

would become

$query = $comments . "DELETE FROM $this->tableIdentifier";

That's quite a DX improvement IMO... just we cannot do all of it toghether in a single MR otherwise it will become too big.

While pondering on the CR to write, I realized we can probably do better here. If we do 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active first, then we would be able to add here a PHPUnit 10 version of the phpunit.xml (i.e. a copy of the current phpunit.xml.dist in HEAD), and in the CR recommend contrib to switch to use it when running D11 tests with PHPUnit 10. Much neater and less instructions required to tell how to change the xml.

In practice, we would swap the current state where phpunit.xml.dist supports PHPUnit 10 (and 11 with the warnings triggered), and .phpunit-next.xml is the PHPUnit 11 only, currently used for testing only. We would end up with a phpunit.xml.dist for PHPUnit 11 only, and a .phpunit-10.xml for PHPUnit 10 only.

Narrowed the PHPCS uncovered code by replacing phpcs:(dis|en)able with targeted phpcs:ignore single line ignore directives, specifying the rule to be ignored.

Disagree it's out of scope. Sorry it confuses you, but the as-is is confusing to developers who may be assuming the metadata is inherited from the base class to the concrete classes, which is not. See https://github.com/sebastianbergmann/phpunit/pull/5981.

If you grep for abstract classes and look at the class definitions, you'll find most if not all the test base classes definitely have no metadata. So the MR is fixing here base classes that were not defined abstract, and as part of it removing metadata that must not be there.

Abstract base classes do not have metadata on class level (either annotation or attribute) normally. This is corrected here.

Ooops no sorry, #7 is wrong. We need to add group metadata, either annotation or attribute, because it is currently missing. We can add annotation if we want to backport the test; I was adding attribute because IMHO this should be our standard moving forward.

You mean in core/modules/pgsql/tests/src/Kernel/EntityQueryServiceDeprecationTest.php? They can be reverted, yea, not strictly in scope.

Drupal's core composer.json has this

    "autoload": {
        "psr-4": {
            "Drupal\\Core\\Composer\\": "core/lib/Drupal/Core/Composer",
            "Drupal\\Composer\\": "composer"
        }
    },

and I suppose this is why we get Class "Drupal\Composer\Composer" not found during test discovery: a core test's dataprovider tries to access it, it fails, then we get the warning above. Looks like an edge case, but I suspect that this unveils a potential bug: if a contrib module would try to access that class, it would fail.

I don't know... what I think is that "/builds/project/scheduler/web/composer" is a directory that should be reachable but it apparently isn't. That should be I suppose the /composer directory of Drupal core, that is on the 1st level of directories of the package, like /core, /modules, /themes, /sites etc.

@jonathan1055 kindly reviewed the new CR, I published it. Tentatively marking this fixed as it looks to me there's nothing else to do.

Do we do still do test runs of 11.x with PHPUnit 10 or is it only 11 now?

Core in GitLabCI is now consistently on PHPUnit 11 (has been so for just a couple of days though, very fresh), but we can’t say for contrib or people running core tests locally or via other CI tools.

Now a duplicate of 📌 Remove deprecated use of Assert::isType Active

I’ve closed 📌 Assert::isType() is deprecated Active that is now a duplicate.

The only question I have here is whether we should wait for PHPUnit 10 support to be removed, or add PHPUnit 10 polyfills for the new methods. PHPUnit 10 lacks them and we might still have someone testing locally core with it ATM.

#16 this is refactoring internal code, and no outside effects are expected. The only suggestion I would have is to compare test result logs from run-tests.sh, in core and contrib, before and after the change. They should be the same.

@smustgrave please see cr https://www.drupal.org/node/3447698 →

mondrake → created an issue.

Added CR https://www.drupal.org/node/3530388 → , appreciate review and publishing.

I'll work on a CR. Re #97:

But if they are skipped due to 3rd-party module problems then that will me we lose test coverage until they can be fixed.

to me this is not going to mean lost test coverage. We need to distinguish test discovery from test execution. Test discovery looks for ALL the test classes available (based on some pre-constraints sometimes, e.g. if you specifically indicate a directory to search in, it will be limited to that), then filters only those that need to be executed.

In the case of the wrong dataprovider in address and token modules of the related issue, these are reported as failing when building the test list during discovery, but are not executed because the filter is @group scheduler_xxx. So, the scheduler tests are not really affected. They would fail loud on tests executed on those modules with PHPUnit 11 (time to apply the static modifier...), or if, from the scheduler module, you'd run tests for @group token or @group address. But that's because the dataprovider must be static, not for the discovery.

I was on the verge of self-RTBC already, #91 confirms, so let's get going.

So as detailed in #3530183-13: Test Scheduler at 11.2.x-dev with patched PhpUnitTestDiscovery.php → , issue reported in #76 are due to gaps present elsewhere, that the PHPUnit discovery process legitimately cannot deal with.

In the MR, now, an implementation of PHPUnit's Tracer is collecting warnings thrown by PHPUnit during discovery, and reporting them in the run-tests.sh output so they can be actioned. The discovery process is now completed insetad of failing as it was identified in #76, but the actual list of tests to be executed will be missing those tests that errored during discovery.

So. It turns out that PHPUnit is right, and that the error that were reported are legit failures of the discovery process, that was working fine.

In the latest patch, I added a tracer that collects and then reports all events thrown internally by PHPUnit while searching for the test classes to execute.

Here we find now a lot of info, but this is the interesting one:

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\Composer\Generator\BuilderTest::testBuilder is invalid
Class "Drupal\Composer\Composer" not found
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\TestRunner\WarningTriggered: No tests found in class "Drupal\Tests\Composer\Generator\BuilderTest".

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\ComposerIntegrationTest::testComposerTilde is invalid
The "/builds/issue/scheduler-3530183/web/composer" directory does not exist.
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\address\Unit\Plugin\Validation\Constraint\CountryConstraintValidatorTest::testValidate is invalid
Data Provider method Drupal\Tests\address\Unit\Plugin\Validation\Constraint\CountryConstraintValidatorTest::providerTestValidate() is not static
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\TestRunner\WarningTriggered: No tests found in class "Drupal\Tests\address\Unit\Plugin\Validation\Constraint\CountryConstraintValidatorTest".

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\address\Kernel\Formatter\AddressPlainFormatterTest::testRender is invalid
Data Provider method Drupal\Tests\address\Kernel\Formatter\AddressPlainFormatterTest::renderDataProvider() is not static
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\token\Kernel\LanguageTest::testLanguageTokenReplacement is invalid
Data Provider method Drupal\Tests\token\Kernel\LanguageTest::languageTokenReplacementDataProvider() is not static
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\token\Kernel\LanguageTest::testCurrentPageLanguageTokenReplacement is invalid
Data Provider method Drupal\Tests\token\Kernel\LanguageTest::currentPageLanguageTokenReplacementDataProvider() is not static
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\TestRunner\WarningTriggered: No tests found in class "Drupal\Tests\token\Kernel\LanguageTest".

...

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\Tests\address\FunctionalJavascript\AddressViewsHandlersTest::testAddressFieldHandlers is invalid
Data Provider method Drupal\Tests\address\FunctionalJavascript\AddressViewsHandlersTest::addressFieldHandlersDataProvider() is not static
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\Test\PhpunitErrorTriggered: The data provider specified for Drupal\BuildTests\Composer\ComposerValidateTest::testValidateComposer is invalid
The "/builds/issue/scheduler-3530183/web/composer" directory does not exist.
/builds/issue/scheduler-3530183/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:113
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:1049
/builds/issue/scheduler-3530183/web/core/scripts/run-tests.sh:189

* PHPUnit\Event\TestRunner\WarningTriggered: No tests found in class "Drupal\BuildTests\Composer\ComposerValidateTest".

IMHO, this means that

a) the GitLab Templates is not symlinking core's /composer subdirectory, so the classes cannot be autoloaded. Quickly checked the script that does that there, and in fact it is missing;

b) in the address module, some Data Provider methods are not static. This is a failure in PHPUnit 11;

c) same in the token module.

So issues should be filed against each of those projects for fixing the problems.

run-tests.sh test discovery is not failing, anyway, but it will not include tests that have legitimate problems. And now the causes are reported too so actions can be taken.

So I think I nailed the problem(s). Will post a new patch for core soon then explain.

Copy/pasting here more details I added in #3530183-8: Test with 11.2.0-rc2 → .

I could replicate the issue in Image Effects https://git.drupalcode.org/project/image_effects/-/merge_requests/72 by choosing to select tests by group instead of by directory.

To me the issue is a bug upstream: PHPUnit 'builds' the list of tests to execute as a hierarchical structure of TestSuite and TestCase objects, and seems to be adding empty TestSuite sub-objects to the main TestSuite object under some unclear circumstances.

Core patch developed here seems to work, and it is now a follow-up MR in core's #3497431-88: Deprecate TestDiscovery test file scanning, use PHPUnit API instead → .

It just skips, though, empty TestSuite objects which may lead to skipped test runs if the @group/#[Group] of the test class skipped corresponds to the group filter when running runt-tests.sh. I will keep investigating in Image Effects issue 📌 Test with 11.2.0-rc2 Active .

I think core's 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active would help as it could open up the possibility to declare the directories to be searched for test discovery by the module, if run-tests.sh execution is filtered by test group. This will prevent discovering all of the core test files as it is happening now.

I could replicate the issue in Image Effects https://git.drupalcode.org/project/image_effects/-/merge_requests/72 by choosing to select tests by group instead of by directory.

To me the issue is a bug upstream: PHPUnit 'builds' the list of tests to execute as a hierarchical structure of TestSuite and TestCase objects, and seems to be adding empty TestSuite sub-objects to the main TestSuite object under some unclear circumstances.

Core patch developed here seems to work, and it is now a follow-up MR in core's #3497431-88: Deprecate TestDiscovery test file scanning, use PHPUnit API instead → .

It just skips, though, empty TestSuite objects which may lead to skipped test runs if the @group/#[Group] of the test class skipped corresponds to the group filter when running runt-tests.sh. I will keep investigating in Image Effects issue 📌 Test with 11.2.0-rc2 Active .

I think core's 📌 Allow indicating alternative phpunit.xml than core's when testing via run-tests.sh Active would help as it could open up the possibility to declare the directories to be searched for test discovery by the module, if run-tests.sh execution is filtered by test group. This will prevent discovering all of the core test files as it is happening now.

We might have surfaced a bug upstream (PHPUnit's test suites builder seems to be adding empty TestSuite sub-objects to the main TestSuite object under some unclear circumstances), but this minimal MR!12382 fix seems to let the overall testing discovery complete in that case. 📌 Test with 11.2.0-rc2 Active is now passing with the equivalent of the MR as a patch.

Will try to see if I can report upstream, but that will require quite some investigation before being able to - building a self-contained minimal patch reproducing the issue is not going to be a simple one.

Filed 📌 Introduce PHPUNIT_FAIL_ON_PHPUNIT_DEPRECATION Active in GitLab Templates issue queue.

📌 Test with 11.2.0-rc2 Active has a debug patch for core that now makes tests pass. Most important would now be to understand why PHPUnit sometimes does not pass a filled in Test object; then we can make a core patch.

Do I need to open a separate issue or an additional MR in this issue, eventually?

mondrake → created an issue.

Well we may argue there are too many ways to filter and get the tests needed for running, but we need to support them all ATM, alas. Simplifying that part would be nice, but I guess that will require deciding how and the deprecating hownots.

For converting annotations to attributes, I worked on a rector rule if you want to try - https://www.drupal.org/project/drupal/issues/3446380 📌 [meta] Define a Rector rule to convert test annotations to attributes Active

There are two issues in NR that were built using that rule, 📌 Convert test annotations to attributes in Drupal/BuildTests Active and 📌 Convert test annotation to attributes in Drupal/Test/Component Active

Thanks @jonathan1055; I appreciate your reporting the issue and support now.

Opened 📌 Test with 11.2.0-rc2 Active in Scheduler's module issue queue to try and understand what's going on.

mondrake → created an issue.

Image Effects runs tests with pwd = /builds/project/image_effects
Project Browser runs tests with pwd = /builds/issue/project_browser-3530110

I am looking into this. Knowing the inners, it's very strange as that exception is thrown after the test list has been already built by PHPUnit, which is common with the other two tests.

The error is

Drupal\Core\Test\Exception\MissingGroupException: Missing group metadata in test class Drupal\Tests\Composer\Generator\BuilderTest in /builds/project/scheduler/web/core/lib/Drupal/Core/Test/PhpUnitTestDiscovery.php:304

which is very very strange as that test (which is a core test btw) clearly DOES have a @group Metapackage annotation and it runs no problem in core.

Gut feeling is that the contrib is playing with the pwd of the job and PHPUnit/autoloading cannot find the class.

The new code can manage both @group annotations and #[Group] attributes, otherwise 99% of core would be failing. So contrib is not requested to add the attribute. The linked tests in #72 and #73 prove it anyway.

So it must be something else, looking.

EntityQueryServiceDeprecation

is actually not running as a test in HEAD currently.

mondrake → created an issue.

Unblocked, rebased.

Updated release note snippet.

Unblocked. Rebased and fixed new CS errors reported.

Blocker is in. Rebased.

mondrake → created an issue.

Project Browser seems to work too Test with 11.2.0-rc2 📌 Test with 11.2.0-rc2 Active

mondrake → created an issue.

Image Effects seems to work fine Test with 11.2.0-rc2 📌 Test with 11.2.0-rc2 Active

mondrake → created an issue.

MR !12367 was a mistake.

Let's try #3 in a separate MR, and see what that brings. That extension provides at least another rule that we could leverage.

A nice alternative would be to add https://github.com/ergebnis/phpstan-rules as a dev dependency, enable the https://github.com/ergebnis/phpstan-rules?tab=readme-ov-file#functionsno... rule, add the failures of the current codebase to the baseline, and have PHPStan report all new code missing the statement.

Added a touch of reality to the IS, 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed is still just a proposal for now.

done

on it

Postponed on 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active

📌 Update PHPStan to 2.1.17 Active is also RTBC by the way

📌 Update PHPStan to 2.1.17 Active is also RTBC by the way

I think it's about time to drop support for PHPStan 1... in any case the baseline would differ significantly.

📌 [CI] Run PHPStan job on PHP 8.4 Active is taking longer, let's try do this for 11.2

mondrake → created an issue.

mondrake → created an issue.