🇮🇹Italy @mondrake

📌 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.

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

This is fixed, right?

Thanks for addressing my input.

#38 well TestTools is a plural already so I do not see a problem with Mocks?

rebased and updated baseline, back to RTBC

Bot????? It looks like the bot is posting based on old info

Well, bot...

Hey, bot...

Works nicely. ✨ Add an Autowire trait for plugins Needs review was just committed so let's wait a bit to see if it holds before committing this.

mondrake → created an issue.

I think #26 accounts for a release manager review. Fully agree, readjusting the deprecation message right now.

Totally missed the CR, sorry. Added a draft one.

📌 Ignore specifc sniffs when adding additional parameters to interface methods Active came up with a different format for the phpcs, @todo and @see dance. Attending the ball here.

#22 well, that's exactly what was discussed above

OK then, lets do that.

#17 actually, PHPUnit 12 will not throw anything... simply, it will not run the test in isolation without the attribute having been set.

That's why I am proposing to convert the deprecation triggered here in the setUp method into an exception - that would be Drupal's way of checking that the concrete test complies with the requirement. Alternatively, we could do that check in a static analysis rule, but that would not be guaranteed to be part of contrib checks: by adding it as an exception in the abstract base class there's no way around it.

So, I think we agree to start throwing the deprecation now. Question is when we start throwing the deprecation instead. Do I understand we can leave that open for now?

I.e. a deprecation message like 'Kernel tests must specify #[RunTestsInSeparateProcesses], not doing so is deprecated in drupal:11.3.0 and is removed when PHPUnit 12 is supported. See https://www.drupal.org/node/3548485' would be it?

I would suggest to move the Mocks sub-namespace under Drupal\TestTools, it eould be consistent with the rest and no need to register another namespace for testing

@dcam thanks for the review effort... really remarkable.

The reason why you see seemingly unrelated rearranging of use imports is due to the fact that the refactoring is done by rector, whereas the resorting is done by phpcbf on any file changed, independently from what the change was.

Will rebase now - tagged 'Avoid commit conflicts' exactly for the reason in #6.

Let's call this issue 'round 1'. A 'round 2' issue would increase type coverage level to max (I checked and that will add a lot more goodness, including argument typing based on inference). 'Round 3' or more would be a manual check/refactor of the leftovers at that stage.

Postponed on 📌 Enforce removal of PHPUnit annotations from test classes' class metadata Postponed .

mondrake → created an issue.

Thank you!

The annotation code is under phpcs ignore so that's why, I assume, phpcbf does not make fixes there.

It made a bunch of lowercase `null` insertions. Not sure what was up with that. I ended up grepping the diff file to find them all. There should be suggestions for all of them.

I really wish we had ✨ Stop using uppercase TRUE, FALSE, NULL literals Active . IMHO, spending time on this is waste of resources.

Inspired by this (thank you!), I have opened 📌 Add return types to core tests code via Rector Active to try tackling test code.

mondrake → created an issue.

not pushed

Thank you. I've added some inline comments in the rule code, to explain a bit what's going on. Leaving at RTBC as no code was changed.

rebased - any dependency update will cause merge errors here, unfortunately

Thanks!

Since we are at it, lets’s do #8.

If you are intersted, the MR in the parent is running tests with PHPUnit 12. It’s my reference to see reported results that lead to issues like this one.

RTBC, but HEAD testing is broken now

#5 they have their separate issues 📌 Remove class metadata from abstract test class ConfigEntityValidationTestBase Active and 📌 Remove class metadata from abstract test class UpdateSemverTestBase Fixed

Bump with increased constraint to prevent regression.

mondrake → created an issue.

mondrake → created an issue.

1. Removed from SettingTest some test cases that were still based on db connection logic that was removed in #3294695: Drupal 8 BC for database driver namespace fails for replicas → .
2. Made keys in array shapes have quotes so to improve readibility
3. Addressed @longwave's feedback

Unblocked

Unblocked

Thanks!

Addressed feedback in #19 and edited the draft CR, hopefully for the good.

Thanks a lot for the review. I'll be addressing the points in #19.

Anyway, let's postpone on 📌 Complete test annotations to attributes conversion for all remaining tests Postponed that is completing the transition from annotation to attributes, and is tagged for avoiding commit conflicts.

Ah right... would it make sense to add a conflict key instead, then?

I think we also need to update the constraint in core/composer.json to ^1.23.0 so to prevent regression