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

Report global maximum duration only once #396

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Dec 3, 2023

#657 must be merged first

replaces #379

fixes #643

Now the global maximum duration is still reported, but only once - in the report header. It is now also clearer what the number in the parenthesis means, as the header describes it.

It the items list, only the non-global maximum durations are shown. This makes the non-global maximum duration to stand out visually.

@mvorisek mvorisek marked this pull request as ready for review December 3, 2023 15:33
Copy link

codecov bot commented Dec 3, 2023

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.76%. Comparing base (27aacf7) to head (115f795).

Files with missing lines Patch % Lines
src/Reporter/DefaultReporter.php 94.44% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #396      +/-   ##
============================================
+ Coverage     56.14%   56.76%   +0.62%     
- Complexity      117      119       +2     
============================================
  Files            28       28              
  Lines           675      687      +12     
============================================
+ Hits            379      390      +11     
- Misses          296      297       +1     

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

@localheinz
Copy link
Member

@mvorisek

Can you explain what problem you are trying to solve with this and #379?

@mvorisek

This comment was marked as off-topic.

@localheinz
Copy link
Member

@mvorisek

No, that's #380 - this pull request here and #379 are about changing the output.

@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 3, 2023

My apologies.

In this PR I improved the UX of the output. Please see the PR description. It deduplicates the global max. duration value in the output. And as coded newly, the value in the parenthesis now makes more sense in the header, as the header mentions what the value is about. This is also how https://github.com/johnkary/phpunit-speedtrap was/is coded.

@localheinz
Copy link
Member

@mvorisek

Perhaps it would make more sense to allow configuring the reporter?

This way everyone can use the reporter (and thus, the format of the report) of their choice.

What do you think?

@mvorisek mvorisek force-pushed the report_global_max_duration_only_once branch 5 times, most recently from ffe4b33 to c2f4300 Compare December 14, 2023 12:18
@mvorisek
Copy link
Contributor Author

PR is done and rebased on the latest main.

Perhaps it would make more sense to allow configuring the reporter?

Deduplicated/simpler to read report is better for UX. IMHO no configuration option is needed, it would only complicate the code.

@mvorisek mvorisek force-pushed the report_global_max_duration_only_once branch from c2f4300 to f937e1e Compare December 17, 2023 01:09
@mvorisek
Copy link
Contributor Author

@localheinz default/native PHPUnit duration formatter always shows minutes - https://github.com/sebastianbergmann/php-timer/blob/6.0.0/src/Duration.php#L81 - I tried to unify the format with this one, but the results were too complex. Then I tried to format times below 1 minute with s/ms and the results are, in my opinion, very readable and the whole output feels very natural. Wdyt?

@mvorisek mvorisek force-pushed the report_global_max_duration_only_once branch from 7fe92b0 to 06ef858 Compare December 17, 2023 20:49
@localheinz localheinz self-assigned this Dec 21, 2023
@mvorisek
Copy link
Contributor Author

@localheinz this is purely an UX change. I want your green light here in order to rebase. If you, for some reasons, do not want this improvement, please share the reasons with me and we can find a common ground or even close this PR. Thanks!

@localheinz
Copy link
Member

Apologies for the delay, @mvorisek - please rebase!

@mvorisek mvorisek force-pushed the report_global_max_duration_only_once branch from 06ef858 to 115f795 Compare February 14, 2025 10:28
@mvorisek mvorisek marked this pull request as draft February 14, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

what does the output mean?
2 participants