🇮🇹Italy @mondrake

FTR, @coversClass has never been a PHPUnit supported annotation...

Filed 📌 Remove remaining references to @group annotations from tests Active , 📌 Remove remaining references to @covers* annotations from tests Active , 📌 Remove remaining references to @dataProvider annotations from tests Active

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

Cool.

We shouldn't, but I saw this happening in a commit recently (sorry I lost track of which one). But yeah - it's harmless anyway and now numbers are just too big to baseline them at the moment.

I think we can close this now. Yes there are couple of follow-ups open but the test codebase is now fully converted.

Setting to RTBC for any comment.

Good catch.

Few comments:
1) this is now partially overlapping with 📌 Enforce removal of PHPUnit annotations from test classes' class metadata Postponed , in the sense that it analyses same scope with slightly different checks. May be better try to have one rule only?
2) the check on #[Group] attribute presence here is limited to Functional and Kernel tests AFAICS - should be on any test class actually

Approx 1k instances of $this->container in Functional/FunctionaJavascript tests... I think that probably needs a meta and childs.

+1, can we check what would be the impact of deprecating access to $container here before deciding if to do here or in follow up?

Also, I wouldn't have allowed access to dynamic properties at all; minor though as in any case both warnings and deprecations are set to cause tests to fail.

added a comment inline

Not sure it's worth changing this isolated case here.

My suggestion would be to do, for core/tests:

• 📌 Add return types to core tests code via Rector Active adding some return types
• Round 2 (issue tbd) - complete return types and add argument types via Rector
• Round 3 (issue tbd) - add types to class properties via Rector
• Round 4 (issue tbd) - convert anything that is left out

Rebased and reverted changes to DocParserTest, back to RTBC.

Per 4.0.0 release notes, this is expected as now the module enforces strict typing.

Uh oh... 4185 errors would be added to the baseline...

mondrake → created an issue.

Let's be bold here, #28 is almost two years old and no activity since.

Looks like the consolidated approach now is to use autowiring attributes in code to solve this issue - I suggest to close won't fix this

Out of curiosity, I checked out the rotated image derivatives from the CI runs, and they're actually quite different in PHP 8.4 and 8.5. Attaching here.

This is an area where local testing may yield different results, as PHP with the embedded GD library (that Drupal GitLabCI uses) has code quite different from PHP that links the library separately, as #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ → shown extensively.

All the new Kernel tests (one per image type) are green on PHP 8.5 now.

#29 - thanks @andypost, AVIF now looks OK in latest tests.

#28 - In the end I think we should be testing conditionally on PHP version, without changing the default interpolation algo. Latest commit does that.

IMHO

imagesetinterpolation($toolkit->getImage(), IMG_NEAREST_NEIGHBOUR);

should be placed in the toolkit code, conditionally on the PHP version, not in the test. Otherwise we'd be testing something that does not match the normal runtime behavior.

Or, we need to have different reference values to check against (again, PHP version-dependent).

Setting to NR for comments.

AVIF images misdetected as HEIF after introducing HEIF support in getimagesize() was fixed upstream so in PHP 8.5RC4 we should not longer have that part to be solved.

What remains is the different default interpolation used in PHP 8.5 vs earlier versions.

This test is one of the longest running kernel tests, I think it would make sense to split it in an abstract base class and one concrete test class per image type (gif, png, webp, etc.).
It should not be that hard, will try to look into it.

#93 and #94 IMHO the effort to try supporting global constants in unloaded .module files is not worth doing at this point. So dropped it in the last commit.

Retitled and updated IS.

Blocker is in.

Looks good to me.

#8 sure it would make sense. Can PHPCS selectively detect that the test class inherits from one of KernelTestBase or BrowserTestBase? Only those should have the attribute, Unit and Build tests do not need it, at least by default.

mondrake → created an issue.

I think this should be cherrypicked to 11.3.x too

📌 doctrine/annotations is abandoned Active seems the culprit, may need reverting

Yes...

HEAD test jobs are broken right now.

Adding a release snippet in case

Rebased post commit of 📌 Remove support for PHPUnit 10 Active

I think the CR of 📌 Remove support for PHPUnit 10 Active is enough. Not sure about a release note specifically for this issue, but leaving the tag.

mondrake → changed the visibility of the branch 3554077-incorrect-failure-message to hidden.

I think @smustgrave this is the issue 🌱 [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages Active

Thank you!

The MR is built on top of 📌 Make Drupal\Core\Database\Database type strict and PHPStan L10 compliant Active , but it is reviewable now.

Newbie here, excuse my ignorance

Could this help with 🐛 [CI] Performance pipeline execution drops warmed caches Postponed ?

@smustgrave yes, or postponed on the referenced. There are already a bunch of issues about run-tests.sh that are not moving, alas, let's not add more.

📌 Enforce removal of PHPUnit annotations from test classes' methods metadata Needs review will report #50 as a PHPStan error.

Apparently, PHPUnit 11 is not throwing deprecations when @testWith is used...

Any reason why Drupal\image\ImageEffectBase is not converted?

Committed to 5.0.x and cherry-picked to 4.0.x

Not cherry-picked to 4.1.x because there is not likely going to be a release 4.1.0 ever, for the same reason of 💬 Stable release imagemagick 4.1.0 Active .

Thanks!

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

mondrake → created an issue.

mondrake → created an issue.

mondrake → created an issue.

No - database drivers in sub-namespaces of Drupal\Driver\Database are long gone, and in fact those test cases should have been removed when the logic was. Better late than never.

Thanks

I wonder whether we should detect whether the rotate_ie plugin is available before swapping the operation.

Either way, we need a small test to prove this works and to prevent future regressions.

Committed to 5.0.x and cherry-picked to 4.0.x

Not cherry-picked to 4.1.x because there is not likely going to be a release 4.1.0 ever, for the same reason of 💬 Stable release imagemagick 4.1.0 Active .

Thanks!

I see the MR already targets 5.0.x, thanks

There are deprecation failures.

mondrake → created an issue.

PHPUnit 12 is out of the game for Drupal 11: #3546029-25: Ensure that #[RunTestsInSeparateProcesses] attribute is added to all Kernel tests →

@catch:

Just discussed this with @alexpott, @bircher and others at DrupalCon, and we think that we should only update to phpunit 12 in Drupal 12 - just because forcing every contrib module to update their tests in a minor release would be a massive break.

It would be good to do 📌 Remove support for PHPUnit 10 Active though, to avoid rerolls.

📌 Remove support for PHPUnit 10 Active must happen before committing this, but the MR can be reviewed now.

Green! The MR now includes 📌 Remove deprecated use of Assert::isType Active , 📌 Test cleanup for PHPUnit 12 - unit tests data providers RTBC .

Please target 5.0.x first.

Merge MR with MRs from 📌 Remove deprecated use of Assert::isType Active and 📌 Test cleanup for PHPUnit 12 - unit tests data providers RTBC

The MR is 1280+ commits behind head, needs a rebase.

Let's add couple more test cases.

Thanks! Applied suggestion.

Added a test for the PHPStan rule.

Needs tests, but I am opening up for review

Let's extend to Kernel tests data providers, too - now we have a clean report of the failures in 📌 Introduce support for PHPUnit 12 Postponed .

Just rebased to see if the new deprecation for Kernel tests missing the #[RunTestsInSeparateProcesses] attribute triggers as expected. 📌 Check that #[RunTestsInSeparateProcesses] attribute is added to all Kernel and Functional tests Needs review

Sorry for the noise.

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