Skip to content

Request for Consistent Handler Restoration on expectOutputString and expectOutputRegex Failure #5842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KentarouTakeda
Copy link

Dear Sebastian Bergmann,

This pull request addresses an issue where error and exception handlers are not restored properly when expectOutputString or expectOutputRegex assertions fail. Below is a minimal code example to reproduce the behavior:

<?php

namespace Test;

use PHPUnit\Framework\TestCase;

class Test extends TestCase
{
    public $existsOriginalErrorHandler;
    public $existsOriginalExceptionHandler;

    // Assume that the error handler is changed within the test code.
    public function setUp(): void
    {
        $this->existsOriginalErrorHandler = (bool)set_error_handler(fn () => null);
        $this->existsOriginalExceptionHandler = (bool)set_exception_handler(fn () => null);
    }

    // Assume that the error handler changes are reliably restored to their original state in `tearDown()`.
    public function tearDown(): void
    {
        restore_exception_handler();
        restore_error_handler();
    }

    // In PHPUnit, tests typically start with this handler setting.
    public function test1()
    {
        $this->assertTrue($this->existsOriginalErrorHandler);
        $this->assertFalse($this->existsOriginalExceptionHandler);
    }

    public function test2()
    {
        // The same assertions as before will obviously succeed.
        $this->assertTrue($this->existsOriginalErrorHandler);
        $this->assertFalse($this->existsOriginalExceptionHandler);

        // This assertion is intentionally set to fail. This failure leads to:
        $this->expectOutputString('This test will be failed'); // Failure (intentional)
    }

    // The next test starts without `tearDown()` being executed,
    public function test3()
    {
        // The exact same assertions will fail here.
        $this->assertTrue($this->existsOriginalErrorHandler);
        $this->assertFalse($this->existsOriginalExceptionHandler); // Failure (unintentional)
    }
}

Although this may not be a "bug" per se, the inconsistent behavior makes it challenging for some frameworks to avoid PHPUnit's warning messages like "Test code or tested code ..." . It would be greatly appreciated if this issue could be addressed in PHPUnit to ensure consistent handler restoration.

Thank you for considering this request.

Best regards,
Kentaro Takeda

@sebastianbergmann sebastianbergmann added type/bug Something is broken feature/test-runner CLI test runner labels May 25, 2024
Copy link

codecov bot commented May 25, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.32%. Comparing base (d475be0) to head (84cf94e).
⚠️ Report is 2913 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5842      +/-   ##
============================================
+ Coverage     89.28%   92.32%   +3.03%     
+ Complexity     6585     6555      -30     
============================================
  Files           693      699       +6     
  Lines         19937    19774     -163     
============================================
+ Hits          17801    18256     +455     
+ Misses         2136     1518     -618     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staabm
Copy link
Contributor

staabm commented Aug 13, 2025

@KentarouTakeda could you please double check your reported issue still exist?
I have a feeling this was fixed in the meantime.

if it still does not produce the expected outcome, please provide describe in more detail why/what does not work.

thank you

@sebastianbergmann sebastianbergmann added the status/waiting-for-feedback Waiting for feedback from original reporter label Aug 14, 2025
@KentarouTakeda
Copy link
Author

KentarouTakeda commented Aug 17, 2025

@staabm
The issue still persists. I tested with the two latest actively supported versions, 11.5.33 and 12.3.5, and confirmed the same behavior as in the initial report.

Let me explain the details. Please take a look at the test code I first presented. test1 and test3 contain exactly the same code. Nevertheless, test1 passes while test3 fails. In this case, the success of test1 is the correct behavior. The unintended failure of test3 is caused by the failure of test2 with expectOutputString(). This pull request aims to fix these behaviors.

The minimal reproduction code above demonstrates the issue, but this behavior also affects feature tests in many frameworks, including Laravel and Symfony, producing unintended test result reports for users.

For example, in Laravel, running the following test code:

<?php

namespace Tests\Feature;

use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Test;
use Tests\TestCase;

// Important note: Here, `TestCase` extends the Laravel Framework's `TestCase`.
class ExampleTest extends TestCase
{
    #[Test]
    public function intentionalFailure()
    {
        $this->expectOutputString('This test will be failed'); // Failure (intentional)
    }

    #[Test]
    #[DataProvider('repeatProvider')]
    public function tenSuccessfulTests()
    {
        $this->assertTrue(true);
    }

    public static function repeatProvider()
    {
        return array_map(fn() => [], range(1, 10));
    }
}

produces the following result:

FRRRRRRRRRR                                                       11 / 11 (100%)

While the first test fails as expected, it is not expected that all subsequent tests are marked as Risky.

The detailed output includes messages like:

There was 1 risky test:

1) Tests\Feature\ExampleTest::tenSuccessfulTests
* Test code or tested code removed error handlers other than its own

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

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

* Test code or tested code removed exception handlers other than its own

As shown above, in frameworks that manipulate error or exception handlers (including Laravel and Symfony), a failure in tests involving input/output can lead to user confusion due to misleading test reports.

This pull request fixes the problem where the failure of one test causes handler state inconsistencies that propagate to subsequent tests, resulting in them being incorrectly marked as Risky.

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 status/waiting-for-feedback Waiting for feedback from original reporter type/bug Something is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants