Problem/Motivation

Discovered in #2878248-7: DrupalStandardsListener improper handling of @trigger_error() deprecation

The testbot does not run all the tests.

TestDiscovery::getPhpunitTestSuite() assigns tests to different test suites based on a regex.

The regex is built with the assumption that all core unit-suite tests live in either core/tests/Drupal/Tests/Core or Component.

There are other unit-suite tests in core outside of those two directories, such as Drupal\Tests\ComposerIntegrationTest.

Because it doesn't consult TestDiscovery::getPhpunitTestSuite(), run-tests.sh --list finds all the tests. run-tests.sh --types PHPUnit-Unit --all only finds the ones in Core or Component because it does consult TestDiscovery::getPhpunitTestSuite().

This means that the tests outside Core or Component are not run during the testbot build time.

So there are currently two edge cases:

  • Drupal\Tests\Scripts\TestSiteApplicationTest
  • Drupal\Tests\ComposerIntegrationTest

TestSiteApplicationTest does not run. ComposerIntegrationTest does. See the log file.

Both are discovered by run-tests.sh --list.

The reason is that TestDiscovery::getPhpunitTestSuite() has a buggy regex that figures out that ComposerIntegrationTest belongs to suite Unit, but does not figure out a test suite for TestSiteApplicationTest. And if no suite is discovered for the test, it is not run by run-tests.sh.

There are two solutions:

  • Somehow ensure that all tests are placed under either Component or Core directories.
  • Allow unit test discovery to occur under core/tests/Drupal/Tests, and fix/simplify the regex.

We're clearly adding tests that end up not being run, so let's just be permissive with where tests can live.

Proposed resolution

Issues like #2935157: Run-test.sh does not run TestSuite tests. essentially happen by accident, so we can't know whether we're running all our tests.

Given that that's the case, modify TestDiscovery::getPhpunitTestSuite() to properly identify all tests in core/tests/Drupal/Tests/ as unit tests, without concern for whether they're in Core or Component subdirectories.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#26 interdiff.txt1.4 KBMile23
#26 2878269_26.patch3.71 KBMile23
#22 interdiff.txt1.19 KBMile23
#22 2878269_22.patch3.73 KBMile23
#18 interdiff.txt1.04 KBMile23
#18 2878269_18.patch2.54 KBMile23
#6 2878269_6.patch1.5 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes
Related issues: +#2863055: Move TestDiscovery out of simpletest module, minimize dependencies

TestDiscovery might move while this issue is happening: #2863055: Move TestDiscovery out of simpletest module, minimize dependencies

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.3.x-dev » 8.4.x-dev
Related issues: +#2659100: Allow run-tests.sh to run just the javascript Functional tests

So here's a nightly core testbot build: https://dispatcher.drupalci.org/job/php5.6_mysql5.5/3035/consoleFull

You'll discover that TestSuiteBaseTest can't be found in it, even though ComposerIntegrationTest can. This means that --all didn't discover TestSuiteBaseTest.

Now we look at the testgroups.txt artifact: https://dispatcher.drupalci.org/job/php5.6_mysql5.5/3035/artifact/jenkin...

It tells us that both tests were discovered by --list.

If we execute ./vendor/bin/phpunit -c core/ --testsuite unit --filter TestSuiteBaseTest we see that phpunit discovers and runs TestSuiteBaseTest.

We could solve this by modifying the unit test test suite discovery to be similar to TestDiscovery. For unit tests this looks easy: We only add test files in core/tests/Drupal/Tests/Core and ../Component. Then phpunit will skip the tests that TestDiscovery also skips.

However, what happens when we address kernel tests? Do we also restrict to core/tests/Drupal/Tests/KernelTests/Core and ../Component? There are quite a few kernel tests outside of those two directories. For instance, Drupal\KernelTests\Config\DefaultConfigTest lives outside /Core.

So should we move all tests to be in Core or Component under proper namespaces? It's probably a good idea to keep things in the right place, but it also means that, in the case of TestSuite, we'd have to fudge and say that it's under Core, which it isn't.

Solution: Don't limit to Core or Component directories, and make a follow-up to move tests around to reflect the namespace of the thing they're testing. We could even use the test listener to validate @coversDefaultClass against the namespace of the test class. Way out of scope here though.

Also, #2659100: Allow run-tests.sh to run just the javascript Functional tests broke TestInfoParsingTest, which doesn't actually test getPhpunitTestSuite(). Making another issue and postponing on that.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Postponed

Let's fix the test first: #2893371: Several methods theoretically added to TestInfoParsingTest were actually not

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Postponed » Needs review
Related issues: +#2893371: Several methods theoretically added to TestInfoParsingTest were actually not
FileSize
2878269_6.patch1.5 KB

#2893371: Several methods theoretically added to TestInfoParsingTest were actually not is in, so we can proceed here. Moving to 8.3.x because it's a current bug.

This patch changes TestDiscovery::getPhpunitTestSuite() to allow for unit tests outside of Drupal\Tests\Core and Drupal\Tests\Component by assuming that extension machine names are always lower-case.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Parent issue: » #2607866: [meta] DrupalCI/SimpletestUI/run-tests.sh/phpunit not in harmony
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: TestDiscovery::getPhpunitTestSuite() allows different tests than phpunit --testsuite unit » Modify TestDiscovery so the testbot runs all the tests

With the patch in #6, 21,707 passes. The most recent branch build: 21,704 passes. So that accounts for the tests which have not been running. See #4.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.5.x-dev » 8.6.x-dev

Patch in #6 still applies.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Actual false positive in the wild:

Fail in #6 under 8.6.x is the same fail as #2962157-4: TestSiteApplicationTest requires a database despite being a unit test revealed here because the test is buggy. That's the false positive.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

This looks good but I'm not confident enough about the test discovery system to rtbc this patch.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.8.x-dev » 8.7.x-dev
Issue summary: View changes

#2935157: Run-test.sh does not run TestSuite tests. moved TestSuiteBaseTest so the test runner runs it, without fixing the problem for this issue.

So there are currently two edge cases:

  • Drupal\Tests\Scripts\TestSiteApplicationTest
  • Drupal\Tests\ComposerIntegrationTest

TestSiteApplicationTest doesn't get run because the regex fails to see it as a unit test.

Updated IS to reflect this.

Patch in #6 still applies, and TestSiteApplicationTest has technical debt fails.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
2878269_18.patch2.54 KB
interdiff.txt1.04 KB

Adds a unit test case.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

There's a branch fail due to #3054649-17: DrupalKernelTest results in ERROR HANDLER CHANGED!, so there will be fails on ClassLoaderTest.

Status: Needs review » Needs work

The last submitted patch, 18: 2878269_18.patch, failed testing. View results

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes

8.7.x branch issues have been fixed, so I'll re-run the tests. This leaves us with a broken TestSiteApplicationTest (because the testbot never runs it unless we fix that).

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.7.x-dev » 8.8.x-dev
Status: Needs work » Needs review
FileSize
2878269_22.patch3.73 KB
interdiff.txt1.19 KB

Marked failing tests incomplete so we can get a review.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented 
  1. +++ b/core/modules/simpletest/src/TestDiscovery.php
@@ -464,15 +464,13 @@ public static function parseTestClassAnnotations(\ReflectionClass $class) {
+      // the extension name. We assume that lower-case strings are module names.

    Is this a safe assumption to be made? Is that assumption documented anywhere other than this line buried deep in TestDiscovery? If not, should we document it somewhere? 

  2. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
@@ -85,6 +85,7 @@ public function testInstallWithFileWithNoClass() {
+    $this->markTestIncomplete('Fix this test after https://www.drupal.org/project/drupal/issues/2878269 is in.');

@@ -254,6 +255,7 @@ public function testTearDownDbPrefixValidation() {
+    $this->markTestIncomplete('Fix this test after https://www.drupal.org/project/drupal/issues/2878269 is in.');

    These should probably not reference this issue but the issue where we will actually fix the test.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

#23.1: We document that your contrib module test must have the module machine name in it: https://www.drupal.org/docs/8/phpunit/phpunit-file-structure-namespace-a...

We also document how modules must use their machine name in their namespace: https://www.drupal.org/docs/develop/coding-standards/namespaces#modules

Otherwise all is lost and we don't have a way to tell \Drupal\Test\ apart from \Drupal\test\.

#23.2: I'm reluctant to file that issue because someone will suggest that we just move the test rather than actually fixing our testing system.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Needs review » Needs work

@Mile23 thanks for the feedback in #24.

The fix for #23.2 is I think #2962157: TestSiteApplicationTest requires a database despite being a unit test, which is indeed moving the test, but I agree the right fix would be to land this first and not move it in that other issue.

Doing this issue first would at least add the test coverage provided by the other methods, so I don't think we should postpone on #2962157: TestSiteApplicationTest requires a database despite being a unit test as that doesn't seem like a trivial fix.

Setting to 'needs work' to reference #2962157 in the markTestIncomplete() calls which would at least give it an actionable @todo instead of a rather vague 'we need to fix this at some point'

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2878269_26.patch3.71 KB
interdiff.txt1.4 KB

Thanks.

Addressed #23-25.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Needs review » Reviewed & tested by the community

All feedback has been addressed, more tests are green then in HEAD, looks good

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2878269_26.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Back to rtbc, random fail.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

This is currently blocking #2912387-90: Stop using wikimedia/composer-merge-plugin

  • catch committed ddd413c on 8.8.x 
    Issue #2878269 by Mile23, Lendude: Modify TestDiscovery so the testbot...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed ddd413c and pushed to 8.8.x. Thanks!

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

::does a little dance::

This does have a follow-up, so that we're testing the test app that we use to do nightwatch testing. It's a testing trifecta: #2962157: TestSiteApplicationTest requires a database despite being a unit test

Status: Fixed » Closed (fixed)

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