Problem/Motivation

This issue is a stop-gap for some of the problems brought forward in #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

symfony/phpunit-bridge adds a test listener to all phpunit tests. In the startTest() event, this listener makes a list of @expectedDeprecation annotations on the test method. During the test, it then uses an error handler to account for E_USER_DEPRECATED errors triggered during the test. In the endTest() event, it accounts for the list of errors and the list of expected errors and fails the test as needed.

If the test is marked as @runTestInSeparateProcess, or otherwise signals that it should be isolated in a process, the start and end events happen, but no errors are ever handled. That's because they all happened in the context of a separate process, with no way to send the errors back to the listener.

This leads to the problem of @expectedDeprecations never having associated errors, leading to test fails based on these expectations.

This means that only non-isolated unit tests can legitimately set @expectedDeprecation. Since our deprecation policy includes writing a test to prove that the deprecation error is triggered, this could lead to some large degree of code contortion to prove the error was triggered. https://www.drupal.org/core/deprecation

Proposed resolution

Add a wrapper test listener which determines whether the test was run in isolation, and if it was, and there were expected deprecations, mark the test as risky.

This is a half-way solution which should prevent false fails, while also letting the user know that their test might not be entirely useful.

Remaining tasks

There is an upstream issue to attempt to fix this: https://github.com/symfony/symfony/issues/23003

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2884530_2.patch9.4 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
Status: Active » Needs review
FileSize
2884530_2.patch9.4 KB

Patch which re-uses tests from #2870194-31: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code

Adds Drupal\Tests\Listeners\DrupalPhpUnitBridgeListener which subclasses SymfonyTestsListener.

The best way to see the effects of this patch is to use the phpunit tool:

SIMPLETEST_DB=... SIMPLETEST_BASE_URL=... ./vendor/bin/phpunit -c core/ --group Test -v

This will produce results as follows:

Testing 
.R..RR...........RI

Time: 1.73 minutes, Memory: 184.00MB

There were 4 risky tests:

1) Drupal\Tests\Core\Test\PhpUnitBridgeIsolatedTest::testDeprecatedClass
Process-isolated tests might yield a false pass for @expectedDeprecation.

/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:42

2) Drupal\KernelTests\Core\Test\PhpUnitBridgeTest::testDeprecatedClass
Process-isolated tests might yield a false pass for @expectedDeprecation.

/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:42

3) Drupal\KernelTests\Core\Test\PhpUnitBridgeTest::testDeprecatedFunction
Process-isolated tests might yield a false pass for @expectedDeprecation.

/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:42

4) Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testSilencedError
Process-isolated tests might yield a false pass for @expectedDeprecation.

/Users/paul/pj2/drupal/core/tests/Drupal/Tests/Listeners/DrupalPhpUnitBridgeListener.php:42

--

There was 1 incomplete test:

1) Drupal\FunctionalTests\Core\Test\PhpUnitBridgeTest::testErrorOnSiteUnderTest
Restore this test to have an @expectedDeprecation annotation when page controllers do not swallow E_USER_DEPRECATED errors.

/Users/paul/pj2/drupal/core/tests/Drupal/FunctionalTests/Core/Test/PhpUnitBridgeTest.php:30

OK, but incomplete, skipped, or risky tests!
Tests: 19, Assertions: 89, Incomplete: 1, Risky: 4.

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Normal » Major

Promoting to match the parent issue. I'm not totally sure about the interim solution here and wondering if we should wait for upstream, but at least I can set the issue priority. :P

I don't think skipped tests are surfaced on CI and I've never seen "risky" actually used before; do we have anything else that does so?

Mile23’s picture

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

'Risky' generally means there's no assertion in the test. We got rid of all those for core, before 8.0 I think. https://phpunit.de/manual/4.8/en/risky-tests.html

The reason we'd do this is because process-isolated tests with @expectedDeprecation actually are risky. :-) They'll be seen as pass even if they fail, even if you use the phpunit runner. After the change here, we'd see R instead of . in a phpunit test run, and that's really the only change.

The only upstream interest is me, and I'm confused by symfony's testing system. Plus we're stuck at an old version of symfony, so we'd probably have to convince someone to OK a backport. https://github.com/symfony/symfony/issues/23003

I'm increasingly convinced the way to solve this is to make our own implementation, so we can include Simpletests.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Closed (outdated)

symfony/phpunit-bridge can now deal with process-isolated deprecation errors, so this issue is outdated. #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code