Skip to content

Conversation

@LukeTowers
Copy link
Contributor

Currently, if a test calls markTestSkipped() (throwing a SkippedWithMessageException) and tearDown() then throws its own exception (e.g. due to uninitialized state), PHPUnit swallows the tearDown() exception and only reports the skip and any resulting risky‑test warnings.

This change treats SkippedWithMessageException differently so that exceptions raised in tearDown() are still surfaced, making it easier to diagnose teardown issues even in skipped tests.

Related: laravel/framework#49502

Issue Fixed:

If a test is marked as skipped and your tearDown() throws an exception before Laravel’s cleanup runs, PHPUnit will swallow that teardown exception and instead show the vague “Test code or tested code did not remove its own exception handlers” message.

Why:

markTestSkipped() throws a SkippedWithMessageException.
PHPUnit’s runBare() always calls tearDown() afterward, but if tearDown() throws, the exception is ignored because PHPUnit assumes the earlier skip exception is the “real” outcome.
Since Laravel’s cleanup never ran, global handlers remain, triggering the risky-test warning.

Example:

public function setUp(): void
{
    parent::setUp();

    if (!File::exists(base_path('node_modules'))) {
        $this->markTestSkipped('This test requires node_modules to be installed');
    }

    // Never reached if skipped:
    $this->fixturePath = base_path('modules/system/tests');
}

public function tearDown(): void
{
    // Uses $this->fixturePath but it’s null if skipped early
    if (File::isDirectory($this->fixturePath . '/node_modules')) {
        File::deleteDirectory($this->fixturePath . '/node_modules');
    }

    parent::tearDown();
}

Result:

Instead of the teardown exception, PHPUnit only reports:

Test code or tested code did not remove its own exception handlers.

Currently, if a test calls markTestSkipped() (throwing a
SkippedWithMessageException) and tearDown() then throws its own exception
(e.g. due to uninitialized state), PHPUnit swallows the tearDown()
exception and only reports the skip and any resulting risky‑test
warnings.

This change treats SkippedWithMessageException differently so that
exceptions raised in tearDown() are still surfaced, making it easier to
diagnose teardown issues even in skipped tests.
LukeTowers added a commit to wintercms/winter that referenced this pull request Jul 26, 2025
Skipped tests still run their tearDown() method.

Related: sebastianbergmann/phpunit#6281
bennothommo pushed a commit to wintercms/wn-system-module that referenced this pull request Jul 26, 2025
Skipped tests still run their tearDown() method.

Related: sebastianbergmann/phpunit#6281
bennothommo pushed a commit to wintercms/wn-backend-module that referenced this pull request Jul 26, 2025
Skipped tests still run their tearDown() method.

Related: sebastianbergmann/phpunit#6281
@sebastianbergmann sebastianbergmann changed the title Fix: preserve tearDown() exceptions when test is skipped Exceptions raised in after-test method are not reported for skipped tests Aug 11, 2025
@sebastianbergmann sebastianbergmann added type/bug Something is broken feature/test-runner CLI test runner version/11 Something affects PHPUnit 11 version/12 Something affects PHPUnit 12 labels Aug 11, 2025
sebastianbergmann added a commit that referenced this pull request Aug 11, 2025
@sebastianbergmann
Copy link
Owner

Cherry-picked into 11.5 and merged to 12.3 and main from there.

nicolas-grekas added a commit to symfony/symfony that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/form that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/yaml that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/security-bundle that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/console that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/validator that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
symfony-splitter pushed a commit to symfony/var-dumper that referenced this pull request Aug 12, 2025
This PR was merged into the 7.4 branch.

Discussion
----------

 run tests with PHPUnit 12.3

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The test updates reflect the following changes in PHPUnit 12.2 and 12.3:

- a warning is emitted when a data provider provides data sets that have more values than the test method consuming the data set
- the format in which data sets are displayed in test reports has changed affecting the modified test in the PhpUnitBridge (see sebastianbergmann/phpunit#6272)
- errors triggered in the `tearDown()` method of skipped tests are no longer silence affecting the modified tests in the Form component where the `$defaultLocale` property is not initialised (see sebastianbergmann/phpunit#6281)

Commits
-------

cf7e41e1092 run tests with PHPUnit 12.3
@raveren
Copy link

raveren commented Aug 13, 2025

It's an unending ball of yarn, but I am pretty sure this change caused several issues on my code base, lets see if I'm the only one:

ArgumentCountError: Too few arguments to function PHPUnit\Runner\ErrorHandler::enable(), 0 passed in /var/www/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php on line 358 and exactly 1 expected
image

the line 77 in the output is just parent::tearDown() :

image

and then there's this
image

I am running Laravel 11.45 and the latest PHPUnit 12.3.4

@raveren
Copy link

raveren commented Aug 13, 2025

Yeah, I am getting

Test code or tested code removed error handlers other than its own

OR

ArgumentCountError: Too few arguments to function PHPUnit\Runner\ErrorHandler::enable(), 0 passed in /var/www/vendor/laravel/framework/src/Illuminate/Foundation/Bootstrap/HandleExceptions.php on line 358 and exactly 1 expected

for EVERY test if I update PHPUnit from 12.2.9 to 12.3.4.

Also, with this, my teamcity pipeline timeouts and nothing can kill it besides a server restart :D

Also I smell smoke brb (just kidding about this one!)

@sebastianbergmann
Copy link
Owner

PHPUnit\Runner\ErrorHandler is @internal and not covered by the backward compatibility promise for PHPUnit. Its API was changed and it looks like Illuminate/Foundation/Bootstrap/HandleExceptions.php needs to be adapted.

@LukeTowers
Copy link
Contributor Author

@raveren it was this commit that's causing the issue: 83f6449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/test-runner CLI test runner type/bug Something is broken version/11 Something affects PHPUnit 11 version/12 Something affects PHPUnit 12

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants