Fix color handling, generate the summary with JS, improve CI #1975

na-- · 2021-04-16T19:20:42Z

Read commit by commit. This should be merged after we release k6 v0.32.0, I'm just polishing it and pushing it, so it's easy to merge after the release.

This PR should fix #1806 and close #1805, as well as make several other minor improvements, mostly about various dependencies (#1933).

It also introduces a minor breaking change, though I'm not sure if it's even worth noting in the release notes... k6 will no longer highlight the output of several sub-commands: pause, resume, scale, stats, and status.

Finally, because I wanted to use the new file embedding capabilities in Go 1.16, I bumped the minimum required version to that. I switched the xk6 tests to run with the latest stable Go version and the tip. For the main test suite, I temporarily disabled the 1.15 tests, but also added testing with the tip (which is non-blocking, so it won't prevent a PR merge or a release). This shouldn't be a problem, since we will only release this in k6 v0.33.0, ~2.5 months from now. But it gave me the opportunity to also drop the go.rice dependency 🎉

codecov-commenter · 2021-04-16T19:53:36Z

Codecov Report

Merging #1975 (6778f12) into master (daca27c) will decrease coverage by 0.20%.
The diff coverage is 12.72%.

❗ Current head 6778f12 differs from pull request most recent head 0dd3012. Consider uploading reports for the commit 0dd3012 to get more accurate results

@@            Coverage Diff             @@
##           master    #1975      +/-   ##
==========================================
- Coverage   71.92%   71.72%   -0.21%     
==========================================
  Files         182      179       -3     
  Lines       14291    14085     -206     
==========================================
- Hits        10279    10102     -177     
+ Misses       3368     3346      -22     
+ Partials      644      637       -7

Flag	Coverage Δ
ubuntu	`71.66% <12.72%> (-0.21%)`	⬇️
windows	`71.47% <12.72%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/cloud.go	`4.73% <0.00%> (-0.11%)`	⬇️
cmd/login_cloud.go	`0.00% <0.00%> (ø)`
cmd/pause.go	`0.00% <0.00%> (ø)`
cmd/resume.go	`0.00% <0.00%> (ø)`
cmd/root.go	`8.82% <0.00%> (+0.06%)`	⬆️
cmd/run.go	`12.14% <0.00%> (-0.26%)`	⬇️
cmd/scale.go	`0.00% <0.00%> (ø)`
cmd/stats.go	`0.00% <0.00%> (ø)`
cmd/status.go	`0.00% <0.00%> (ø)`
cmd/ui.go	`20.61% <0.00%> (-1.99%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daca27c...0dd3012. Read the comment docs.

mstoykov

Seems fine on first read, but we need to test this a lot ... especially on windows

mstoykov · 2021-04-19T07:37:42Z

cmd/status.go

-			ui.Dump(stdout, status)
-			return nil
+
+			return yamlPrint(stdout, status)


Maybe this is a good case for wrapping errors as "could not marshal YAML:" will be quite strange error for someone calling k6 pause or co.

Yeah, though to be fair, the new state isn't any worse than the previous behavior where it would have printed just the raw YAML marshaling error, without even the could not marshal YAML: in front of it 😅 but yeah, I may wrap these errors locally as well...

na-- · 2021-04-20T07:38:51Z

I converted this back to draft because I want to make a few more changes, prompted by grafana/jslib.k6.io#32

First, I think I should rename the uiState sent to handleSummary() to the more generic state, and have it include things like the total test run duration. It is currently in the summary, just not exposed in the JS object directly: https://github.com/k6io/k6/blob/47a0b493cce0020d445a47edbca79c25ab5a550f/lib/runner.go#L110

Also, AreColorsDisabled (i.e. NoColors) should probably not be in data.state and should instead be moved to the data.options.

If the environment contains TERM=dumb, we don't consider them to be a TTY. This is in line with what the github.com/fatih/color library checks: https://github.com/fatih/color/blob/v1.10.0/color.go#L20-L21

mstoykov

LGTM, I had a couple of nitpicks, but if it works .... I am fine with merging it and my 5 minutes of testing didn't show any problems so 👍

mstoykov · 2021-05-17T13:41:13Z

cmd/cloud.go

@@ -330,7 +333,8 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
 				return ExitCode{error: errors.New("Test progress error"), Code: cloudFailedToGetProgressErrorCode}
 			}

-			fprintf(stdout, "     test status: %s\n", ui.ValueColor.Sprint(testProgress.RunStatusText))
+			valueColor := getColor(noColor || !stdoutTTY, color.FgCyan)


Given how now you repeat color.FgCyan a lot I would argue it should probably be a constant somewhere ... probably named valueColorColor :P

It's already a constant 😛 It's used in like 5 places across 2 modules, and not always for values (e.g. the banner), so I don't want to define a constant for that.

js/summary.go

imiric

No blockers from me, just minor suggestions. I would've preferred to review separate PRs for the color fixes and JS summary changes, but the commits are isolated, so it wasn't a big issue.

I think we're missing some integration tests to ensure color is handled correctly, but it could be added later. Though it worked fine in my manual tests. It's kind of wild we're generating the summary entirely in JS now 😄 🎉

And kudos for dropping all those Go dependencies! 👍

cmd/ui.go

imiric · 2021-05-17T12:52:03Z

cmd/ui.go

+// getColor() returns the requested color, or an uncolored object, depending on
+// the value of noColor. The explicit EnableColor() and DisableColor() are
+// needed because the library checks os.Stdout itself otherwise...
+func getColor(noColor bool, attributes ...color.Attribute) *color.Color {


Could you add high-level (integration?) tests for this, to check the new behavior, using TERM=dumb, etc.?

This is very difficult to do currently with all of these globals, I think it falls under #342 and #883. The end goal is to almost completely remove the globals, i.e. have something that passes os.Environ() and the CLI flags as parameters, which will allow us to have sane tests as well.

I meant high-level as in user acceptance tests, similar to the xk6 ones we have now, running k6 itself in a shell, checking the output, etc.

But that requires coming up with the framework and CI pipeline, so it's fine if we do it later as part of #342.

js/summary.go

.github/workflows/all.yml

js/summary.go

imiric · 2021-05-17T13:35:52Z

js/summary.go

 )

-// TODO: move this to a separate JS file and use go.rice to embed it
-const summaryWrapperLambdaCode = `
+// Copied from https://github.com/k6io/jslib.k6.io/tree/master/lib/k6-summary


This means we can deprecate the jslib, right?

No? jslib has a ton of other libraries. Besides, having textSummary() in there is very helpful for users that want to define their own handleSummary() function, e.g. to emit a json or a junit file, but they also want to continue getting the text summary at the end of the test. They can just import { textSummary } from 'https://jslib.k6.io/lib/k6-summary/0.0.1/index.js' and use that in handleSummary()'s result.

I didn't mean deprecating jslib.k6.io altogether 😄, just this lib.

I wasn't aware there were differences between the version committed here and the one on jslib. Though since we already bundle it in the binary, couldn't we expose it as a built-in JS module and avoid the remote loading? 🤔

There aren't any differences, except we don't bundle the JUnit code (we'll probably split it apart in jslib as well soon). And we can potentially expose it as a JS module, if we figure out a nice module name, but that will be a separate issue/PR

We still use the noColor and stdoutTTY global variables, but we explicitly pass them as function arguments, so it's clear which parts of the code depend on them when we eventually fully refactor them into locals.

The affected commands are k6 pause, resume, scale, stats and status. This allows us to remove the github.com/zyedidia/highlight dependency and significantly simplify the color management mess.

Due to incomplete test setup, there were differences between the expected text and JSON results of the end-of-test summary handling...

This is necessary so that any handleSummary() callback, including the default one, would know if it can use colors (or other special terminal features) or not.

This is needed because we want to use the file embedding API.

This allows us to drop the github.com/dustin/go-humanize and also the explicit nature of the golang.org/x/text dependency.

This should no longer be needed, since we hopefully don't have any more rogue color generators that don't respect stdoutTTY/stderrTTY and noColor. The next step would be making these variables local and passing them through the cmd logic as arguments, not reading their values directly out of the global scope.

imiric

LGTM, just a couple of minor notes.

imiric · 2021-05-18T08:42:41Z

js/runner.go

@@ -344,7 +345,8 @@ func (r *Runner) HandleSummary(ctx context.Context, summary *lib.Summary) (map[s
 	}()
 	*vu.Context = ctx

-	handleSummaryWrapperRaw, err := vu.Runtime.RunString(summaryWrapperLambdaCode)
+	wrapper := strings.Replace(summaryWrapperLambdaCode, "/*JSLIB_SUMMARY_CODE*/", jslibSummaryCode, 1)


I'm probably missing it, but what's the reason for having a wrapper and doing this replace? Since both summaryWrapperLambdaCode and jslibSummaryCode are static, couldn't we just bundle a single JS file?

We probably could, but this insulates us from future changes in the jslib summary code, and it also keeps the scope a bit cleaner 🤷‍♂️

Ah, sorry, misread your question. I don't want to bundle a single JS file since I want it to be easy to update the summary.js when the one in jslib is updated

imiric · 2021-05-18T08:49:29Z

stats/stats_test.go

@@ -29,120 +29,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )

-func TestMetricHumanizeValue(t *testing.T) {


These were very important tests IMO that we lose in the conversion to JS. We should probably add some for the JS implementation.

I replicated (and IIRC even expanded a bit) these tests in https://github.com/k6io/jslib.k6.io/blob/master/tests/summary.js when I was writing the JS summary generation code.

Ah, neat, I forgot about those. I suppose it's fine then, though running them as part of the k6 tests would give us more confidence, as they're run more frequently and use the latest k6 version. But this goes back to the point of eventually deprecating the jslib version, which we can consider later.

In k6 we have somewhat higher-level tests in https://github.com/k6io/k6/blob/master/js/summary_test.go - they are like integration tests for the JS summary code, so I'd rather expand those than to write more low-level ones.

na-- added this to the v0.33.0 milestone Apr 16, 2021

na-- changed the title ~~[WIP] Fix colors switch to js summary~~ [WIP] Fix color handling and generate the summary with JS Apr 16, 2021

na-- force-pushed the fix-colors-switch-to-js-summary branch from 2c61abd to ab62876 Compare April 16, 2021 20:13

na-- changed the title ~~[WIP] Fix color handling and generate the summary with JS~~ [WIP] Fix color handling, generate the summary with JS, improve CI Apr 16, 2021

na-- force-pushed the fix-colors-switch-to-js-summary branch from 45cdd2e to 3ff9787 Compare April 16, 2021 20:31

na-- changed the title ~~[WIP] Fix color handling, generate the summary with JS, improve CI~~ Fix color handling, generate the summary with JS, improve CI Apr 16, 2021

na-- marked this pull request as ready for review April 16, 2021 20:51

mstoykov reviewed Apr 19, 2021

View reviewed changes

na-- mentioned this pull request Apr 20, 2021

new function that generates junit xml for checks grafana/jslib.k6.io#32

Open

na-- marked this pull request as draft April 20, 2021 07:34

na-- mentioned this pull request Apr 22, 2021

Show the number of passed checks when all the checks pass grafana/jslib.k6.io#22

Open

na-- mentioned this pull request May 10, 2021

Update go version at go.mod #2002

Closed

mstoykov mentioned this pull request May 11, 2021

k6 dependencies update policy discussion #1933

Closed

na-- force-pushed the fix-colors-switch-to-js-summary branch from cef7172 to 0aad6a6 Compare May 14, 2021 07:43

na-- added 2 commits May 14, 2021 14:12

Improve TTY detection for stdout and stderr

4037eab

If the environment contains TERM=dumb, we don't consider them to be a TTY. This is in line with what the github.com/fatih/color library checks: https://github.com/fatih/color/blob/v1.10.0/color.go#L20-L21

Differentiate between stderr and stdout TTY detection for loggers

73b4a9b

na-- marked this pull request as ready for review May 14, 2021 11:41

na-- force-pushed the fix-colors-switch-to-js-summary branch from 0aad6a6 to 25cd528 Compare May 14, 2021 11:41

na-- requested review from mstoykov and imiric May 14, 2021 11:42

mstoykov previously approved these changes May 17, 2021

View reviewed changes

imiric reviewed May 17, 2021

View reviewed changes

na-- added 6 commits May 18, 2021 10:34

Do not use global variables and state to colorize some UI elements

b02fe7b

We still use the noColor and stdoutTTY global variables, but we explicitly pass them as function arguments, so it's clear which parts of the code depend on them when we eventually fully refactor them into locals.

Do not highlight the output of k6 control subcommands

479332f

The affected commands are k6 pause, resume, scale, stats and status. This allows us to remove the github.com/zyedidia/highlight dependency and significantly simplify the color management mess.

Fix discrepancies in the the end-of-test summary tests

b6de30b

Due to incomplete test setup, there were differences between the expected text and JSON results of the end-of-test summary handling...

Propagate the current UI state through to handleSummary()

f0b96c4

This is necessary so that any handleSummary() callback, including the default one, would know if it can use colors (or other special terminal features) or not.

Add testRunDurationMs to the handleSummary() state data

580b054

Bump the minimum required Go version to 1.16 and test xk6 with tip

c03a01b

This is needed because we want to use the file embedding API.

na-- added 5 commits May 18, 2021 10:34

Run the test suite with the Go tip version as well

dad3e26

Generate the end-of-test text summary with JS instead of with Go

1c0f101

Drop the old Go code that was used for the end-of-test summary

2810d83

This allows us to drop the github.com/dustin/go-humanize and also the explicit nature of the golang.org/x/text dependency.

Use go:embed for babel, drop github.com/GeertJohan/go.rice

c7e8a7b

na-- dismissed mstoykov’s stale review via c7e8a7b May 18, 2021 07:35

na-- force-pushed the fix-colors-switch-to-js-summary branch from 25cd528 to c7e8a7b Compare May 18, 2021 07:35

mstoykov previously approved these changes May 18, 2021

View reviewed changes

Move the summary wrapper JS code to a new file and use it with go:embed

6f6bfac

na-- dismissed mstoykov’s stale review via 6f6bfac May 18, 2021 07:46

Make the CI config more robust

6778f12

na-- requested review from imiric and mstoykov May 18, 2021 07:57

mstoykov previously approved these changes May 18, 2021

View reviewed changes

Fix error typo in HandleSummary()

0dd3012

imiric dismissed mstoykov’s stale review via 0dd3012 May 18, 2021 08:26

imiric approved these changes May 18, 2021

View reviewed changes

na-- merged commit 6b635e9 into master May 18, 2021

na-- deleted the fix-colors-switch-to-js-summary branch May 18, 2021 09:33

na-- mentioned this pull request May 18, 2021

Fix TTY color support #1805

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix color handling, generate the summary with JS, improve CI #1975

Fix color handling, generate the summary with JS, improve CI #1975

na-- commented Apr 16, 2021 •

edited

Loading

codecov-commenter commented Apr 16, 2021 •

edited

Loading

mstoykov left a comment

mstoykov Apr 19, 2021

na-- Apr 19, 2021

na-- commented Apr 20, 2021

mstoykov left a comment

mstoykov May 17, 2021

na-- May 18, 2021

imiric left a comment

imiric May 17, 2021

na-- May 18, 2021

imiric May 18, 2021

imiric May 17, 2021

na-- May 18, 2021

imiric May 18, 2021

na-- May 18, 2021

imiric left a comment

imiric May 18, 2021

na-- May 18, 2021

na-- May 18, 2021

imiric May 18, 2021

na-- May 18, 2021

imiric May 18, 2021

na-- May 18, 2021

Fix color handling, generate the summary with JS, improve CI #1975

Fix color handling, generate the summary with JS, improve CI #1975

Conversation

na-- commented Apr 16, 2021 • edited Loading

codecov-commenter commented Apr 16, 2021 • edited Loading

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- commented Apr 20, 2021

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- commented Apr 16, 2021 •

edited

Loading

codecov-commenter commented Apr 16, 2021 •

edited

Loading