-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix order of groups in the full summary #4807
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
…nternal/output/summary/summary.go
Co-authored-by: Erik Sommer <erik.sommer@grafana.com>
internal/js/summary.js
Outdated
@@ -385,7 +387,7 @@ class ReportBuilder { | |||
return [ | |||
...this._renderChecks(group.checks, renderContext), | |||
...this._renderMetrics(group.metrics, renderContext), | |||
...(group.groups ? this._renderNestedGroups(group.groups) : []), | |||
...(group.groups ? this._renderNestedGroups(group.groups, group.groups_order, renderContext) : []), |
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.
Jfyi, the missing renderContext
here is what was making nested groups to not be properly indented.
0ba122a
to
03da452
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.
Everything else looks good, but I would prefer if the rendering code already gets the groups in order instead of having to provide it with the groups and the order and then make it do it.
This likely will be even more helpful when #4803 is being worked on
856648a
to
81d4a29
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.
LGTM! good job
I am not certain if there wasn't a left over argument - I have left a comment
81d4a29
to
14423ba
Compare
Cannot confirm this as working with typescript :( |
In #4807 the order was fixed when there were no scenarios, but due to how the code structured this didn't work for scenarios. Here we basically do the same change for the scenarios.
What?
It changes the sorting algorithm for printing groups (and their metrics) in the end-of-test summary (full), from alphabetically, to preserving the same order as declared in the code, as discussed in #4724 and historically implemented as part of #1316.
It also fixes a bug with nesting groups within groups, that right now aren't correctly padded to the right.
Why?
Because preserving the same order as declared in the code is something that was discussed in the past (see #1316), and implemented, and thus it makes all sense to keep the new end of test
summary following the same principle (the same rationale remains).
Example
When executing a script like this:
the current output in v1.0.0 looks like this:
where we have the groups ordered alphabetically (at least on the top level) and the indentation of the inner groups isn't applied consistently.
With change on this PR, it will preserve the order of the groups in the output and will indent inner groups consistently, regardless of how much nesting there is.
Checklist
make check
) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #4724