Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The group annotation in the ExamplesBrowserTestBase
is missing and yesterday I noticed to the testbot required it even for the base test classes (see #2618192: Increase basic test coverage)
I'm going to attach a patch adding it in my next comment.
Thanks.
Comment | File | Size | Author |
---|---|---|---|
#7 | 2853581-7.patch | 1.35 KB | gnuget |
| |||
#7 | 2853581-interdiff-5-7.txt | 384 bytes | gnuget |
#5 | 2853581-5.patch | 1.72 KB | gnuget |
| |||
#2 | 2853581-group-1.patch | 503 bytes | gnuget |
|
Comments
Comment #2
gnugetComment #3
gnugetAfter to think about this, I just move this to "major" when the group is missing in the base class the phpunit doesn't die, it just ignore the tests using this class, this will provoke to some tests be ignored.
Comment #4
Mile23So if we need the @group annotation for the base class, why is the number of tests run here the same as the branch? https://www.drupal.org/node/594964/qa
The testbot uses --directory to specify where to look for tests, so that might be why. (--directory ignores groups entirely.) There's also the fact that some @group annotations are ignored by TestDiscovery which might be adding to the confusion. #2297541: [policy, no patch] Secondary/additional test @group names
I'd rather not annotate the base class as belonging to a group, because we get this:
TestDiscovery shouldn't find the base class in the first place, since its abstract, so that's a bug in core.
Also, after the patch the only difference is that if you specify the group to the test runner, it finds and tries to run the base class. Thankfully the part of the runner that actually does the running figures out that it's an abstract class and doesn't do the work.
So what needs to happen here is that we make sure we specify @group annotations for all test classes, and that core gets it together to discover more than one group per test.
Updating this issue in core: #2296615: Accurately support multiple @groups per test class
Comment #5
gnugetHi!
Thanks for your review.
I was wrong about why my tests were ignored thanks for the explanation, I will remove the group from my base class on #2618192: Increase basic test coverage.
As you suggested I just reviewed all the test classes and all have the group annotations I just added the group "examples" in those where it was missing.
(I don't add an interdiff for this patch because it is something completely different from #2, so we can just ignore #2)
Thanks for your help again.
Comment #6
Mile23Thanks. I meant more to ensure that all tests have at least one @group annotation, which I guess they do. It'd be nice to have them all belong to @group examples, though, so we'll add that.
This isn't a test.
Comment #7
gnugetYes, I checked all the tests and all of them have at least one @group annotation and most of the have the @group examples
I got confused by
ProtectedPrivates.php
because it has a "ingroup" annotation :-p.New patch removing the changes from
ProtectedPrivates.php
Comment #9
Mile23Verified that all the tests have @group examples in them.
Committed and pushed. Thanks for working on this.