🇮🇹Italy @mondrake

mondrake → created an issue.

mondrake → created an issue.

back to rtbc

The composer lock test fail on PHP 8.3 seems a legit one.

@ghost of drupal past sorry I do not understand your question. Can you please elaborate?

Closing this issue, since the fixes identified here are now part of core's 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active .

Apparently the syntax to pick the test files in the /module subdirectory was failing somehow, changed after some thorough manual testing documented in https://drupal.slack.com/archives/CGKLP028K/p1748635336355999

This should hopefully fix the issue in 📌 Update for changes to System requirements Active with no changes to need happen there (once this is back in).

mondrake → created an issue.

Thanks for fixing that typo!

mondrake → created an issue.

Sorry, better wait for #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead that is now partially overlapping here too.

Done #58.

Will be solved in the re-opened parent.

Solved the CSpell problem.

Yes needs waiting for https://www.drupal.org/project/drupal/issues/3497431 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active

I do not understand why the CSpell job fail given that the file reported is included in the ignored paths in .cspell.json.

Then waiting for feedback to question in #3527063-8: Update for changes required by D11.2 → to see if we do need to edit the <directory> sections of phpunit.xml.dist to accomodate contrib testing.

Better wait for 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active that is now partially overlapping.

Question: in this type of build, what is the path to the directories where test classes are defined? In relation if possible to Drupal's core /core subdirectory.

Asking because if it's missing from the list here (example for Javascript tests section of core's phpunit.xml.dist), we may want to try to add to the MR in 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active .

    <testsuite name="functional-javascript">
      <directory>tests/Drupal/FunctionalJavascriptTests</directory>
      <directory>modules/**/tests/src/FunctionalJavascript</directory>
      <directory>recipes/*/tests/src/FunctionalJavascript</directory>
      <directory>profiles/**/tests/src/FunctionalJavascript</directory>
      <directory>themes/**/tests/src/FunctionalJavascript</directory>
      <directory>../modules/**/tests/src/FunctionalJavascript</directory>
      <directory>../profiles/**/tests/src/FunctionalJavascript</directory>
      <directory>../themes/**/tests/src/FunctionalJavascript</directory>
    </testsuite>

In next iteration, I would consider doing:

1) Bump PHPUnit to 11 in the lock file, instead of keeping it locked to 10. 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed has been in for 3 weeks now and AFAICS no issue have been reported about that. PHPUnit 11 also support PHP 8.3, which is minimum for D11. We can still keep 10 in the composer.json in case anyone has the need to downgrade for incompatibility (which, again, was not found yet).

2) Include 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active which was meant to be a followup of this, but since now it’s open again, better do straight ahead.

Then, if 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active willl still not be resolved, it should be (theoretically) only a matter of listing the relevant directories in phpunit.xml.dist - and that would require another core fix if contrib is meant to run with core configuration.

I’d also suggest to commit initially only to 11.x to allow proper checking in gitlab templates, and only eventually backport to 11.2.x upon complete validation.

One thought: would bumping core lock to PHPUnit 11 with option to downgrade (opposite than HEAD now that locks PHPUnit 10 with option to upgrade) be an option here?

IMHO the first thing that should happen here is to bump PHPUnit to 11 anytime Drupal SUT is 11.2 or above, but composer isn’t friendly here, it seems drupal/core-recommended locks packages down.

See attempts at #3527063-5: Update for changes required by D11.2 → .

Then yes, there maybe bugs to fix in latest core.

However, Image Effects seems to run tests just fine when specifying --directory instead of --module :https://git.drupalcode.org/project/image_effects/-/pipelines/508827

Cannot find a way out.

1) Cannot bump PHPUnit to 11 as core jobs do. PHPUnit 11 requires sebastian/diff 6, but apparently drupal/core-recommended has requirements that block both updates.

2) Trying to run tests by specifying --directory instead of --module does not work. Strange because tests for Image effects do work: https://git.drupalcode.org/project/image_effects/-/pipelines/508827

Going to bed now...

mondrake → made their first commit to this issue’s fork.

Sorry I see the job failed, but cannot understand what is the MR that originated it? Can you share? Mind if I give a couple tries?

Generally speaking, I would suggest to bump PHPUnit to 11 for testing Drupal 11.2 onwards. Core is doing it for PHP 8.4 and above, but PHPUnit 11 also supports PHP 8.3. That should fix test discoverability hopefully .

Missing group metadata sounds to me it’s missing as an annotation either, but a @group annotation is requested by run-tests.sh (and by code standards IIRC). Should not have a problem if running via PHPUnit CLI directly.

Given that 📌 Use avif with webp fallback for all core image styles Active has practically made AVIF the default for 11.2, I think this will be Critical in case PHP is not binding the avif functions.

