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

Lazy collorize method name #5728

Closed
wants to merge 1 commit into from
Closed

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 9, 2024

looking into blackfire profiles I can see the TestDoc colorization takes 10-15% of runtime.

testing of this PR confirmed the thesis

before

php ./phpunit --testsuite unit     
…
Time: 00:00.466, Memory: 44.00 MB

after

php ./phpunit --testsuite unit     
…
Time: 00:00.420, Memory: 52.00 MB

blackfire diff

grafik

now the bad news :-). while this PR works with the unit test-suite, it does not with the end-to-end suite.

I can see that the changed TestDox class is no longer serializable, and therefore does not work with tests using process isolation or similar features.

since 10-15% percent is not that bad, I figured it would nevertheless make sense to bring this up for discussion.
maybe we can utilize the basic idea of doing coloring lazily for all tests which do not use process isolation..?

I would love anyones feedback

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it.

PHPUnit\Event\Code\TestDox is a value object and must not do anything else than hold the values and provide access to it.

@staabm staabm deleted the lazy-pretty branch March 10, 2024 06:23
@staabm
Copy link
Contributor Author

staabm commented Mar 10, 2024

yeah I realized it, after everything broke when changing it :-).

the only place we currently - depending on config - use colorizing of the pretty method name is here:

$this->printer->print($test->test()->testDox()->prettifiedMethodName($this->colors) . PHP_EOL);

what do you think about moving - the colorization part only - out of the TestDox class and move it instead in the only class which uses it?

@sebastianbergmann
Copy link
Owner

Makes sense, I think.

This would mean, though, that the optional $colorize parameter of TestDox::prettifiedMethodName(bool $colorize = false) would no longer have an effect as the value TestDox object would only have the prettified method name without colors. This would be a (very tiny) break of backward compatibility.

I am all for implementing this in the next major version. I am still pondering whether this break of backward compatibility could be considered negligible, allowing us to do it in the next patch release.

@staabm staabm restored the lazy-pretty branch March 11, 2024 07:54
@staabm
Copy link
Contributor Author

staabm commented Mar 11, 2024

did some research on github.com and I was not able to find a single project which call TestDox->prettifiedMethodName(true) (except phpunit forks)

@sebastianbergmann sebastianbergmann added feature/testdox The TextDox printer/formatter type/performance Issues related to resource consumption (time and memory) version/10 Something affects PHPUnit 10 version/11 Something affects PHPUnit 11 labels Mar 12, 2024
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) version/10 Something affects PHPUnit 10 version/11 Something affects PHPUnit 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants