Accurately support multiple @groups per test class

LocaleTranslateStringTourTest changed @group from Tour to locale in #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)

so when we can have two, it should probably have both.

This is a bug because you're trying to run tests that belong to an annotated @group, but you won't get all the tests in that group.

This is an 8.3.x bug because it improves the testing system, seems like it'd be allowed under alpha.

This resurfaces an older bug, because I had to run the tests under PHPUnit's parallel discovery mechanism in order to discover whether the change breaks test discovery. #2722731: Mark MissingDependentModuleUnitTest as incomplete

Moved the UI portion of this issue to a follow-up: #2858652: Support multiple @group test annotations in Simpletest UI

This allows a pure test system change for 8.3.x with UI changes moved to 8.4.x.

Updated issue summary.

Patch still applies. Re-running test.

Patch still applies. Re-running test.

Patch still applies. re-running test.

Needed a re-do after #2893371: Several methods theoretically added to TestInfoParsingTest were actually not so I guess that shows me for finding other bugs. :-)

I like us being able to finally do that! Well, technically we are able already for phpunit based tests, so this just syncs the capability.

🤔 I'm a bit confused as I don't see an actual annotation test class with multiple @group or a @groups statement. Maybe its enough to write an example in a change record?

Note: I'm using emoji based code review, see https://gist.github.com/pfleidi/4422a5cac5b04550f714f1f886d2feea

The emoji just shows up as a little square to me.

There's no such annotation as @groups. We use multiple @group annotations.

I'm adding a 'groups' array key to the test info so that we can keep BC with 'group' for Simpletest UI.

OK, so I added some test cases to TestDiscoveryTest: One unit test of getTestInfo() with more @groups than anyone will probably ever use, and some for getTestClasses() that prove that we're adding more groups during a file system scan.

Needed a re-roll.

This is still a bug, since it allows for false passes when you use run-tests.sh and specify a group. So it's still targeted to 8.4.x, but I'd understand if others don't see it that way. Also it's not an API change.

This is still a bug, since it allows for false passes when you use run-tests.sh and specify a group. So it's still targeted to 8.4.x, but I'd understand if others don't see it that way. Also it's not an API change.

For me at least arguing for the current minor always adds effort, from individual contributors, subsystem maintainers and finally from core committers. I think from my point of view this absolutely nice developer experience improvement.

Well, it's a bug and it might be causing false positives on test runs, which is why I aimed it at 8.4.x at the time.

Rerunning the test from #19.

1. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -156,9 +156,6 @@ public function registerTestNamespaces() {
   -    $reader = new SimpleAnnotationReader();
   -    $reader->addNamespace('Drupal\\simpletest\\Annotation');
   -
   

   +1 Nice catch that this is totally unused.

2. +++ b/core/modules/simpletest/src/TestDiscovery.php
   @@ -344,9 +345,14 @@ public static function getTestInfo($classname, $doc_comment = NULL) {
   +        if ($annotation == 'group') {
   
   +++ b/core/scripts/run-tests.sh
   @@ -81,10 +81,24 @@
   +      if (!in_array($class, $classes)) {
   

   Should we use a "===" here / in_array(, , TRUE)

Addressed #22.

+1. :-)

Thank you @jibran!

Unrelated fail, setting back to RTBC.

+++ b/core/scripts/run-tests.sh
@@ -81,10 +81,24 @@
+  foreach($groups as $group => $tests) {

nit: missing a space here, fails phpcs

Rerolled the test and fixed the #29

Testbot hickup

I'd RTBC but I wrote the original patch.

Since this was already RTBC before and #31 fixes the only minor complaint raised I think this can be RTBC again. Also read through the patch and couldn't find anything to complain about, but this is not really my area of expertise.

+++ b/core/scripts/run-tests.sh
@@ -81,10 +81,24 @@
+
+  // A given class can appear in multiple groups. For historical reasons, we
+  // need to present each test only once, under its first group.
+  $classes = [];
+  $list = [];
   foreach ($groups as $group => $tests) {
+    foreach (array_keys($tests) as $class) {
+      if (!in_array($class, $classes, TRUE)) {
+        $list[$group][] = $class;
+        $classes[] = $class;
+      }
+    }
+  }
+
+  foreach ($list as $group => $tests) {
     echo $group . "\n";
-    foreach ($tests as $class => $info) {
-      echo " - $class\n";
+    foreach ($tests as $test) {
+      echo " - $test\n";
     }
   }

This can be one loop and not two. Ie:

  // A given class can appear in multiple groups. For historical reasons, we
  // need to present each test only once, under its first group.
  $printed_tests = [];
  foreach ($groups as $group => $tests) {
    echo $group . "\n";
    $tests = array_diff(array_keys($tests), $printed_tests);
    foreach ($tests as $test) {
      echo " - $test\n";
    }
    $printed_tests = array_merge($printed_tests, $tests);
  }
  exit(SIMPLETEST_SCRIPT_EXIT_SUCCESS);

+++ b/core/scripts/run-tests.sh
@@ -81,10 +81,24 @@
+  // A given class can appear in multiple groups. For historical reasons, we
+  // need to present each test only once, under its first group.

Also this isn't quite what is happening here. If a class has the annotation:

 * @group search
 * @group config

and another class has

 * @group config
 * @group search

they will both be in the config group because the config group is printed first. It's not it's first group.

Fixes #36 & #37.

they will both be in the config group because the config group is printed first. It's not it's first group.

After this issue is fixed, the only (core) reason we need to present that historical list is so that the testbot can have a list of test classes to use for converting {simpletest} entries into JUnit. It discards the group info.

Eventually it won't be doing this for PHPUnit: #2943340: Process PHPUnit junit reports separately

Anyway: Better code, yay! More +1 from me, except I can't RTBC.

I have one small question: When you do something like: run-tests.sh node entity, potentially the same test might be executed multiple times, wouldn't it?
Maybe we need the same logic in the execution as we need when we list the available tests.

That is a bit of an oversight, isn't it?

Note that we have to write code like this:

      // Ensure our list of tests contains only one entry for each test.
      foreach ($args['test_names'] as $group_name) {
        $test_list = array_merge($test_list, array_flip(array_keys($groups[$group_name])));
      }
      $test_list = array_flip($test_list);

...because TestDiscovery is discovering tests *for the Simpletest UI.* #2945465: Make TestDiscovery find info we need, not info we don't need

Thank you for fixing that @Mile23!

+++ b/core/scripts/run-tests.sh
@@ -1148,16 +1148,22 @@ function simpletest_script_get_test_list() {
+      if (!empty($unknown_groups = array_diff($args['test_names'], $all_groups))) {
+        $first_group = reset($unknown_groups);
+        simpletest_script_print_error('Test group not found: ' . $first_group);
+        simpletest_script_print_alternatives($first_group, $all_groups);
+        exit(SIMPLETEST_SCRIPT_EXIT_FAILURE);
+      }

Nice find!

+++ b/core/scripts/run-tests.sh
@@ -1141,16 +1148,22 @@ function simpletest_script_get_test_list() {
+      // Remove groups the user doesn't want.
+      $groups = array_intersect_key($groups, array_flip($args['test_names']));

We don't really need this here, do we? Nope!

+++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -317,6 +318,8 @@ public static function scanDirectory($namespace_prefix, $path) {
    *   - group: The test's first @group (parsed from PHPDoc annotations).
+   *   - groups: All of the test's @group annotations, as an array (parsed from
+   *     PHPDoc annotations).

This is really confusing, you can always get first element of array so "for historical reasons" could be replaced with proper implementation - "provider" as all discovery stuff set and groups!

Re: #46: That's true. But we can't deprecate an array key, we have to keep 'group.' And we can't change the expectation for 'group' to be an array instead a string. It'd be an API break. So we're better off providing both and documenting why.

If we're getting rid of the UI form, we can get rid of most of this code anyway: #2945465: Make TestDiscovery find info we need, not info we don't need

Here's a re-roll. Also, I encountered #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself along the way.

Dialing this in to 8.7.x because it's a test improvement.

+++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
@@ -15,10 +15,6 @@
- *
- * Since TestDiscovery is expected to discover Simpletest-based tests, it will
- * likely trigger deprecation errors. Therefore, we add the legacy group.
- * @group legacy

This should only really be added back for issues like #2847678: Deprecate WebTestBase and its required classes in favour of BrowserTestBase when/if we end up calling @trigger_error() in those classes.

+++ b/core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php
@@ -15,10 +15,6 @@
- *
- * Since TestDiscovery is expected to discover Simpletest-based tests, it will
- * likely trigger deprecation errors. Therefore, we add the legacy group.
- * @group legacy

@@ -77,6 +76,7 @@ public function infoParserProvider() {
         'type' => 'PHPUnit-Kernel',

While I agree that could be removed for now, it feels a little out of scope here? But other then that, I see no problems here anymore, all feedback has been addressed.

Crediting dawehner, andypost and myself for code review.

Committed and pushed 2af907ca36 to 8.8.x and 9e8d9b73dd to 8.7.x. Thanks!

I tested this locally and seen that run-tests.sh can discover tests in multiple groups and picks then when limited to any group the test is in.

Cool, thanks. Now we've unblocked #2858652: Support multiple @group test annotations in Simpletest UI and #2021077: make search also match the test groups on the test overview page

Crap, this is causing all tests with multiple groups to be run multiple times :(

Search for Drupal\Tests\user\Kernel\UserServiceProviderTest in the test log

this might fix it....

@Lendude +1 to going forward - let's open a followup and go forwards if that works.

I can confirm that #56 fixes the problem.

Here is the follow up #3039572: Prevent tests with multiple groups running multiple times