Skip to content

Conversation

X-Coder264
Copy link
Contributor

@X-Coder264 X-Coder264 commented Jan 25, 2022

Fixes problems described in #40592 (comment)

TLDR; Laravel never cleaned up its handlers, resulting in the PHPUnit error handler never firing for unit tests that came after Laravel's integration tests, plus since the handlers were never cleaned it resulted in a memory leak.

In order to avoid having to call $this->withoutDeprecationHandling() in every Laravel integration test I've added that call in the protected function deprecationHandling() hook method which is now called from the base test class setUp method (users can override it in their base class if they want to change the behavior).

The changes made in the base test class here should also be ported over to Testbench, after which I can fix all the deprecation exceptions that will pop up on CI, before this PR can be merged.

Also note that this PR makes use of a terminating callback to restore the handlers, which also meant having to manually call $this->app->terminate() in the test teardown (it's weird that this was not already called as terminate is always called in a real HTTP request).

Fixes #40554

@donnysim
Copy link
Contributor

donnysim commented Jan 26, 2022

There might still be something wrong, the deprecation errors are still not popping up in full test runs.

Edit: probably never mind, forgot about the Testbench note.

@X-Coder264
Copy link
Contributor Author

Yeah, the Testbench changes are needed before the deprecation exceptions pop up on CI.

Thank you for testing.

@nunomaduro nunomaduro marked this pull request as draft January 26, 2022 12:49
@nunomaduro
Copy link
Member

Converting this to draft while I try to understand the reasoning behind this pull request.

@nunomaduro
Copy link
Member

@X-Coder264 @donnysim Let me know what do you think about this alternative: #40656.

@nunomaduro nunomaduro closed this Jan 29, 2022
@X-Coder264 X-Coder264 deleted the fix-error-handling branch February 5, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in tests because HandleExceptions never cleans up
3 participants