With this in, 📌 Convert test annotation to attributes in Drupal/Test/Component Active and 📌 Convert test annotations to attributes in Drupal/BuildTests Active will be passing.

'Active' is more appropriate as a status, maybe.

Added inline comments

one more failure, on it

Postpone on 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active

Filed 🐛 PhpUnitApiGetTestClassesTest and PhpUnitApiFindAllClassFilesTest need to execute PHPUnit discovery before TestDiscovery Active after finding an issue while working on the followup conversions from annotations to attributes.

mondrake → created an issue.

mondrake → created an issue.

Adjusting new tests to use attributes.

MR built on top of 📌 Reduce run-tests.sh complexity in spawning subprocesses Active .

Found an interesting bug... no matter what the CONCURRENCY was set on the child pipeline variables, HEAD is anyway using 24 which is what is set on tha main gitlab ci job.

mondrake → created an issue.

Thank you!

Do we need publishing the CR?

LGTM, albeit this could use a +1 by someone more familiar with build tests.

It seems PHPUnit does not like files where multiple namespaces are defined (e.g. the test namespace + test class + a fixture namespace to work around SUT).

BTW, neither Rector does: it fails to determine the ‘main’ namespace and therefore forces the use of FQCN while adding attributes, instead of adding an use import + attribute short class name.

Maybe for now we just skip converting those cases.

Weird that we get this only while moving to attributes.

Can't figure out what would be impacting pipelines here - the MR one after revert and rebase seems to work fine. Pipeline config is not touched apart from the addition of a parameter when calling run-tests.sh.

All blockers are in. Will be working on this one.

Only merged with head, now tests are green. There's fortuity in randomness of test failures.

Let's do 📌 [CI] Run PHPStan job on PHP 8.4 Active first.

mondrake → created an issue.

I do not think

// @phpstan-ignore missingType.return
/**
 * Test 
 */
public function ignore_phpstan_comment() {
}

needs to be supported - PHPStan will fail in that case as the ignore is not in the line immediately above the concrete code failing.

#161 well for sure NamedPlaceholderConverter has its own overhead. But yea... later.

Ok, I think we are safe assuming if anyone is using the mysqli driver, we will not support doing magic to fix malformed settings in settings.php like we do for the drivers that were present in Drupal before database drivers became modules.

Looking into latest feedback.

.. and it's not Critical. Maybe Major since we're in the middle of the river crossing from default PHP 8.3 to default PHP 8.4 for GitLab jobs.

Updated IS because the problem is not an override of the code, but from where the job executes.

Made some updates to the CR to provide some input how to convert tests from annotation to attributes. Would be lovely to get a review there too.

Thank you @quietone

@quietone, fwiw the issue that would unblock broader usage of attributes vs annotations in core is 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active . That would allow using #[Group('bar')] attributes in place of @group bar annotations in particular, which is causing fatal errors right now.

Unfortunately the needs:job entry does not accept variables.

The Build test failure had the same root cause of 📌 [CI] Run PHPStan job on PHP 8.4 Active .

composer run-script drupal-phpunit-upgrade-check, run during the PHPCS lint job, flashes a new composer.lock file as well as the packages metadata json files. But only the /vendor folder is stored as an artifact, not the updated files.

The MR now is built on top of the one in 📌 [CI] Run PHPStan job on PHP 8.4 Active ; it is extended by including also the updated files in the PHPCS artifacts.

The downside: now the composer build artifacts as built by PHP 8.4 are also used by the PHP 8.3 tests of the child pipelines.

Added comments across the entire script.

... you can (and in fact should) use #[IgnoreDeprecations] right now. There's plenty of examples in HEAD already.

I think we should be a little bit future proof here - @group legacy annotation will have to be dropped soon. So I would suggest to mention using the #[IgnoreDeprecations] attribute as main direction, and leave the use of the annotation in brackets with info that it will have to be dropped before we hit PHPUnit 12.

Per https://drupal.slack.com/archives/C1BMUQ9U6/p1747310372256759?thread_ts=..., the MR could use some manual testing to check that with xdebug 3.3+ enabled we do not get any longer failures related to the unpredictable object destruction sequence.

Fleshed the CR

Please try the beta of the 4.1 release.

https://www.drupal.org/project/image_effects/releases/4.1.0-beta1 →

mondrake → created an issue.

Suggest doing this directly in the parent issue.

Lint cache warming will not work proper with this, the cache would be taken from the wrong place. I think it's better go straight to doing what 📌 [CI] Run PHPStan job in the GitLab project directory instead of /build Postponed would do. Doing it here so we can keep the status and history.