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

Keeping tests memory usage at bay #4679

Closed
malarzm opened this issue May 20, 2021 · 3 comments
Closed

Keeping tests memory usage at bay #4679

malarzm opened this issue May 20, 2021 · 3 comments
Labels
type/enhancement A new idea that should be implemented

Comments

@malarzm
Copy link

malarzm commented May 20, 2021

Lately we wanted to tackle PHPUnit's deprecations in my work project and the one we're struggling with is the event listener change. Having read #4676 and seeing the draft implementation for the event system I do understand the motivation for the change and I'll be happy to use it for most of our listeners. Sadly there is one I have no idea what to do about as it violates the fix of a fundamental flaw in the new proposal, namely passing Test instances around. To the point: we have a "memory guard" listener (https://gist.github.com/malarzm/e8c6141b510708e52c8535d2a13cd613 inspired by https://github.com/mybuilder/phpunit-accelerator) that makes sure all properties we set throughout the test run are freed afterwards. It helps us immensely and we have a hard time saying good bye to it :) We could put the code in our base test class but that would need to happen for every base test class we have (not that many but still not one). Also I believe we're not the only ones with such problems thus more people could benefit from a general solution.

Is the "free all userland properties in test classes" something that could become a feature in PHPUnit itself? Probably it'd need to become an opt-in one to prevent problems for users, however with time it could become an opt-out to promote tests that do not share state between them. I'll be happy to devote some time and contribute it for the next minor/major release.

@malarzm malarzm added the type/enhancement A new idea that should be implemented label May 20, 2021
@sebastianbergmann
Copy link
Owner

Is the "free all userland properties in test classes" something that could become a feature in PHPUnit itself?

Your implementation does not work for properties that have a declared type. Simply doing $property->setValue($this, null); does not work when the property's type declaration is not nullbable. Sure, this can be safeguarded against to avoid the type error, but the goal of of what you're trying to do cannot be achieved.

I see two right solutions to this problem. In the short term, implement a tearDown() (or #[After} attributed or @after annotated) method that correctly and specifically cleans up any objects stored in properties on the test case object.

In the long run, this problem will go away once PHPUnit's test runner no longer holds references to TestCase objects after a test has been run.

@malarzm
Copy link
Author

malarzm commented May 21, 2021

Simply doing $property->setValue($this, null); does not work when the property's type declaration is not nullbable. Sure, this can be safeguarded against to avoid the type error, but the goal of of what you're trying to do cannot be achieved.

That is from the original implementation and indeed is broken when typed properties come into play. Our listener however calls unset which circumvent typed property issue you are mentioning and works perfectly (we're using typed properties in tests).

In the long run, this problem will go away once PHPUnit's test runner no longer holds references to TestCase objects after a test has been run.

How long is the run if I may ask? That sounds great in its simplicity, is there anything I could do to help?

@Kingdutch
Copy link

For anyone who comes across this issue, the issue tracking not keeping around a reference to TestCase is: #4705 That also contains some more info on what comes before that can be tackled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

No branches or pull requests

3 participants