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
orCore
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
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff.txt | 1.4 KB | Mile23 |
#26 | 2878269_26.patch | 3.71 KB | Mile23 |
#22 | interdiff.txt | 1.19 KB | Mile23 |
#22 | 2878269_22.patch | 3.73 KB | Mile23 |
#18 | interdiff.txt | 1.04 KB | Mile23 |
Comments
Comment #2
Mile23Comment #3
Mile23TestDiscovery might move while this issue is happening: #2863055: Move TestDiscovery out of simpletest module, minimize dependencies
Comment #4
Mile23So 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 thoughComposerIntegrationTest
can. This means that--all
didn't discoverTestSuiteBaseTest
.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 runsTestSuiteBaseTest
.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 incore/tests/Drupal/Tests/Core
and../Component
. Then phpunit will skip the tests thatTestDiscovery
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 testgetPhpunitTestSuite()
. Making another issue and postponing on that.Comment #5
Mile23Let's fix the test first: #2893371: Several methods theoretically added to TestInfoParsingTest were actually not
Comment #6
Mile23#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 ofDrupal\Tests\Core
andDrupal\Tests\Component
by assuming that extension machine names are always lower-case.Comment #7
Mile23Comment #8
Mile23With 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.
Comment #9
Mile23Comment #12
Mile23Patch in #6 still applies.
Comment #13
Mile23Actual 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.
Comment #14
borisson_This looks good but I'm not confident enough about the test discovery system to rtbc this patch.
Comment #17
Mile23#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.Comment #18
Mile23Adds a unit test case.
Comment #19
Mile23There's a branch fail due to #3054649-17: DrupalKernelTest results in ERROR HANDLER CHANGED!, so there will be fails on
ClassLoaderTest
.Comment #21
Mile238.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).Comment #22
Mile23Marked failing tests incomplete so we can get a review.
Comment #23
LendudeIs 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?
These should probably not reference this issue but the issue where we will actually fix the test.
Comment #24
Mile23#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.
Comment #25
Lendude@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'Comment #26
Mile23Thanks.
Addressed #23-25.
Comment #27
LendudeAll feedback has been addressed, more tests are green then in HEAD, looks good
Comment #29
borisson_Back to rtbc, random fail.
Comment #30
Mile23This is currently blocking #2912387-90: Stop using wikimedia/composer-merge-plugin
Comment #32
catchCommitted ddd413c and pushed to 8.8.x. Thanks!
Comment #33
Mile23::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