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

Speed optimize test case pretty printing #5735

Closed
wants to merge 1 commit into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 11, 2024

because the colored printing is slow and most of the time its not used, see investigation in #5728 (comment)

github.com didn't let me re-open the previous PR, therefore I created a new one.

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.01%. Comparing base (e04999a) to head (76c826f).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #5735   +/-   ##
=========================================
  Coverage     90.01%   90.01%           
- Complexity     6491     6492    +1     
=========================================
  Files           688      688           
  Lines         19715    19720    +5     
=========================================
+ Hits          17746    17751    +5     
  Misses         1969     1969           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@staabm staabm changed the title Drop colored prettified method name printing Speed optimize test case pretty printing Mar 12, 2024
@staabm staabm marked this pull request as ready for review March 12, 2024 11:53
@staabm staabm marked this pull request as draft March 12, 2024 11:55
@staabm
Copy link
Contributor Author

staabm commented Mar 12, 2024

new approach looks good - perf wise

grafik

@staabm staabm marked this pull request as ready for review March 12, 2024 11:57
@sebastianbergmann sebastianbergmann added feature/testdox The TextDox printer/formatter type/performance Issues related to resource consumption (time and memory) labels Mar 12, 2024
@sebastianbergmann
Copy link
Owner

After this change, TestDox::prettifiedMethodName(true) will no longer return a colorized version of the prettified test method name when colorized output is not requested. This is confusing and not acceptable, sorry.

We should instead change the signature from prettifiedMethodName(bool $colorize = false) to prettifiedMethodName() to make it clear that the method can no longer optionally return a colorized string.

When colorized output in TestDox format is configured, we should perform the colorization in (invoke it from) PHPUnit\TextUI\Output\TestDox\ResultPrinter.

@staabm
Copy link
Contributor Author

staabm commented Mar 12, 2024

when TestDox is created via fromClassNameAndMethodName - even before this PR TestDox::prettifiedMethodName(true) will also not return colorized output

When colorized output in TestDox format is configured, we should perform the colorization in (invoke it from) PHPUnit\TextUI\Output\TestDox\ResultPrinter.

to colorize we need a reference to a TestCase, which we don't have in the ResultPrinter

@staabm
Copy link
Contributor Author

staabm commented Mar 13, 2024

after #5747 and #5748 landed, I need to re-measure whether this PR is still worth it

@staabm staabm marked this pull request as draft March 13, 2024 19:42
@staabm
Copy link
Contributor Author

staabm commented Mar 14, 2024

the pretty printing is still visible in the profiles.. but pretty minor now. in case we could drop doing it in non --testdox that would be awesome.

atm I don't see a way to achieve this though.

@staabm staabm closed this Mar 14, 2024
@staabm staabm deleted the lazy-pretty branch March 14, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/testdox The TextDox printer/formatter type/performance Issues related to resource consumption (time and memory)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants