Skip to content

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

Merged
merged 13 commits into from
Jun 17, 2025
Merged

Conversation

the-it
Copy link
Contributor

@the-it the-it commented May 22, 2025

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:

import { group } from 'k6';

export default function () {
	group('E', function () {});
	group('D', function () {});
	group('C', function () {
		group('B', function () {
			group('A', function () {
				group('too much nesting', function () {
					// ...
				});
			});
		});
	});
}

the current output in v1.0.0 looks like this:

  █ GROUP: C 

    ↳ GROUP: B 

    ↳ GROUP: A 

    ↳ GROUP: too much nesting 


  █ GROUP: D 


  █ GROUP: E

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.

  █ GROUP: E


  █ GROUP: D


  █ GROUP: C

    ↳ GROUP: B

      ↳ GROUP: A

        ↳ GROUP: too much nesting 

Checklist

  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added tests for my changes.
  • I have run linter and tests locally (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.

  • I have added the correct milestone and labels to the PR.
  • I have updated the release notes: link
  • I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
  • I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

Closes #4724

@the-it the-it marked this pull request as ready for review May 22, 2025 17:15
@the-it the-it requested a review from a team as a code owner May 22, 2025 17:15
@the-it the-it requested review from mstoykov and joanlopez and removed request for a team May 22, 2025 17:15
@the-it the-it changed the title esommer/fix_summary_order Fix order of groups in the full summary May 22, 2025
@@ -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) : []),
Copy link
Contributor

@joanlopez joanlopez Jun 13, 2025

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.

@joanlopez joanlopez force-pushed the esommer/fix_summary_order branch from 0ba122a to 03da452 Compare June 13, 2025 08:41
joanlopez
joanlopez previously approved these changes Jun 13, 2025
@joanlopez joanlopez self-assigned this Jun 13, 2025
@joanlopez joanlopez added the ux label Jun 13, 2025
@joanlopez joanlopez added this to the v1.1.0 milestone Jun 13, 2025
Copy link
Contributor

@mstoykov mstoykov left a 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

@joanlopez joanlopez force-pushed the esommer/fix_summary_order branch from 856648a to 81d4a29 Compare June 16, 2025 10:52
mstoykov
mstoykov previously approved these changes Jun 16, 2025
Copy link
Contributor

@mstoykov mstoykov left a 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

@joanlopez joanlopez merged commit 15450dd into master Jun 17, 2025
46 of 48 checks passed
@joanlopez joanlopez deleted the esommer/fix_summary_order branch June 17, 2025 08:05
@TheEisbaer
Copy link

TheEisbaer commented Aug 14, 2025

Cannot confirm this as working with typescript :(
@mstoykov @joanlopez

mstoykov added a commit that referenced this pull request Aug 15, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--summary-mode=full outputs results by group name in alphabetical order
4 participants