-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Revamp the machine-readable (JSON) end-of-test summary #5338
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
b05cddb to
aee0f9e
Compare
ffcd44b to
d6df1f0
Compare
758c9f4 to
e06e637
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 got this panic if I run with the following command:
./k6sum run --summary-export=summary.json --disable-new-machine-readable-summary=false ./examples/http_get.js
running (00m00.4s), 0/1 VUs, 1 complete and 0 interrupted iterations
default ✓ [======================================] 1 VUs 00m00.4s/10m0s 1/1 iters, 1 per VU
ERRO[0000] unexpected k6 panic: runtime error: invalid memory address or nil pointer dereference
goroutine 1 [running]:
runtime/debug.Stack()
/home/codebien/go/pkg/mod/golang.org/[email protected]/src/runtime/debug/stack.go:26 +0x5e
go.k6.io/k6/internal/cmd.(*rootCommand).execute.func2()
/home/codebien/code/grafana/k6/internal/cmd/root.go:140 +0x67
panic({0x1b22d00?, 0x31d3cf0?})
/home/codebien/go/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:792 +0x132
go.k6.io/k6/internal/lib/summary.machineReadableSummaryResultsChecksBuilder(0xc00013c120)
/home/codebien/code/grafana/k6/internal/lib/summary/machine_readable.go:55 +0x86
go.k6.io/k6/internal/lib/summary.machineReadableSummaryResultsBuilder(0xc00013c120)
✅ EDIT(@joanlopez): Fixed in 0ff344a
| push: | ||
| branches: | ||
| - master | ||
| pull_request: |
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.
Instead of doing it on each pull request, should we do only on the ones directly applying changes to lib/summary/... package?
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.
Yes, we can. Although it's super cheap and fast, so not sure if it really worth it.
internal/cmd/runtime_options.go
Outdated
| "output the end-of-test summary report to JSON file", | ||
| ) | ||
| // TODO(@joanlopez): remove by k6 v2.0, once the new summary model is the default and the only one. | ||
| flags.Bool("disable-new-machine-readable-summary", true, "disables the new machine-readable summary, "+ |
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'm confused here. The PR's description says it is an opt-in feature. But here it sounds an opt-out feature. Is the description outdated?
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.
The feature is opt-in, because it's disabled by default.
I think what's confusing you is that the existing option, which is enabled by default, is to disable the feature. In other words, what the highlighted line says is disabled=true.
I intentionally did choose this approach because:
- it's opt-in, to avoid breaking changes
- the action you do is "disable", because semantically it reflects better this will be the new standard/default by k6 v2.0.
But if there's consensus among reviewers that's better to add a flag to enable it, disabled by default, then I'll be happy to change it.
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.
the action you do is "disable", because semantically it reflects better this will be the new standard/default by k6 v2.0.
My 2cents, I don't think the flag is the best way to communicate it. If we want to warn about it then we can log a warning line + clearly document on the docs.
I'd prefer if the flag reflects the current status of what it does. If I have it: I get a new summary format, if I don't set it then I get the old one.
Ah, nice catch! ^^ That's something I first considered (checking if |
oleiade
left a comment
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.
Took it for a good spin and reviewed commit by commit (which was really easy, and I appreciate it!).
Don't have many comments besides: this is really great work @joanlopez 👏🏻 🚀
I'm sure there might be things I've missed, but from my perspective, this is good to go 🙇🏻
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.
Nice work 🚀
PS: There are usual CI failures. One of them is "TestNavigationSpanCreation," which #5357 fixes (soon: on review).
Notes for the reviewers:
What?
Adds a new (opt-in) data format (machine-readable), for the programmatic shapes of the end-of-test summary (
--summary-exportandhandleSummary()), based on https://github.com/grafana/k6-summary and the human-readable summary introduced in #4089.Why?
Because the initial plan of revamping the end-of-test summary according to the captured user needs also included the programmatic shapes of it, although those were discarded from the first iteration (#4089), for agility and also to smooth the transition for users.
Indeed, that's why this new data format is disabled by default (until k6 v2), to avoid breaking changes until the new major and to let users gradually shift.
Summary of changes
internal/lib/summary/machinereadableand.github/workflows/summary-code-generation.yml.cog-generated code, with the current (latest) version of the k6-summary schema. Basically all the Go code underinternal/lib/summary/machinereadable.--summary-exportandhandleSummary()).internal/lib/summary/machine_readable.goand the changes ininternal/js/runner.go.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 #4871
Contributes to #4872 and #4803.
Copilot summary
This pull request introduces support for toggling the new machine-readable summary feature in k6 via a CLI flag and environment variable, and adds a GitHub Actions workflow to ensure generated summary code is kept up-to-date. The most significant changes include the addition of the
--new-machine-readable-summaryflag, propagation of its value through runtime options, and comprehensive test coverage for the new flag.Feature: Machine-readable summary toggle
--new-machine-readable-summaryCLI flag and correspondingK6_NEW_MACHINE_READABLE_SUMMARYenvironment variable, allowing users to enable or disable the new summary format. This is intended as a transitional feature until the new summary becomes the default. (internal/cmd/runtime_options.go,internal/cmd/run.go) [1] [2] [3]summaryMetastruct to the summary handler. (internal/cmd/run.go) [1] [2] [3]Testing and validation
internal/cmd/runtime_options_test.go) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20] [21] [22]CI/CD improvements
.github/workflows/summary-code-generation.yml) to check that summary code generation scripts have been run and all generated code is committed, preventing out-of-date generated files from being merged.Dependency and code organization
cloudoutput package and the standard libraryslicespackage to support summary metadata and output detection. (internal/cmd/run.go) [1] [2]