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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Dec 3, 2023

replaces #379

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: 1 lines in your changes are missing coverage. Please review.

Comparison is base (99464cc) 70.48% compared to head (06ef858) 70.85%.

Files Patch % Lines
src/Reporter/DefaultReporter.php 94.44% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #396      +/-   ##
============================================
+ Coverage     70.48%   70.85%   +0.37%     
- Complexity      110      112       +2     
============================================
  Files            26       26              
  Lines           515      525      +10     
============================================
+ Hits            363      372       +9     
- Misses          152      153       +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?

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.

2 participants