Problem/Motivation
This issue supports #2807237: PHPUnit initiative
It's a child of #2863044: [plan] Remove simpletest dependencies from PHPUnit-based tests
We have a long-standing issue about removing the caching of results in TestDiscovery
: #1667822: Remove caching of test classes in TestDiscovery
We also have a need to remove TestDiscovery
from the simpletest module context. Currently it's a service defined by simpletest module, but is only used as a service by simpletest module's test list form, which we want to phase out. It's used as a class by behaviors we want to keep, such as TestSuiteBase
and run-tests.sh
.
TestDiscovery also declares a dependency on ModuleHandlerInterface
which is used to support hook_simpletest_alter()
. chx wants to remove that hook, do YOU? #2460521: Deprecate hook_simpletest_alter()
Proposed resolution
Turn TestDiscovery
into a lean, mean, test discovering machine, with few external dependencies.
Keep a deprecated semi-stub Drupal\simpletest\TestDiscovery
class which subclasses the new one. This allows BC for code which passes the dependencies. This simpletest subclass is also responsible for invoking simpletest module's hooks and performs caching for consistent behavior.
Have the new class not cache results beyond static, because developers are adding and moving tests all the time, and will need to invalidate the cache every time they do. The cache system is also a dependency which TestDiscovery
currently manages as a special case, so this amounts to removing that special case. Work underway here: #1667822: Remove caching of test classes in TestDiscovery
Move TestDiscovery
outside of the simpletest module context, so it's not dependent on the module. Also move MissingGroupException
which it depends on. Move them to core/tests
.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#79 | interdiff.txt | 2.17 KB | Mile23 |
#79 | 2863055_79.patch | 79.89 KB | Mile23 |
#77 | interdiff.txt | 21.34 KB | Mile23 |
#77 | 2863055_77.patch | 77.96 KB | Mile23 |
#75 | interdiff-2863055-72-75.txt | 470 bytes | martin107 |
Comments
Comment #2
Mile23This patch makes a stub
TestDiscovery
in simpletest which still does caching and calling hooks. The coreTestDiscovery
only finds tests.It also fixes
TestInfoParsingTest
, which erroneously has a test method on a stub class instead of the test class.This fails locally for a few tests, mostly because of race conditions having to do with timing out building the UI test list. That is: The main consumer of the
TestDiscovery
cached results is tests of the form that shows the tests.Let's see how the testbot fares.
Comment #3
Mile23Comment #4
klausiShouldn't you pass $test_list to the module handler, not $list?
trailing white space
Out of curiosity I checked for a test runtime regressions with an example:
../vendor/bin/phpunit --filter AccessManagerTest
That finishes in roughly 7 seconds before and after the patch, so I see no performance problem with this patch.
Keeping hook_simpletest_alter() in the simpletest specific TestDiscovery makes sense to me. I don't understand why we want to keep the cache in the Simpletest version and not just remove it completely. Because the Simpletest UI interacts with it and would be slow?
Comment #5
Mile23There's a little bit of problem with that, though, because the problem from #2460521: Deprecate hook_simpletest_alter() is that the hook isn't running from CLI (apparently). Part of the reason we'd be doing this refactor is so the CLI can avoid a dependency on ModuleHandler.
But we still need a way to satisfy the hook_simpletest_alter() use case, even if it's running from the CLI or other low-dependency environment. At some point, all the core simpletest will be converted, and database tests could say @requires postgre or whatever, but that still leaves contrib.
1) For the purposes of this issue, we still want the same behavior, just refactored. We can make decisions about the cache in #1667822: Remove caching of test classes in TestDiscovery
2) I initially left it out, but tests were failing locally. It was all the usual suspects, such as SimpleTestBrowserTest. These tests run testception within the UI, and were timing out. Adding the cache back helped them pass, because the margin was large enough that it mattered.
This patch fixes concerns from #4.
Comment #6
Mile23Forgot interdiff.
Comment #7
dawehnerJust some super quick comment.
Should we deprecate this exception?
When we deprecate something we should call out to
trigger_error()
now, see https://www.drupal.org/core/deprecationComment #8
Mile23Added trigger_error(), including an incomplete test to check whether the deprecation worked. See #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation
Moved and renamed some tests because we're testing the class, not one method.
Comment #10
Mile23I guess I *didn't* try and run that test locally. Thought I did.
Comment #11
Mile23Comment #12
Mile23Needed reroll.
Comment #13
dawehnerI really like how this patch allows us to move towards a more simple testing system, even on the longrun I hope we get rid of some of those aspects.
To be honest I'm wondering whether we can really get rid of caching ...
Comment #14
Mile23Thanks.
There's no use case where we need to discover tests more than once per request, other than when we test the UI form to make sure the form can discover tests and display them. And of course there's a use case where we *shouldn't* cache the list, because then the UI keeps the old discovered data even if the tests have changed.
Since it takes a really long time to generate the form *after* the files are discovered, it's possible to time out, depending on how fast your computer is. I did some timing research in #1667822-17: Remove caching of test classes in TestDiscovery
For now, we can leave it for consistency and make a determination in that other issue.
Comment #15
dawehnerThis is a good point!
Comment #16
Mile23Comment #17
alexpottWhy wouldn't we swap to using the non-deprecated code here?
Comment #18
Mile23@alexpott #17... Only simpletest module uses that service, and only uses it to build the testing UI form. See #14... We need the caching for the UI form at present, because otherwise tests of the UI form time out due to repeated renders.
We can remove usages of the deprecated
Drupal\simpletest\TestDiscovery
in a follow-up, which is normal deprecation anyway. Probably here: #1667822: Remove caching of test classes in TestDiscoveryComment #19
Mile23Patch still applies. Re-running test.
Comment #21
Mile23Fixing the failing test from #12 reveals a couple of interesting problems:
One is that it looks like kernel tests are swallowing the
E_USER_DEPRECATION
messages in some way, such that the thrown error remains but the message does not. I'm not sure where the problem might be for this.Another is that
DrupalStandardsListener
callsclass_exists()
for@coversDefaultClass
. This causes the deprecated class file to be loaded outside the context of a test, and@trigger_error()
gets called. The phpunit-bridge error handler is still around, so we get a deprecation error.The hacky-seeming solution is to add
@group legacy
to the listener method.Note that the test probably still won't pass. It just *should* pass. :-)
Comment #23
Mile23Added an issue about deprecations and
DrupalStandardsListener
: #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecationComment #24
Mile23Re-rolled after #2878248: DrupalStandardsListener improper handling of @trigger_error() deprecation, moved the failing test to be a unit test.
Comment #25
Mile23Patch still applies, re-running test.
Comment #26
Mile23Isolated issue just for the broken test: #2893371: Several methods theoretically added to TestInfoParsingTest were actually not
Comment #28
Mile23Comment #29
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #30
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #32
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #33
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedApplying patch #24 for version 8.4.x shows following error :
error: patch failed: core/modules/simpletest/src/TestDiscovery.php:309
error: core/modules/simpletest/src/TestDiscovery.php: patch does not apply
error: patch failed: core/modules/simpletest/src/TestDiscovery.php:74
error: core/modules/simpletest/src/TestDiscovery.php: patch does not apply
error: core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php: already exists in working directory
error: core/modules/simpletest/tests/src/Unit/TestInfoParsingTest.php: No such file or directory
error: patch failed: core/tests/TestSuites/TestSuiteBase.php:1
error: core/tests/TestSuites/TestSuiteBase.php: patch does not apply
Comment #34
martin107 CreditAttribution: martin107 as a volunteer commented@Mohit Malik, this issue has a tag "Needs reroll" which is a signal that the patch no longer applies.
Happily there are people in the community who act on this signal
Comment #35
Mile23This will need rerolls for #2893117: Improve HTML caching of Simpletest UI test form and/or #1667822: Remove caching of test classes in TestDiscovery
Those will decide what to do with
TestDiscovery
's use of cache services.I'm calling this one postponed on those, because we might not need to make a second
TestDiscovery
service class just for simpletest module.Comment #36
Mile23#2893117: Improve HTML caching of Simpletest UI test form is in, and it removes the persistent caching from
TestDiscovery
.This leaves us with one service dependency in
TestDiscovery
:ModuleHandler
. We only useModuleHandler
to firehook_simpletest_alter()
, which is basically unused at this point and will likely be deprecated in #2460521: Deprecate hook_simpletest_alter()Let's stay postponed here until #2460521: Deprecate hook_simpletest_alter() is implemented.
Comment #37
Mile23#2893117: Improve HTML caching of Simpletest UI test form was reverted, now it's RTBC again.
Comment #38
Mile23I made a plan issue with wider scope here: #2945465: Make TestDiscovery find info we need, not info we don't need
Comment #39
Mile23#2893117: Improve HTML caching of Simpletest UI test form is back in.
Reroll plus very minor changes.
Comment #41
Mile23Let's try that without a .DS_Store file.
Comment #43
Mile23Added CR.
Adding
@trigger_error()
toDrupal\simpletest\TestDiscovery
fails all tests which a) enable Simpletest module and b) use services, which is most functional tests. So I took it back out. We'll need a follow-up against D9 to add that (if at all).Comment #44
Mile23Patch still applies. Re-running tests.
Comment #47
Mile23Re-roll, setting to 8.7.x because this is a testing improvement.
Comment #48
andypostmakes sense to add theme engines, is there ability to configure it?
it could use core discovery (alterable) to allow other extension types, like db-drivers
Comment #49
andypostI mean that simpletest means that core can bootstrap and probably already yep
Comment #51
Mile23Fail on #47.... SOOOOOO many deprecations.
Important subsequent changes, documented here because your future git log might not show them to you: #2296615: Accurately support multiple @groups per test class #2973127: Exclude *TestBase.php, *Trait.php and *Interface.php files from test discovery reflection #2949363: Impossible to make trigger_error in some files without test fails
The interdiff here is a catch-up with those issues. Git apparently doesn't think this is a file move, so the rebase was a reversion.
Since the service container now automatically triggers deprecation errors for services marked @deprecated and that makes things complicated, let's move formal deprecation of 'test_discovery' to a follow-up.
#48:
TestDiscovery
shouldn't be configurable. If theme engines can be testable then their tests should be discovered. Also I'm not quite sure why db drivers require alteration of extension discovery in order to have their tests discovered. They should use@requires
and then we're done.It seems like both of these are out of scope here anyway, since this is mostly a 1:1 move out of simpletest module.
#49: Not sure exactly what you mean... Ideally, we don't want to bootstrap core in order to run tests.
Also I still see #3034015: Class to test deprecation error for deprecated trait triggers deprecation error itself running tests locally.
Comment #52
LendudeJust some nits I see
remove when we remove simpletest module I'd say, not deprecate.
8.4/6.x => 8.7.x
unrelated, this is hard enough to read, lets make it as small as possible.
Other then that, I went through the move of TestDiscovery with FileMerge and it is indeed only a move of old to new with two changes:
- the removal of moduleHandler
- the addition of alterHook to provide BC in the new version
Annoying indeed that git doesn't detect this as a move as it makes the diff hard to read. *shrug*
Comment #53
Mile23Thanks. Got the things.
Comment #54
Lendude@Mile23 thanks, looks good to me, and would remove another blocker for removing Simpletest.
Comment #55
larowlanComment #56
Mile23Rerolled.
Comment #57
LendudeThanks, looks good, back to RTBC.
Comment #58
alexpottI think this is really odd to include here. But I cant think of another way of doing this.
I'm not sure we can make these changes yet as this means the hooks aren't fired for run-tests.sh or for phpunit - and they currently are - right?
Comment #59
Mile23Re: #58: The hooks are only fired from the UI form handler. We have the
alterHook()
method only so that we can have BC forhook_simpletest_alter()
.#2460521: Deprecate hook_simpletest_alter() and #2234479: Deprecate hook_test_* hooks in simpletest have the details.
In the CR for deprecating
hook_simpletest_alter()
, we tell people to quit using simpletest in favor of phpunit and then add a test listener instead of implementing the hook: https://www.drupal.org/node/2939892Comment #60
Mile23Comment #61
alexpotthook_simpletest_alter() is triggered in run-tests.sh for sure.
Add
and use run-tests.sh to list all tests and you see
As far as I can see you're right that these hook are not fired for PHP unit so yeah the change to
TestSuiteBase
is fine.I think rather than adding an alterDefinitions method in the new core class we should override the whole TestDiscovery::getTestClasses() in \Drupal\simpletest\TestDiscovery that way the new core class never has any pretensions of alterability.
Comment #62
Mile23See the interdiff to see how old and crusty the #56 patch was. It didn't even account for #2949363: Impossible to make trigger_error in some files without test fails which is almost exactly a year ago.
Copypasting code back into
\Drupal\simpletest\TestDiscovery
for the special purpose of supporting a deprecated hook. Re-adding the tests for that code so we can maintain it. (I should put 'maintain' in air quotes.)Un-deprecating
\Drupal\simpletest\TestDiscovery
because we don't actually have a replacement until we decide whether the UI and its test runner are to be deprecated. #2750461: Remove Simpletest UI because we don't want to maintain a graphical test runner (Also because if it does@trigger_error()
then there are a bunch of deprecation errors that fail a bunch of simpletest-based tests, which we can't handle like we can with PHPUnit ones.)Added a
getTestDiscoveryMock()
method to the simpletest version ofTestDiscoveryTest
, in order to get around the problem of that test file causing a deprecation error outside of the test context. (It declared a subclass ofTestDiscovery
.) But now I've removed the deprecation, and I think it's a better test anyway. Now it's in bothTestDiscovery
tests.Also
TestRunnerKernel
no longer has a stealth dependency on simpletest module.Comment #63
Mile23CS.
Comment #64
Mile23Reroll and fixed some deprecated test assertions.
Comment #65
Mile23Woops.
Comment #66
LendudeFeedback has been addressed, fixed a minor nit, went through this again and this seems like a good BC solution. I'm not convinced we need all the BC here (who uses the simpletest hooks? seriously?), but this seems the safe way to do it.
Comment #68
LendudeAlso update the test...
Comment #69
Mile23Comment #70
martin107 CreditAttribution: martin107 as a volunteer commentedI am rerolling.
Comment #71
Mile23Some planning happening in this meta.
Comment #72
martin107 CreditAttribution: martin107 as a volunteer commentedResolving conflicts -- I blindly copied methods from
Drupal\simpletest\TestDiscovery
to
Drupal\Tests\Core\Test\TestDiscovery
I may have missed a subtle change so I will keep this assigned to myself until the test come back green. and the test count comeback unchanged.
Comment #73
martin107 CreditAttribution: martin107 as a volunteer commentedComment #74
martin107 CreditAttribution: martin107 as a volunteer commentedI think the reroll is ok.
Comment #75
martin107 CreditAttribution: martin107 as a volunteer commentedSpoke too soon just cleaning up a coding standard nit ... I introduced. Sorry for all the noise.
Comment #76
Mile23LGTM but I can't RTBC.
Re @Lendude #66: We already got this one: #2460521: Deprecate hook_simpletest_alter() and this one remains: #2234479: Deprecate hook_test_* hooks in simpletest
I think we can formally deprecate here with @trigger_error(), given that we're probably going to move simpletest module to contrib.
Of course we don't yet have an issue to link to for that.
Comment #77
Mile23Adds
@trigger_error()
to\Drupal\simpletest\TestDiscovery
, which means marking a few tests@group legacy
.The thinking here is this: We'll likely move the simpletest module to contrib. Therefore, people need to quit using
Drupal\simpletest\TestDiscovery
. Eventually it will go away from core, and in contrib it can be un-deprecated as needed.Remove a bunch of duplicate testing from core/modules/simpletest/tests/src/Unit/TestDiscoveryTest.php. We leave behind the tests of
Drupal\simpletest\TestDiscovery::getTestClasses()
which is really all that's left in that version ofTestDiscovery
.Comment #79
Mile23Added
@group legacy
to some simpletest tests, and made that work inWebTestBase
, thanks to #2296615: Accurately support multiple @groups per test classComment #80
LendudeThe reasoning in #77 makes sense to me, so +1
Went through this again, not an easy diff to read, with all the stuff being moved around, but I didn't spot anything getting lost.
If it turns out we do need to move more stuff it could probably be done in a follow up to minimise scope here
Comment #82
larowlanCommitted 590ca4a and pushed to 8.8.x. Thanks!
Comment #84
Mile23Fan freaking tastic.