Config refactoring and tests #935
Conversation
This is woefully insufficient, but even that was painful to write... Further refactoring and tests will follow, especially relating to JSON/JS configs and mixing configuration between the CLI/env-vars/JS/JSON layers. The effort will hopefully be worth it, since we should be able to reuse these tests to verify that we're not breaking things when we finally tackle #883 properly.
Codecov Report
@@ Coverage Diff @@
## scheduler-config-wip #935 +/- ##
========================================================
+ Coverage 70.73% 71.64% +0.91%
========================================================
Files 127 127
Lines 9452 9467 +15
========================================================
+ Hits 6686 6783 +97
+ Misses 2355 2269 -86
- Partials 411 415 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## scheduler-config-wip #935 +/- ##
========================================================
+ Coverage 70.73% 71.86% +1.12%
========================================================
Files 127 128 +1
Lines 9452 9482 +30
========================================================
+ Hits 6686 6814 +128
+ Misses 2355 2256 -99
- Partials 411 412 +1
Continue to review full report at Codecov.
|
cmd/cloud.go
Outdated
| flags.SortFlags = false | ||
| flags.AddFlagSet(optionFlagSet()) | ||
| flags.AddFlagSet(runtimeOptionFlagSet(false)) | ||
| //TODO: figure out a better way to handle the CLI flags - global variables are not very testable... :/ |
There was a problem hiding this comment.
Well you can use local variables but you will need to not use init :) . I suppose at some point we will rewrite it but this seems as too small of a problem atm
There was a problem hiding this comment.
It likely needs to be rewritten as we tackle the big config refactoring... Using global variables is somewhat fine, until you need to test them. And it can hide issues like the fact we currently somehow have 2 different variables for saving the value of the --config flag (details).
But in my head, we have at least 2 other similar issues that are more problematic than this, both in terms of improving test coverage and correctness, and in their potential for hiding bugs...
The first problem is the liberal use of os.GetEnv() to read environment variables directly into those same global variables. grep -FR --exclude-dir=vendor "os.Getenv" produces
./cmd/config.go:var configFile = os.Getenv("K6_CONFIG") // overridden by `-c` flag!
./cmd/run.go: runType = os.Getenv("K6_TYPE")
./cmd/run.go: runNoSetup = os.Getenv("K6_NO_SETUP") != ""
./cmd/run.go: runNoTeardown = os.Getenv("K6_NO_TEARDOWN") != ""
./cmd/cloud.go: exitOnRunning = os.Getenv("K6_EXIT_ON_RUNNING") != ""
That's on top of the other issues we also have due to using envconfig... Ideally, we shouldn't directly call os.Getenv() or other os functions more than once. Instead, we should just capture the environment variables once per run, in a single place in the code, and then pass them through to the configuration parsing and setting logic. That way we can actually mock the process and write simple and sensible tests that are able to check all of the configuration parsing and management logic...
The second problem lies in the way we're dealing with the CLI flags that don't save their values into global variables, i.e. the majority of the CLI flags. And I'm not even talking about how we use them to store both the default option values and the highest-priority user-supplied ones... The problem I've hit a few times recently is that a lot of the potential developer errors in setting CLI flags won't happen at compile time, but rather at runtime.
The problematic code can be seen here and here (and in a few other places as well). Because we're using string identifiers in multiple places, and we panic() if we don't have the expected flag, and we don't have tests for the different cobra.Command global variables ( i.e. runCmd, cloudCmd, etc.), minor developer errors can lead to releasing broken code, especially in rarely used commands...
So yeah, I won't fix this specific TODO with this pull request, but it and the others I mentioned above should definitely be fixed sometime soon...
For some reason, when running the tests on appveyor, the `os.Environ()` result contains the value `=C:=C:\gopath\src\github.com\loadimpact\k6` :/ ...
cmd/config_test.go
Outdated
| logHook := simpleLogrusHook{levels: []log.Level{log.WarnLevel}} | ||
| log.AddHook(&logHook) | ||
| log.SetOutput(ioutil.Discard) | ||
| defer log.SetOutput(os.Stderr) |
There was a problem hiding this comment.
This once again makes me think we should not just be calling some global log.Error functions everywhere
There was a problem hiding this comment.
Probably... And to be fair, we have some logrus instances we pass in some places. For example, each VU has its own logger IIRC. But far from enough.
And this usually isn't a huge deal for tests, it just makes reading the debug output when a test fails very messy... So I'd classify it in the "nice to have" category of things I'd like to fix 😑
This PR is to the branch in #913, not
master.It is woefully insufficient, but even this much was painful to write... Further refactoring and tests will follow, especially relating to JSON/JS configs and mixing configuration between the CLI/env-vars/JS/JSON layers. The effort will hopefully be worth it, since we should be able to reuse these tests to verify that we're not breaking things when we finally tackle #883 properly.
Edit: turns out that this PR actually inadvertently fixed an old issue: #864