Remove most direct access to the os package, support NO_COLOR and K6_NO_COLOR

[na--]

This wasn't quite part of the plan, but several hours of yak shaving produced some nice results 😅 Anyway, cmd/ looks much nicer and it's now finally safe to test with basically full internal integration tests 🎉

That said, there is a very high chance of bugs and errors, since we don't yet have full test coverage of cmd/... 😞 So a careful review and PRs that add more tests are going to be quite welcome 🙇 Ideally, we should try to merge this at the beginning of the v0.38.0 cycle, so we also have the maximum time to safely hit any bugs during our manual tests and work...

And I think there were actually some (potential) bugs that were fixed by this refactor. For example, these loggers:

k6/cmd/root.go

Lines 163 to 175 in 1e28a3e

	logger := &logrus.Logger{
	Out: os.Stderr,
	Formatter: new(logrus.TextFormatter),
	Hooks: make(logrus.LevelHooks),
	Level: logrus.InfoLevel,
	}
	var fallbackLogger logrus.FieldLogger = &logrus.Logger{
	Out: os.Stderr,
	Formatter: new(logrus.TextFormatter),
	Hooks: make(logrus.LevelHooks),
	Level: logrus.InfoLevel,
	}

Did not actually use the wrapped stdout that is protected by a mutex:

k6/cmd/root.go

Line 97 in 1e28a3e

stderr: &consoleWriter{colorable.NewColorableStderr(), stderrTTY, outMutex, nil},

With the last commit, this PR also closes #2091 🎉

[na--]

Forgot that this PR has a minor breaking change. I renamed the --logformat CLI flag to --log-format, to more closely match --log-output. The old flag still works, it just emits a deprecation warning.

Also, I noticed that in the docs, we advertise that the option can be configured through K6_LOGFORMAT, but as far as I can see, that has never been the case before now, only the CLI flag was usable. Anyway, the now we have support for env vars, though the name is K6_LOG_FORMAT, again to match K6_LOG_OUTPUT.

[na--]

With the last commit I had to fix a minor issue where an error in the CLI flags (the parsing of which happens before setupLoggers()) will result in an ugly log message with the format we use for non-TTY terminals, even if a user is using a TTY. As a nice side-effect, I also added a few lines of code to close #2091 😅

[oleiade]

Experimenting with this PR, and using both the K6_NO_COLOR= and NO_COLOR= statements, the colors don't seem to be disabled :-/

I might have missed something though 😸

[oleiade]

I will wait for clarification on the no color options not working on my sides. Other than that, I've spotted a few places that could use documentation. Thumbs up otherwise, it's a nice change 🎉

PS: 👎🏻 on working on the weekend though ;)

[na--]

Fixed support for NO_COLOR and K6_NO_COLOR with 5e14510 🤦‍♂️ I can't wait to finally fix #883 once and for all. This PR is a nice step in that direction, but as it's obvious from this commit, it's still soooo easy to make minor mistakes that silently break something 😞

[oleiade]

It works 🎉

[codebien]

For now only just one suggestion.

and PRs that add more tests are going to be quite welcome

I can try to support here, which part did you see very fragile and you would get some tests?

[na--]

and PRs that add more tests are going to be quite welcome

I can try to support here, which part did you see very fragile and you would get some tests?

It will probably be better to add any tests on top of #2412, it's much cleaner and easier to add them there. And it's not that I see the changes in these PRs as fragile, quite the opposite, they are much more robust than the previous code. The problem is that before #2412, it was just so flaky and unpractical to test logic in cmd/ that we had very few tests. Now, it's easy to add a test for pretty much any behavior 😞

See this file for some simple tests I did in the other PR.

[codebien]

and it's not that I see the changes in these PRs as fragile

I meant which part of the code we think it's more exposed to be broken in the future if we wouldn't add tests for it. I didn't mean that the current changes are fragile.

Btw, I will add some on top of #2412, as you suggested, if I will identify something useful by reviewing and testing the PRs.

[codebien]

LGTM

[na--]

I'm honestly not sure which part would benefit the most 😅 Maybe we can look at the code coverage by just running the existing cmd tests in #2412 and go from there? 🤔

[oleiade]

🎉 👍🏻

[olegbespalov]

General comment, what is the idea behind globalState? Is it a temporary thing, because to be honest, it seems scary since we are coupeling many of the structs with the global state? 🤔

[na--]

General comment, what is the idea behind globalState? Is it a temporary thing, because to be honest, it seems scary since we are coupeling many of the structs with the global state? 🤔

It is scary, but the question is "is is scarier than everything directly touching the os package"? And to that, I'd answer with a very emphatic "no" 😅

I have some ideas for further improvements here, but so far this seems to work out very well and unlock very nice integration testing, see #2412 for the follow-up 🤷‍♂️

[olegbespalov]

is is scarier than everything directly touching the os package

My concern is that globalState aggregates not only the functionality of the os package interactions but also many other things.

[na--]

My concern is that globalState aggregates not only the functionality of the os package interactions but also many other things.

k6/cmd/root.go

Lines 80 to 100 in f82b364

	type globalState struct {
	ctx context.Context
	fs afero.Fs
	getwd func() (string, error)
	args []string
	envVars map[string]string
	defaultFlags, flags globalFlags
	outMutex *sync.Mutex
	stdOut, stdErr *consoleWriter
	stdIn io.Reader
	osExit func(int)
	signalNotify func(chan<- os.Signal, ...os.Signal)
	signalStop func(chan<- os.Signal)
	logger *logrus.Logger
	fallbackLogger logrus.FieldLogger
	}

@olegbespalov, "many other things" seems like a bit of an exaggeration 😕 Out of these 16 things, 11 are os proxies directly. The defaultFlags and flags directly derived from os values and defaultFlags will hopefully disappear once we have croconf. The remaining 3 are in ctx, logger and fallbackLogger and are there purely for convenience. We'll have to pass them everywhere we pass globalState anyway, so why not include them in the struct?

[na--]

@olegbespalov, any other comments or more specific suggestions?

[mstoykov]

LGTM as much as it can as that code is always hairy 😬.

I would've preferred more new tests for what previously wasn't testable.

[na--]

@mstoykov, re: more tests, it's still not super easy to write them in this PR, but check out #2412 - it makes it much easier and adds several integration tests

[oleiade]

🎉 🍾 🍪