Skip to content

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