Problem/Motivation

In drupalci.yml the value of testsgroups: can accept a comma-separated list of two (or more) test group names to match against the @group value in the test files. When using two values I discovered that some tests are not selected even though others with the same matching @group are selected. After more trial runs I found out that the order of specifying the two values makes a difference to the final list of tests that are selected. I also found out that removing a test from one of the groups allowed one of the non-found tests to be re-discovered and run. See the investigations on #3251399: Drupalci.yml filtering for testgroups: 'scheduler,scheduler_api' does not run all the required tests

Steps to reproduce

Run a test with two values for testgroups, for example in drupalci.yml have testgroups: 'group_a,group_b'.
Providing that you have at least one test in each @group the total number of tests that are run will be fewer than should be. Some with a matching @group are lost.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 3251817-9.patch905 bytesalexpott
#9 4-9-interdiff.txt905 bytesalexpott
#4 3251817-4.run-tests-with-multiple-groups.patch492 bytesjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented

My hunch was that there is a bug in how the selection of tests are collected and/or merged. I found out how to test this locally, by calling php core/scripts/run-tests.sh scheduler,scheduler_api in this example. Here is my output from run-tests.sh --list

scheduler
 - Drupal\Tests\scheduler\Functional\SchedulerAdminSettingsTest
 - Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest
scheduler_api
 - Drupal\Tests\scheduler\Functional\SchedulerEventsTest

I have cut down the number of tests locally, to make this example easier to read. There are two tests in @group scheduler (AdminSettings and BasicMedia) and 1 test in @group scheduler_api (Events). Each test class only has one @group, they are not in multiple groups. There is a third @group scheduler_rules_integration which we do not want to run, hence the reason for specifying the two groups in the drupalci.yml testgroups parameter.

jonathan1055’s picture

jonathan1055 commented

Adding debug to run-tests.sh function simpletest_script_get_test_list we see:

$args[test_names] = Array
(
    [0] => scheduler
    [1] => scheduler_api
)
$group_name=scheduler
loop: array_keys($groups[scheduler]) = Array
(
    [0] => Drupal\Tests\scheduler\Functional\SchedulerAdminSettingsTest
    [1] => Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest
)
loop: $test_list = Array
(
    [Drupal\Tests\scheduler\Functional\SchedulerAdminSettingsTest] => 0
    [Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest] => 1
)

After the second loop we have

$group_name=scheduler_api
loop: array_keys($groups[scheduler_api]) = Array
(
    [0] => Drupal\Tests\scheduler\Functional\SchedulerEventsTest
)
loop: $test_list = Array
(
    [Drupal\Tests\scheduler\Functional\SchedulerAdminSettingsTest] => 0
    [Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest] => 1
    [Drupal\Tests\scheduler\Functional\SchedulerEventsTest] => 0
)

Then the line array_flip($test_list) causes one of the tests to get dropped, because there are two with value 0

Original $test_list = Array
(
    [0] => Drupal\Tests\scheduler\Functional\SchedulerEventsTest
    [1] => Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest
)

However, simply changing to use array_keys($test_list) we keep all the tests

Correct $test_list = Array
(
    [0] => Drupal\Tests\scheduler\Functional\SchedulerAdminSettingsTest
    [1] => Drupal\Tests\scheduler\Functional\SchedulerBasicMediaTest
    [2] => Drupal\Tests\scheduler\Functional\SchedulerEventsTest
)
jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 3251817-4.run-tests-with-multiple-groups.patch492 bytes

Here's a patch that makes the single line change. I've searched for test coverage, and found plenty for the testDiscovery class, but could not find any tests that actually check the run-tests.sh script. If there are some tests, let me know where to find them, and I will add coverage for this bug if you'd like me to. Hopefully the description and debug output above is enough to see the problem and the correction.

jonathan1055’s picture

jonathan1055 commented

#3251399-17: Drupalci.yml filtering for testgroups: 'scheduler,scheduler_api' does not run all the required tests demonstrates that this patch fixes the problem. I have applied it via host_command. No tests are lost now - the previously missing ones are being included.

nicxvan’s picture

nicxvan commented
Project: DrupalCI: Test Runner » Drupal core
Version: » 9.5.x-dev
Component: Testrunner Codebase » phpunit
nicxvan’s picture

nicxvan commented

Moved to core queue per conversation on slack with longwave.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

The debug output in #3 is enough to convince me that the switch from array_flip() to array_keys() is the correct thing to do here. I don't think we have any test coverage of run-tests.sh itself.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 4-9-interdiff.txt905 bytes
new 3251817-9.patch905 bytes

I think we can make this code way easier to grok. Without some flipping and array_keys()

Here's a code way to test this locally...

php ./core/scripts/run-tests.sh --sqlite /tmp/coretest.sqlite --dburl sqlite://localhost/sites/default/files/db.sqlite --color --non-html --url http://YOUR_URL/  Annotation,Annotation,Assertion

This should run 12 tests. Currently it runs 11 because of the flipping and the way array_flip works with duplicate values of test_class => 0.

nicxvan’s picture

nicxvan commented

After reviewing the patch it looks right to me, the added array unique should help prevent extraneous test runs too.

First I pulled down the default drupal core branch 9.5.x then I ran the command you provided. I received an error about needing phpunit prophecy for the version of php unit.
I ran composer require --dev phpspec/prophecy-phpunit

I'm trying to test this and it looks like it is working with the two changes made.

This is my first time running Drupal core tests so let me know if something doesn't look right in my results.

UNPATCHED COMMAND RESULTS
11 Tests run


Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\Component\Assertion\InspectorTest
  - Drupal\Tests\Component\Annotation\AnnotatedClassDiscoveryTest
  - Drupal\Tests\Component\Annotation\AnnotationBaseTest
  - Drupal\Tests\Component\Annotation\DocParserIgnoredClassesTest
  - Drupal\Tests\Component\Annotation\Doctrine\DocParserTest
  - Drupal\Tests\Component\Annotation\MockFileFinderTest
  - Drupal\Tests\Component\Annotation\PluginIdTest
  - Drupal\Tests\Component\Annotation\PluginTest
  - Drupal\Tests\Component\Plugin\Discovery\AnnotatedClassDiscoveryTest
  - Drupal\Tests\Core\Annotation\PluralTranslationTest
  - Drupal\Tests\Core\Annotation\TranslationTest

Test run started:
  Friday, October 7, 2022 - 13:50

Test summary
------------

[32mDrupal\Tests\Component\Assertion\InspectorTest                32 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\AnnotatedClassDiscoveryTes   2 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\AnnotationBaseTest           3 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\DocParserIgnoredClassesTes   1 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\Doctrine\DocParserTest     227 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\MockFileFinderTest           1 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\PluginIdTest                 2 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\PluginTest                   6 passes                                      
[0m[32mDrupal\Tests\Component\Plugin\Discovery\AnnotatedClassDiscov  22 passes                                      
[0m[32mDrupal\Tests\Core\Annotation\PluralTranslationTest             5 passes                                      
[0m[32mDrupal\Tests\Core\Annotation\TranslationTest                   2 passes                                      
[0m
Test run duration: 7 sec

PATCHED COMMAND Results
12 tests run


Drupal test run
---------------

Tests to be run:
  - Drupal\Tests\Component\Annotation\AnnotatedClassDiscoveryCachedTest
  - Drupal\Tests\Component\Annotation\AnnotatedClassDiscoveryTest
  - Drupal\Tests\Component\Annotation\AnnotationBaseTest
  - Drupal\Tests\Component\Annotation\DocParserIgnoredClassesTest
  - Drupal\Tests\Component\Annotation\Doctrine\DocParserTest
  - Drupal\Tests\Component\Annotation\MockFileFinderTest
  - Drupal\Tests\Component\Annotation\PluginIdTest
  - Drupal\Tests\Component\Annotation\PluginTest
  - Drupal\Tests\Component\Plugin\Discovery\AnnotatedClassDiscoveryTest
  - Drupal\Tests\Core\Annotation\PluralTranslationTest
  - Drupal\Tests\Core\Annotation\TranslationTest
  - Drupal\Tests\Component\Assertion\InspectorTest

Test run started:
  Friday, October 7, 2022 - 13:51

Test summary
------------

[32mDrupal\Tests\Component\Annotation\AnnotatedClassDiscoveryCac   1 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\AnnotatedClassDiscoveryTes   2 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\AnnotationBaseTest           3 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\DocParserIgnoredClassesTes   1 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\Doctrine\DocParserTest     227 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\MockFileFinderTest           1 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\PluginIdTest                 2 passes                                      
[0m[32mDrupal\Tests\Component\Annotation\PluginTest                   6 passes                                      
[0m[32mDrupal\Tests\Component\Plugin\Discovery\AnnotatedClassDiscov  22 passes                                      
[0m[32mDrupal\Tests\Core\Annotation\PluralTranslationTest             5 passes                                      
[0m[32mDrupal\Tests\Core\Annotation\TranslationTest                   2 passes                                      
[0m[32mDrupal\Tests\Component\Assertion\InspectorTest                32 passes                                      
[0m
Test run duration: 7 sec

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Reviewed & tested by the community

Thanks for your work on this. Yes the new way of merging the classes is more readable that the flips.

I've just run a live test on #3049585-7: Check drupal.org testing infrastructure where I patched the project's drupalci.yml with patch #9 from above, instead of patch #4 that I had been using. The job correctly ran all the test classes in the requested groups, so it looks good. In agreement with @nicxvan so I'm marking this RTBC

  • catch committed 45461e7 on 10.0.x 
    Issue #3251817 by alexpott, jonathan1055, nicxvan, longwave: Make run-...
  • catch committed c7e78f4 on 10.1.x
    Issue #3251817 by alexpott, jonathan1055, nicxvan, longwave: Make run-...
  • catch committed b9a7acb on 9.4.x
    Issue #3251817 by alexpott, jonathan1055, nicxvan, longwave: Make run-...
  • catch committed 1da99ce on 9.5.x
    Issue #3251817 by alexpott, jonathan1055, nicxvan, longwave: Make run-...
catch’s picture

catch
he/him
Primary language English
commented
Version: 10.1.x-dev » 9.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked back through to 9.4.x, thanks!

jonathan1055’s picture

jonathan1055 commented

Thank you that's great. Also good that you backported to 9.5 and 9.4

jonathan1055’s picture

jonathan1055 commented

I have just tested Scheduler with 9.4.dev without the patch I had added in drupalci.yml and all the required test groups are being run, none dropped. Thanks for fixing this.

Status: Fixed » Closed (fixed)

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