fix: Setting error handler inside an isolated test may cause test failure on PHP 8.3

[siketyan]

Related Issues

• Process isolation doesn't work starting from 10.2.0 #5595
• PHPUnit's process isolation doesn't work due to Laravel's application bootstrapping laravel/framework#49237
• Running feature tests in a separate process in PHP 8.3 fails laravel/framework#49593

Summary

On PHP 8.3, calling set_error_handler in an isolated test (via #[RunInSeparateProcess]) may cause test failure, regardless of what the error handler does.

Some frameworks, including Laravel, enables their error handler on booting. After the test run, PHPUnit's temporary code tries to call rewind(STDOUT). This may cause an error when STDOUT is not rewindable, as the comment says; this is expected. The error handler set by the framework is called even though @ is prepended to the call.

After that PHPUnit\Runner\ErrorHandler is called somehow only on PHP 8.3; I don't know why this is called (a PHP bug?). ErrorHandler tries to find the call stack, but it's not possible. Then the test fails by ErrorHandler throwing NoTestCaseObjectOnCallStackException. In my application tests, this is shown as 'Test was run in child process and ended unexpectedly'.

This pull request sets a no-op error handler temporarily during doing fallible calls with @. This helps the tests calling set_error_handler but not calling restore_error_handler after the test.

Workaround

Call restore_error_handler on tearDown or tearDownAfterClass depending on the situation.

[sebastianbergmann]

CC @mvorisek

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0bd6637) 89.92% compared to head (a5f3908) 89.92%.

Additional details and impacted files

@@            Coverage Diff            @@
##               10.5    #5677   +/-   ##
=========================================
  Coverage     89.92%   89.92%           
  Complexity     6426     6426           
=========================================
  Files           680      680           
  Lines         20532    20532           
=========================================
  Hits          18463    18463           
  Misses         2069     2069           

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

[mvorisek]

What is the behaviour

before/after f38fc88 (PHPUnit 10.5.9)

and before/after #5619 (PHPUnit master (upcoming 11.x))?

On PHP 8.3, calling...

Is it somehow related with php/php-src@b3e33be?

[mvorisek]

Suggested change

	set_error_handler('__phpunit_error_handler');
	set_error_handler(static fn() => false);

use anonymous function

and return false to keep error_get_last() set, the PHP error handlers does not support nesting, @ should supress any PHP output

[siketyan]

Fixed in a5f3908.

[siketyan]

before/after f38fc88 (PHPUnit 10.5.9)

Nothing changed for the regression test.

before/after #5619 (PHPUnit master (upcoming 11.x))?

Before 93aa92b: Same as 10.5 branch.
After 93aa92b: Test succeeds but marked as a risky test:

PHPUnit 10.5.3-33-g5da503ce1 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.2

R                                                                   1 / 1 (100%)

Time: 00:00.069, Memory: 8.00 MB

There was 1 risky test:

1) PHPUnit\TestFixture\Issue5595Test::test
Test code or tested code did not remove its own error handlers

/Users/siketyan/.local/src/github.com/sebastianbergmann/phpunit/tests/end-to-end/regression/5595/Issue5595Test.php:25

OK, but there were issues!
Tests: 1, Assertions: 1, Risky: 1.

Is it somehow related with php/php-src@b3e33be?

It can be, I couldn't find which part of the change cause this.

[mvorisek]

before/after #5619 (PHPUnit master (upcoming 11.x))?

Before 93aa92b: Same as 10.5 branch. After 93aa92b: Test succeeds but marked as a risky test:

If so, it seems the error handler is set during a test not during a bootstrap. Such issue is detected by PHPUnit master (upcoming 11.x) correctly and the proper fix is to never end a test with modified global handlers as advised in laravel/framework#49237 (comment) and like reasoned in #5619 (comment).

[siketyan]

I'll submit a pull request to Laravel later. Are we not going to add this change for the temporary fix in PHPUnit 10 anyway?

[siketyan]

To be clear: "bootstrapping" which I wrote in the description is of Laravel, not PHPUnit. It's called in setUp for each tests.

[mvorisek]

In PHPUnit 10.x is it only undetected, in (upcoming) PHPUnit 11.x is it detected. In PHPUnit 10.x the detection was not added for BC reasons. So I think there is nothing to fix.

https://github.com/sebastianbergmann/phpunit/blob/0647107202/tests/end-to-end/regression/5592/Issue5592Test.php test should be improved to test an error handler added in setUp/tearDown and /w process isolation.