Problem/Motivation

In #2571475: Outbound HTTP requests fail with KernelTestBase we added a test without an assertion. PHPunit usually fails if a test does not assert anything, but because KernelTestBaseTest::tearDown() performs an assertion, all KernelTestBaseTests are considered to assert something.

Steps to reproduce

Proposed resolution

Refactor KernelTestBaseTest::tearDown() so it does not perform an assertion but still fails if the teardown conditions are not met.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3196507-9.patch1.04 KBlongwave
#7 run_isolated_disables_strict.png86.52 KBmxr576
#2 3196507.patch799 byteslongwave

Issue fork drupal-3196507

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3196507.patch799 bytes
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Thanks for opening this follow up, LGTM.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Why didn't this fail while #3196470: KernelTestBaseTest::testOutboundHttpRequest() does not perform any assertion is still open?

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

hm, indeed, that is odd beStrictAboutTestsThatDoNotTestAnything="true", my gut says something else also performs an assert.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Okay, so the following happens, if I remove ALL test methods before testOutboundHttpRequest() then the error is triggered, if at least one test method runs before testOutboundHttpRequest() then PHPUnit does not warn about the missing assert in testOutboundHttpRequest().

2) Drupal\KernelTests\KernelTestBaseTest::testOutboundHttpRequest
This test did not perform any assertions

/var/www/html/core/tests/Drupal/Tests/Listeners/DrupalListener.php:127
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:423
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:360
/var/www/html/vendor/phpunit/phpunit/src/Util/PHP/AbstractPhpProcess.php:171
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:627
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:204
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:163
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new run_isolated_disables_strict.png86.52 KB

Just as I thought, the fact that this test runs in a separate process causes the problem. Before the test starts, PHPUnit disables the strict mode, bug?

No, that is not true, I am not sure why I saw this result, but since then I realised \Symfony\Bridge\PhpUnit\Legacy\SymfonyTestsListenerTrait::startTest() gets called and because \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait is implemented by KernelTestBase (which extended by its own test), it disables the beStrictAboutTestsThatDoNotTestAnything() :(

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Related issues: +#3172438: Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead.

The commit which added the problematic line to upstream: https://github.com/symfony/phpunit-bridge/commit/82c0ff21da6ace856ac3494...

and the ExceptionDeprecationTrait was added in Since symfony/phpunit-bridge 5.1: Using "@expectedDeprecation" annotations in tests is deprecated, use the "ExpectDeprecationTrait::expectDeprecation()" method instead.

This seems a side-effect of this change.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3196507-9.patch1.04 KB

Is that because expected deprecations are counted as assertions, but if a test only expects deprecations and doesn't assert anything else, if beStrictAboutTestsThatDoNotTestAnything is set then PHPUnit will fail early? The bridge class does its own checks using the checkNumAssertions flag to apparently work around this in some cases.

We can add an explicit extra test, which helps I guess, but doesn't stop us adding other tests that don't assert anything.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

We might also throw a \RuntimeException instead, but it does not really matter. RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
$  ./vendor/bin/phpunit -v core/tests/Drupal/KernelTests/KernelTestBaseTest.php --filter testTearDown                                                                                                                                                                     PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.13
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml

Testing Drupal\KernelTests\KernelTestBaseTest
R                                                                   1 / 1 (100%)

Time: 1.73 seconds, Memory: 6.00 MB

There was 1 risky test:

1) Drupal\KernelTests\KernelTestBaseTest::testTearDown
This test is annotated with "@doesNotPerformAssertions" but performed 1 assertions

/Users/alex/dev/sites/drupal8alt.dev/vendor/phpunit/phpunit/src/Framework/TestResult.php:916

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 1, Risky: 1.
$ ./vendor/bin/phpunit -v core/tests/Drupal/KernelTests/KernelTestBaseTest.php                                                                                                                                                                                  
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.13
Configuration: /Users/alex/dev/sites/drupal8alt.dev/phpunit.xml

Testing Drupal\KernelTests\KernelTestBaseTest
.........................                                         25 / 25 (100%)

Time: 27.38 seconds, Memory: 6.00 MB

OK (25 tests, 87 assertions)

The test only fails when it is run by itself?!?!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This is a problem with Symfony code. We need to implement proper logic if a test returns true for $test->doesNotPerformAssertions() and cause it to fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Here's a PR to address this in symfony - https://github.com/symfony/symfony/pull/40957

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We are now using a version of symfony/phpunit-bridge that includes the fix in #13 but I still get the same result in #11 when running the single test vs running the entire class?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake
Status: Needs work » Active

Maybe time to try again in PHPUnit 10.

mondrake opened merge request !9122

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: KernelTestBaseTest::tearDown() should not perform assertions » [PP-1] KernelTestBaseTest::tearDown() should not perform assertions
Assigned: mondrake » Unassigned
Status: Active » Postponed
Related issues: +#3466788: Fix ‘risky’ tests

Postponed on #3466788: Fix ‘risky’ tests

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: [PP-1] KernelTestBaseTest::tearDown() should not perform assertions » KernelTestBaseTest::tearDown() should not perform assertions
Assigned: Unassigned » mondrake
Status: Postponed » Active

Blocker is in.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Refactor seems fine, accomplishes the task without breaking anything

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Is it not simpler just to do this to explicitly test the tearDown() method, and avoid this code running on all other teardowns in this class?

-  protected function tearDown(): void {
+  public function testTearDown(): void {
     parent::tearDown();

Maybe I could have thought of this three years ago :)

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Reviewed & tested by the community

Uhm we shouldn’t call tearDown() from a test directly IMHO. The tear down process involves the framework calling methods marked with PostCondition and After attributes - tearDown is just ‘one’ of such methods nowadays.

  • longwave committed d38aac15 on 11.x 
    Issue #3196507 by mondrake, longwave, mxr576, alexpott:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Fair enough. Let's get this in.

Committed d38aac1 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mondrake closed merge request !9122