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

Also destruct TestCase objects early that use a data provider #5875

Closed
wants to merge 1 commit into from

Conversation

talkinnl
Copy link

Refactor unsetting tests during TestSuite::run, faster destruction.

  • Continuing on the destruction fix: The iterator also holds the array of tests. For tests with dataProviders, this results in not immediately destructing each Test, but only when the deeper TestSuite is done.
  • Refactored cleanup tricks: Just do a first loop so we no longer need properties nor the iterator, and then array_shift as we go.
  • This also reduces amount of code executed: One loop up front instead of the unset loop per test.
  • Added test coverage.

…uction of instances.

- Continuing on the destruction fix: The iterator also holds the array of tests. For tests with dataProviders, this results in not immediately destructing each Test, but only when the deeper TestSuite is done.
- Refactored cleanup tricks: Just do a first loop so we no longer need properties nor the iterator, and then array_shift as we go.
- Added test coverage.
@talkinnl talkinnl changed the base branch from main to 10.5 June 20, 2024 06:36
@sebastianbergmann sebastianbergmann added feature/test-runner CLI test runner type/performance Issues related to resource consumption (time and memory) labels Jun 20, 2024
@sebastianbergmann
Copy link
Owner

Thank you, Maarten.

I guess this also solves @mvorisek's issue with TestCase object destruction expressed in #5867 (comment).

@talkinnl
Copy link
Author

In #4705 (comment) , @mvorisek noted that his issue wasn't fixed yet.
The situation was quite improved by #5861 , but I discovered the timing is still not optimal if dataProviders are used.

Seeing the order of events is easy, just add a __destruct() to TestCase which outputs a 'Z':
AFTER PR 5861:

./phpunit
PHPUnit 10.5.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.18
Configuration: /Users/talkinnl/checkouts/phpunit/phpunit.xml

..ZZ....ZZZZ....ZZZZ......................................ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ...ZZZ..ZZ..ZZ.Z.Z..ZZ..   61 / 3493 (  1%)
ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ....ZZZZ..ZZ...ZZZ....ZZZZ...ZZZ...ZZZ.Z.Z.Z...ZZZ..ZZ..ZZ..ZZ..ZZ..  122 / 3493 (  3%)
ZZ..ZZ..ZZ...ZZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..ZZ..  183 / 3493 (  5%)

(Before that change, all Zs would be at the very end)

WITH THIS PR 5875:

./phpunit
PHPUnit 10.5.22 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.18
Configuration: /Users/talkinnl/checkouts/phpunit/phpunit.xml

.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.   61 / 3493 (  1%)
Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.  122 / 3493 (  3%)
Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.Z.  183 / 3493 (  5%)

So a nice pattern between dots and Zs. :)

@talkinnl
Copy link
Author

phpunit own test suite:

Time: 00:52.337, Memory: 42.00 MB
Tests: 3493, Assertions: 9540, Skipped: 10.

TO

Time: 00:51.999, Memory: 40.00 MB
Tests: 3493, Assertions: 9540, Skipped: 10.

And then I added the test, so tests and assertions did increase. So no regressions, and a change from 42 to 40MB.

@sebastianbergmann sebastianbergmann changed the title Improved Test destruction, fixed last delays and dataProvider behavior. Also destruct TestCase objects early that use a data provider Jun 20, 2024
@sebastianbergmann
Copy link
Owner

Merged manually.

@sebastianbergmann
Copy link
Owner

@talkinnl FYI: I changed your test case into an end-to-end test (67323d2) so that the execution order of its tests is not affected by randomness.

@mvorisek
Copy link
Contributor

This is perfect and thank you @talkinnl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/performance Issues related to resource consumption (time and memory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants