Fix test classes with no *Test suffix

EntityQueryServiceDeprecation

is actually not running as a test in HEAD currently.

I came to this issue with "How is this major?" but #4 makes a case.

I agree we need to fix these independent from the other issue and with at least one test not actually running this seems like it's at least major.

I would've expected to only see file and class name changes, but there's a couple of extra things as well, mostly removing phpunit annotations and adding new attributes instead. Is this part of the transition to phpunit 11 and were these files missed because they weren't discovered or why are we including these changes?

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

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.

I agree that there's a little scope blur with the annotations, but the issue is small enough and it's enough of a gray area that I wouldn't knock it back from RTBC specifically for that. I agree that it should be a standard practice going forward but does that have its own meta issue? It should if not.

Also it looks like some docblock annotations are being removed without it being moved to the attributes? For the image tests.

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

@mondrake OK that's definitely out of scope and confusing to the reviewer; can we do it in a followup? Unless it causes a static analysis fail or something.

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.

@mondrake, OK, I am compelled by your argument: we need to update them because they are being made abstract when they were not previously. We will still want a followup though for other places it may be wrong in core. :)

I reviewed the MR and checked that every class that has been made abstract has at least one concrete implementation. I also checked that all the removed annotations from base classes existed on all of the child classes. One unfortunate side effect is that if you were to implement a new child class it wouldn't be as obvious which annotations you might want to use, whereas previously you might be inclined to copy/paste from the parent class.

Looking at the base classes in question, nearly all those dodgy phpunit annotations were introduced by me, something like:

Split a long-running tests into 2 or 3 classes by adding a base class and two tests extending from it - when copying methods to the base class, failed to remove the phpunit annotations.

So theoretically there are not very many of these at all in core since that's only happened a few times at most. Opened 📌 Ensure base test classes don't have phpunit annotations Active anyway.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → closed merge request !12378

Automatically closed - issue fixed for 2 weeks with no activity.