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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gnuget created an issue. See original summary.

gnuget’s picture

Status: Active » Needs review
FileSize
503 bytes
gnuget’s picture

Priority: Normal » Major

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

Mile23’s picture

Status: Needs review » Needs work

So 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:

$ php ./core/scripts/run-tests.sh examples

Tests to be run:
  - Drupal\examples\Tests\ExamplesTest
  - Drupal\Tests\events_example\Functional\EventsExampleTest
  - Drupal\Tests\events_example\Kernel\EventsExampleServiceTest
  - Drupal\Tests\examples\Functional\ExamplesBrowserTestBase
[..]

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

gnuget’s picture

Title: missing group annotation on ExamplesBrowserTestBase » make sure we specify @group annotations for all test classes
Priority: Major » Normal
Status: Needs work » Needs review
FileSize
1.72 KB

Hi!

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.

Mile23’s picture

Category: Bug report » Task
Status: Needs review » Needs work

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

+++ b/phpunit_example/src/ProtectedPrivates.php
@@ -14,6 +14,9 @@
+ *
+ * @group phpunit_example
+ * @group examples

This isn't a test.

gnuget’s picture

Status: Needs work » Needs review
FileSize
384 bytes
1.35 KB

Yes, 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

  • Mile23 committed c128db7 on 8.x-1.x authored by gnuget
    Issue #2853581 by gnuget: make sure we specify @group annotations for...
Mile23’s picture

Status: Needs review » Fixed

Verified that all the tests have @group examples in them.

Committed and pushed. Thanks for working on this.

Status: Fixed » Closed (fixed)

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