-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Revamp the end-of-test summary #4089
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
Conversation
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
Co-authored-by: oleiade <theo@crevon.me>
58138ac
to
126a188
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done one more pass, and given the time restraint (and the size of a bunch of the code) and that it is mostly internal - I am okay with merging this as is.
Please do move lib/summary.go
under internal package - maybe internal/lib
just to get it out of the way - although having a summary
package is probably a good idea.
For the record this was not used last I checked during the move packages that are not used in internal
- just it is part of lib
package which is imported a lot.
bfdbc98
to
3404474
Compare
7e4f74a
to
0dfdf7e
Compare
0dfdf7e
to
4fc0440
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not forget to squash this 😅
Since we don't have a convention (or a strong one? 🤔), and personally I feel like what's atomic and what not is blurry, ambiguous and subjective, I generally squash all my PRs, so it would be very weird that specially for this one I misclick on "Merge", but thanks for the reminder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work! 🚀
Thinking loudly: it seems like the summary mode could also take responsibility of displaying no summary, which makes the UX more straightforward, but it's probably not a big deal
Contributes to #4052
Overview
This pull request changes the current end-of-test summary by a new design, with two available formats:
aiming to bring clearer a more valuable results to users. Find a screenshot below.
User-facing details
compact
summary with--summary-mode=compact
, or with no argument, as it is the default choice.full
summary with--summary-mode=full
.legacy
summary with--summary-mode=legacy
.handleSummary
function is now different. So, those users relying on it must migrate their implementation or use--summary-mode=legacy
meanwhile.Technical details (for review purposes)
output.Output
namedsummary
.internal/cmd/testdata/summary/...
with different scenarios, groups, thresholds, custom metrics and what not... that can be used for both automated and manual testing. If you think anything is missing, just suggest it.lib.Summary
vslib.LegacySummary
), to keep support for the legacy summary for some time, in an easy way, so things aren't complexity mixed and the the clean up in the future is simpler, just by removing that type, all the references to it, and simplifying the few conditionals that behave depending on which summary type is provided.summary-legacy.js
, for simpler cleanup whenever we remove that support, which I guess might be for v2 (once we ship the formalized JSON output format within v1).Internal Checklist
Before review readiness
lib.Report
as the new API for customhandleSummary
(note this would be a breaking change), or if we want to ship this progressively.js/summary.go
according to that decision.js/summary.js
:summarizeMetrics
andsummarizeMetricsWithThresholds
, or just replace the first with the second one (as we may no longer need the first one if we remove the old summary code).output/summary
package:playground/full-summary
files, or define a proper location for them.--- Ideas left for a second iteration---
General
make lint
) and all checks pass.make tests
) and all tests pass.