Problem/Motivation
As of Drupal 8.4.0, Drupal\system\Tests\Update\UpdatePathTestBase
is @deprecated
.
It's deprecated with a @trigger_error()
before the class is loaded, so it fails any phpunit test run of one of our process-isolated tests, such as BrowserTestBase
. Note that it's a base class for WebTestBase
tests, which means all conversions around it will be process-isolated BrowserTestBase
tests.
It's actually used by two BrowserTestBase
tests: Drupal\Tests\hal\Functional\Update\CreateHalSettingsForLinkDomainUpdateTest
and Drupal\Tests\hal\Functional\Update\MigrateLinkDomainSettingFromRestToHalUpdateTest
It's also used by 42 WebTestBase
-based tests. We aren't concerned about those at the moment because Simpletest doesn't do anything with @trigger_error()
. We will have to concern ourselves with those other tests when they are converted to BrowserTestBase
tests as part of #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) The same error being made for these hal tests will probably be made for the other ones as they're converted.
Here's the bad part:
If you try to run CreateHalSettingsForLinkDomainUpdateTest
using phpunit, you get a failure to run the test:
$ ./vendor/bin/phpunit -c core/ --testsuite functional --filter CreateHalSettingsForLinkDomainUpdateTest
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.
Time: 7.89 seconds, Memory: 40.00MB
No tests executed!
Remaining deprecation notices (1)
Drupal\system\Tests\Update\UpdatePathTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Use \Drupal\FunctionalTests\Update\UpdatePathTestBase instead. See https://www.drupal.org/node/2896640: 1x
1x in FunctionalTestSuite::suite from Drupal\Tests\TestSuites
$ echo $?
1
Our deprecation of Drupal\system\Tests\Update\UpdatePathTestBase
fails the test because it performs @trigger_error()
before any of the test has run.
This happens during discovery, which is why you can see symfony/phpunit-bridge telling us the error was triggered during TestSuites
. (PHPUnit discovery uses reflection, so it loads the class.)
This is good in the sense that we find the deprecation when we run phpunit. But it's bad because run-tests.sh
does not fail this test. run-tests.sh
does not use reflection during discovery, so it doesn't load the class. And since the test is run in an isolated process, the PHPUnit child never gets back a report about triggered errors:
$ php ./core/scripts/run-tests.sh --class 'Drupal\Tests\hal\Functional\Update\CreateHalSettingsForLinkDomainUpdateTest'
Drupal test run
---------------
Tests to be run:
- Drupal\Tests\hal\Functional\Update\CreateHalSettingsForLinkDomainUpdateTest
Test run started:
Tuesday, August 29, 2017 - 23:03
Test summary
------------
Drupal\Tests\hal\Functional\Update\CreateHalSettingsForLinkD 228 passes
Test run duration: 2 min 43 sec
Proposed resolution
1) Move the individual tests in question to the new Drupal\FunctionalTests\Update\UpdatePathTestBase
.
2) As a best practice, run all process-isolated tests using the phpunit tool during conversion, in order to catch these types of deprecations.
3) Work on making symfony/phpunit-bridge work with process-isolated tests. #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#4 | 2905470-4.patch | 1.34 KB | Lendude |
Comments
Comment #2
dawehnerGood ideas!
I like using 2) I guess we want to at least have one successful run using phpunit directly in the issue summary.
Do you want to fix 1) here?
Comment #3
Mile23Comment #4
LendudeHere is the patch to update the HAL tests.
What would this look like in practice? This sounds like it can take a while running it on a local machine.
What I try to do usually is just check if there are any classes extending the deprecated class and are in the wrong dir, something you can easily do in PHPStorm. But since this is a manual thing I also tend to forget this (as the Update conversion shows), so having a standard way of going about this sounds like a good idea.
Comment #5
Mile23Basically run functional tests in PHPUnit. So for the system module:
You're right it could take a while, which is why we should fix it in
run-tests.sh
. Like this: #2906009: To avoid false passes, run-tests.sh should force phpunit to discoverComment #6
LendudeThe problem I see with running that command is that you have no idea which group you want.
For example the two tests we are discussing here are marked as
@group hal
, there is no way I would have run that group after doing the update conversions, your example ofgroup system
sounds more likely, so then we still wouldn't have caught the missed classes. (Same would have been true when updating the Views module, the Views tests in the node module are@group node
).So the only way to be sure would be to run the whole test suite locally with phpunit, which, lets be honest, is not gonna happen, so hooray for #2906009: To avoid false passes, run-tests.sh should force phpunit to discover.
I think I'll update the steps in #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) to include checking if all test classes still using a deprecated base class are in the correct dir/namespace, that sounds like a best practice step we can do until we can automate it.
Make sense, or talking nonsense?
Comment #7
Mile23The
@group
is always the module you're converting. The first annotated@group
in a test class should be the module name.In the case of
UpdatePathTestBase
, the@trigger_error()
was added during the system module conversion, so--group system
would cover it.But, in reality, ANY test run which discovers deprecated classes will cause this fail. We limit by
--group
and/or--testsuite functional
because it's quicker and we're converting functional tests.Again: This is all because the symfony/phpunit-bridge
@trigger_error()
functionality depends on a test that is NOT process-isolated.BrowserTestBase
IS process-isolated, and that's a complex problem. #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated codeComment #8
dawehner@Mile23
So if I understand you correctly right now, we should remove / comment out the @trigger_error statements till we have proper support for them?
Comment #9
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedI would say we should not remove them or comment them out, as this would (a) be a backwards step, (b) make more work later and (c) hinder the tasks of discovering where those classes are used. I think what @mile23 was saying is that due to PHPUnit discovery using reflection (which I am just quoting from the issue summary, I do not know much about this), then the @deprecated message will be found no matter what actual tests are being run. So we can use
--group module-name
and it should get triggered.I don't think we have an automated way to run PHPUnit as part of the drupal.org testbot setup, but the developer who is coding the WTB to BTB conversion should always run it locally before uploading patches. Maybe as a condition of adding any
@deprecated
line, the programmer needs to show on the issue that they have run PHPunit. This could be added as an instruction on the main parent issue.Or maybe I have got that completely wrong?
Comment #10
Mile23So the deprecated
UpdatePathTestBase
is no longer in use, so that's good. :-)But to understand what happened, you can do this:
Add
@trigger_error('deprecation message', E_USER_DEPRECATED)
toDrupal\KernelTests\Core\Path\PathUnitTestBase
, just after the namespace declaration. As it turns out, this class only has one subclass:Drupal\KernelTests\Core\Path\AliasTest
.So let's run a kernel test that is NOT
AliasTest
. Let's run it's neighbor,UrlAlterTest
. It does NOT inherit fromPathUnitTestBase
.We'll use
--filter
.See the deprecation fail at the end? We never executed any class code in
PathUnitTestBase
, but we did end up having to require it during discovery, which triggered the error.Now let's run the same test by specifying a file name:
No deprecation fail, because no discovery was performed.
This means that if you were to only run tests by specifying the file name, you'd miss this type of deprecation fail, because no discovery was performed.
So: If you're deprecating test base classes (or anything, really) you should run tests using
phpunit
with--filter
or--group
to make sure you're not missing the deprecation error that you *might* be able to see.This still doesn't account for the deprecation errors you can't see due to the problem with process-isolated tests.
Comment #11
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedThanks Mile23 that is a really informative post. I kinda got what you meant before, but now I understand a whole lot more about Test Discovery, and I have replicated your example above.
The doc comment has the
@deprecated
tag, but I think it would also be useful to have@trigger_error('msg', E_USER_DEPRECATED)
just so that it is really clear, and no one inadvertantly starts using that class again (unlikely I know).Folks following this issue may also be interested in a new Coder sniff I am writing which actually insists that the presence of one of the above requires that the other is also added. #2908391: Add a rule for expected format of @deprecated and @trigger_error() text
Comment #13
LendudeIf I'm still reading this right, this is now about adding a trigger_errors where needed. We have an issue for that #2905470: @deprecated Drupal\system\Tests\Update\UpdatePathTestBase breaks test runs, so closing this now as a duplicate of that.
If I missed something and the scope of this is actually bigger, feel free to reopen of course.
Comment #14
LendudeAnd just saw that the trigger error for UpdatePathTestBase was already added in #2870009: Update: Convert system functional tests to phpunit