Skip to content
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

Refactor deprecation warning tests #6093

Merged
merged 4 commits into from
Dec 31, 2023

Conversation

distantnative
Copy link
Member

PHPUnit 10 won't throw errors anymore, which is why we need to switch to a different test setup for our deprecation errors

@distantnative distantnative added this to the 4.1.0 milestone Dec 27, 2023
@distantnative distantnative self-assigned this Dec 27, 2023
@distantnative distantnative mentioned this pull request Dec 27, 2023
@distantnative distantnative requested a review from a team December 27, 2023 16:37
@lukasbestle
Copy link
Member

Looking into the CI failure. Something seems to be off as my commit worked locally before but now works neither in CI nor locally.

We need to pop exactly as many error handlers off the stack as we added, even if e.g. the `assertError()` method was called multiple times
@lukasbestle lukasbestle force-pushed the refactor/tests-deprecated-warning branch from f5f9a89 to 0cbf7c8 Compare December 30, 2023 17:23
@lukasbestle
Copy link
Member

@distantnative Reverted my earlier commit as it broke subsequent tests, but I couldn't find out why.

From reading the code, this test should not need to restore the error handler as the one set inside Helpers::handleErrors() is already reset within the method and no other error handler is set:

public function testHandleErrorsWarningCaughtCallbackValue()
{
$this->hasErrorHandler = true;
$this->assertSame('handled', Helpers::handleErrors(
fn () => trigger_error('Some warning', E_USER_WARNING),
fn (int $errno, string $errstr) => true,
fn () => 'handled'
));
}

Can you remember why you added the hasErrorHandler line in this test?

@distantnative distantnative merged commit 113791a into develop-minor Dec 31, 2023
12 checks passed
@distantnative distantnative deleted the refactor/tests-deprecated-warning branch December 31, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants