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

CommentFileSizeAuthor
#4 2905470-4.patch1.34 KBLendude
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Good 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?

Mile23’s picture

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

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Active » Needs review
Issue tags: +phpunit initiative
FileSize
2905470-4.patch1.34 KB

Here is the patch to update the HAL tests.

run all process-isolated tests using the phpunit tool during conversion

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.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

"run all process-isolated tests using the phpunit tool during conversion"

What would this look like in practice? This sounds like it can take a while running it on a local machine.

Basically run functional tests in PHPUnit. So for the system module:

$ ./vendor/bin/phpunit -c core/ --testsuite functional --group system --stop-on-fail

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 discover

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

The 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 of group 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?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

The problem I see with running that command is that you have no idea which group you want.

The @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 code

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

@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?

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

So if I understand you correctly right now, we should remove / comment out the @trigger_error statements till we have proper support for them?

I 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?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

So 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) to Drupal\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 from PathUnitTestBase.

We'll use --filter.

$ ./vendor/bin/phpunit -c core/ --testsuite kernel --filter UrlAlterTest
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing 
.

Time: 3.88 seconds, Memory: 36.00MB

OK (1 test, 2 assertions)

Remaining deprecation notices (1)

deprecation message: 1x
    1x in KernelTestSuite::suite from Drupal\Tests\TestSuites

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:

$ ./vendor/bin/phpunit -c core/ core/tests/Drupal/KernelTests/Core/Path/UrlAlterTest.php 
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Path\UrlAlterTest
.

Time: 1.36 seconds, Memory: 4.00MB

OK (1 test, 2 assertions)

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.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented
Related issues: +#2908391: Add a rule for expected format of @deprecated and @trigger_error() text

Thanks 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 deprecated UpdatePathTestBase is no longer in use, so that's good. :-)

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

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.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Closed (duplicate)

If 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.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented

And just saw that the trigger error for UpdatePathTestBase was already added in #2870009: Update: Convert system functional tests to phpunit