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

Add a way to cleanup error and exception handlers #1698

Closed
simPod opened this issue Feb 7, 2024 · 7 comments
Closed

Add a way to cleanup error and exception handlers #1698

simPod opened this issue Feb 7, 2024 · 7 comments
Assignees

Comments

@simPod
Copy link
Contributor

simPod commented Feb 7, 2024

Problem Statement

PHPUnit 11 checks for any leftovers in error handlers sebastianbergmann/phpunit#5619

Solution Brainstorm

When \Sentry\ErrorHandler::registerOnceErrorHandler() is called, error handler is registered.

It would be nice to add degisterOnceErrorHandler(). Since the error handler is kept in ErrorHandler's state, it would be easy to deregister it.

@simPod simPod changed the title Add a way to cleanup error handler Add a way to cleanup error and exception handlers Feb 7, 2024
@cleptric
Copy link
Member

cleptric commented Feb 7, 2024

That makes sense, let me try to use PHPUnit 11 in the SDK tests and see what happens 😄

@stayallive
Copy link
Collaborator

Can I ask why you would want this?

Sentry shouldn't be active when running tests ideally and our own test suite is not going to be ready for PHPUnit 11 for some time probably. If you do load Sentry in your tests isn't it better to prevent loading the (fatal) error handler integrations so they don't register in the first place instead of de-registering them afterwards which I'm assuming needs to be done manually by the user in their own code anyway?

There is no method to de-register a specific error handler so by the time a degisterOnceErrorHandler is called other handler could have been registered causing us to remove other handlers in the process.

@simPod
Copy link
Contributor Author

simPod commented Feb 20, 2024

When doing full app integration tests I want to simulate the same environment as is in prod in order to test exactly the same thing that runs there.

@stayallive
Copy link
Collaborator

And how would you have setup the degisterOnceErrorHandler in your test suite if it existed?

Is not registering the error handler integrations not an good compromise here? I'm assuming your integration tests don't test for unhandled or fatal errors caught by Sentry? Or do they?

@simPod
Copy link
Contributor Author

simPod commented Feb 22, 2024

Is not registering the error handler integrations not an good compromise here? I'm assuming your integration tests don't test for unhandled or fatal errors caught by Sentry? Or do they?

Sure but then my tests skip some part of code that get's executed in prod and therefore do not cover it.


Currently my workaround in teardown looks similar to this:

        $res = [];

        while (true) {
            $previousHandler = set_exception_handler(static fn () => null);
            restore_exception_handler();

            if ($previousHandler instanceof Closure) {
                $reflection = new ReflectionFunction($previousHandler);
                if (
                    $reflection->getClosureThis() instanceof \Sentry\ErrorHandler
                    && $reflection->getName() === 'handleException'
                ) {
                    restore_exception_handler();

                    continue;
                }
            }

            if ($previousHandler === null) {
                break;
            }

            $res[] = $previousHandler;
            restore_exception_handler();
        }

        $res = array_reverse($res);

        foreach ($res as $handler) {
            set_exception_handler($handler);
        }

as imperfect as it may be, it allowed me to run my tests with phpunit 11 without compromising coverage.

@cleptric cleptric closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 2024
@simPod
Copy link
Contributor Author

simPod commented May 15, 2024

@cleptric what is the sentry's statement on this?

@cleptric
Copy link
Member

What @stayallive said.

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

No branches or pull requests

3 participants