Refactor some CLI configs and add some basic configuration tests

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.

cmd/archive.go

@@ -25,6 +25,7 @@ import (
 
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 )
 
 var archiveOut = "archive.tar"
@@ -90,11 +91,19 @@ An archive is a fully self-contained test run, and can be executed identically e
 	},
 }
 
+func archiveCmdFlagSet() *pflag.FlagSet {
+	flags := pflag.NewFlagSet("", pflag.ContinueOnError)
+	flags.SortFlags = false
+	flags.AddFlagSet(optionFlagSet())
+	flags.AddFlagSet(runtimeOptionFlagSet(false))
+	flags.AddFlagSet(configFileFlagSet())
+	//TODO: figure out a better way to handle the CLI flags - global variables are not very testable... :/
+	flags.StringVarP(&archiveOut, "archive-out", "O", archiveOut, "archive output filename")
+	return flags
+}
+
 func init() {
 	RootCmd.AddCommand(archiveCmd)
 	archiveCmd.Flags().SortFlags = false
-	archiveCmd.Flags().AddFlagSet(optionFlagSet())
-	archiveCmd.Flags().AddFlagSet(runtimeOptionFlagSet(false))
-	archiveCmd.Flags().AddFlagSet(configFileFlagSet())
-	archiveCmd.Flags().StringVarP(&archiveOut, "archive-out", "O", archiveOut, "archive output filename")
+	archiveCmd.Flags().AddFlagSet(archiveCmdFlagSet())
 }

cmd/cloud.go

@@ -36,6 +36,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/spf13/afero"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 
 	log "github.com/sirupsen/logrus"
 )
@@ -235,10 +236,18 @@ This will execute the test on the Load Impact cloud service. Use "k6 login cloud
 	},
 }
 
+func cloudCmdFlagSet() *pflag.FlagSet {
+	flags := pflag.NewFlagSet("", pflag.ContinueOnError)
+	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... :/
+	flags.BoolVar(&exitOnRunning, "exit-on-running", exitOnRunning, "exits when test reaches the running status")
+	return flags
+}
+
 func init() {
 	RootCmd.AddCommand(cloudCmd)
 	cloudCmd.Flags().SortFlags = false
-	cloudCmd.Flags().AddFlagSet(optionFlagSet())
-	cloudCmd.Flags().AddFlagSet(runtimeOptionFlagSet(false))
-	cloudCmd.Flags().BoolVar(&exitOnRunning, "exit-on-running", exitOnRunning, "exits when test reaches the running status")
+	cloudCmd.Flags().AddFlagSet(cloudCmdFlagSet())
 }

cmd/config.go

@@ -48,6 +48,7 @@ var configFile = os.Getenv("K6_CONFIG") // overridden by `-c` flag!
 
 // configFileFlagSet returns a FlagSet that contains flags needed for specifying a config file.
 func configFileFlagSet() *pflag.FlagSet {
+	//TODO: remove?
 	flags := pflag.NewFlagSet("", 0)
 	flags.StringVarP(&configFile, "config", "c", configFile, "specify config file to read")
 	return flags

cmd/config_test.go

@@ -1,7 +1,7 @@
 /*
  *
  * k6 - a next-generation load testing tool
- * Copyright (C) 2016 Load Impact
+ * Copyright (C) 2019 Load Impact
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU Affero General Public License as
@@ -21,11 +21,25 @@
 package cmd
 
 import (
+	"fmt"
+	"io/ioutil"
 	"os"
+	"strings"
+	"sync"
 	"testing"
+	"time"
+
+	"github.com/loadimpact/k6/lib/scheduler"
+	"github.com/loadimpact/k6/lib/types"
+
+	"github.com/spf13/afero"
+	"github.com/spf13/pflag"
 
 	"github.com/kelseyhightower/envconfig"
+	"github.com/loadimpact/k6/lib"
+	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gopkg.in/guregu/null.v3"
 )
 
@@ -83,8 +97,227 @@ func TestConfigCmd(t *testing.T) {
 	}
 }
 
-//TODO: write end-to-end configuration tests - how the config file, in-script options, environment variables and CLI flags are parsed and interact... and how the end result is then propagated back into the script
+// A simple logrus hook that could be used to check if log messages were outputted
+type simpleLogrusHook struct {
+	mutex        sync.Mutex
+	levels       []log.Level
+	messageCache []log.Entry
+}
+
+func (smh *simpleLogrusHook) Levels() []log.Level {
+	return smh.levels
+}
+func (smh *simpleLogrusHook) Fire(e *log.Entry) error {
+	smh.mutex.Lock()
+	defer smh.mutex.Unlock()
+	smh.messageCache = append(smh.messageCache, *e)
+	return nil
+}
+
+func (smh *simpleLogrusHook) drain() []log.Entry {
+	smh.mutex.Lock()
+	defer smh.mutex.Unlock()
+	res := smh.messageCache
+	smh.messageCache = []log.Entry{}
+	return res
+}
+
+var _ log.Hook = &simpleLogrusHook{}
+
+// A helper funcion for setting arbitrary environment variables and
+// restoring the old ones at the end, usually by deferring the returned callback
+//TODO: remove these hacks when we improve the configuration... we shouldn't
+// have to mess with the global environment at all...
+func setEnv(t *testing.T, newEnv []string) (restoreEnv func()) {
+	actuallSetEnv := func(env []string) {
+		os.Clearenv()
+		for _, e := range env {
+			val := ""
+			pair := strings.SplitN(e, "=", 2)
+			if len(pair) > 1 {
+				val = pair[1]
+			}
+			require.NoError(t, os.Setenv(pair[0], val))
+		}
+	}
+	oldEnv := os.Environ()
+	actuallSetEnv(newEnv)
+
+	return func() {
+		actuallSetEnv(oldEnv)
+	}
+}
+
+var verifyOneIterPerOneVU = func(t *testing.T, c Config) {
+	// No config anywhere should result in a 1 VU with a 1 uninterruptible iteration config
+	sched := c.Execution[lib.DefaultSchedulerName]
+	require.NotEmpty(t, sched)
+	require.IsType(t, scheduler.PerVUIteationsConfig{}, sched)
+	perVuIters, ok := sched.(scheduler.PerVUIteationsConfig)
+	require.True(t, ok)
+	assert.Equal(t, null.NewBool(false, false), perVuIters.Interruptible)
+	assert.Equal(t, null.NewInt(1, false), perVuIters.Iterations)
+	assert.Equal(t, null.NewInt(1, false), perVuIters.VUs)
+}
+
+var verifySharedIters = func(vus, iters int64) func(t *testing.T, c Config) {
+	return func(t *testing.T, c Config) {
+		sched := c.Execution[lib.DefaultSchedulerName]
+		require.NotEmpty(t, sched)
+		require.IsType(t, scheduler.SharedIteationsConfig{}, sched)
+		sharedIterConfig, ok := sched.(scheduler.SharedIteationsConfig)
+		require.True(t, ok)
+		assert.Equal(t, null.NewInt(vus, true), sharedIterConfig.VUs)
+		assert.Equal(t, null.NewInt(iters, true), sharedIterConfig.Iterations)
+	}
+}
+
+var verifyConstantLoopingVUs = func(vus int64, duration time.Duration) func(t *testing.T, c Config) {
+	return func(t *testing.T, c Config) {
+		sched := c.Execution[lib.DefaultSchedulerName]
+		require.NotEmpty(t, sched)
+		require.IsType(t, scheduler.ConstantLoopingVUsConfig{}, sched)
+		clvc, ok := sched.(scheduler.ConstantLoopingVUsConfig)
+		require.True(t, ok)
+		assert.Equal(t, null.NewBool(true, false), clvc.Interruptible)
+		assert.Equal(t, null.NewInt(vus, true), clvc.VUs)
+		assert.Equal(t, types.NullDurationFrom(duration), clvc.Duration)
+	}
+}
+
+func mostFlagSets() []*pflag.FlagSet {
+	// sigh... compromises...
+	return []*pflag.FlagSet{runCmdFlagSet(), archiveCmdFlagSet(), cloudCmdFlagSet()}
+}
+
+// exp contains the different events or errors we expect our test case to trigger.
+// for space and clarity, we use the fact that by default, all of the struct values are false
+type exp struct {
+	cliError           bool
+	consolidationError bool
+	validationErrors   bool
+	logWarning         bool //TODO: remove in the next version?
+}
+
+// A hell of a complicated test case, that still doesn't test things fully...
+type configConsolidationTestCase struct {
+	cliFlagSets   []*pflag.FlagSet
+	cliFlagValues []string
+	env           []string
+	runnerOptions *lib.Options
+	//TODO: test the JSON config as well... after most of https://github.com/loadimpact/k6/issues/883#issuecomment-468646291 is fixed
+	expected        exp
+	customValidator func(t *testing.T, c Config)
+}
+
+var configConsolidationTestCases = []configConsolidationTestCase{
+	// Check that no options will result in 1 VU 1 iter value for execution
+	{mostFlagSets(), nil, nil, nil, exp{}, verifyOneIterPerOneVU},
+	// Verify some CLI errors
+	{mostFlagSets(), []string{"--blah", "blah"}, nil, nil, exp{cliError: true}, nil},
+	{mostFlagSets(), []string{"--duration", "blah"}, nil, nil, exp{cliError: true}, nil},
+	{mostFlagSets(), []string{"--iterations", "blah"}, nil, nil, exp{cliError: true}, nil},
+	{mostFlagSets(), []string{"--execution", ""}, nil, nil, exp{cliError: true}, nil},
+	{mostFlagSets(), []string{"--stage", "10:20s"}, nil, nil, exp{cliError: true}, nil},
+	// Check if CLI shortcuts generate correct execution values
+	{mostFlagSets(), []string{"--vus", "1", "--iterations", "5"}, nil, nil, exp{}, verifySharedIters(1, 5)},
+	{mostFlagSets(), []string{"-u", "2", "-i", "6"}, nil, nil, exp{}, verifySharedIters(2, 6)},
+	{mostFlagSets(), []string{"-u", "3", "-d", "30s"}, nil, nil, exp{}, verifyConstantLoopingVUs(3, 30*time.Second)},
+	{mostFlagSets(), []string{"-u", "4", "--duration", "60s"}, nil, nil, exp{}, verifyConstantLoopingVUs(4, 1*time.Minute)},
+	//TODO: verify stages
+	// This should get a validation error since VUs are more than the shared iterations
+	{mostFlagSets(), []string{"--vus", "10", "-i", "6"}, nil, nil, exp{validationErrors: true}, verifySharedIters(10, 6)},
+	// These should emit a warning
+	{mostFlagSets(), []string{"-u", "1", "-i", "6", "-d", "10s"}, nil, nil, exp{logWarning: true}, nil},
+	{mostFlagSets(), []string{"-u", "2", "-d", "10s", "-s", "10s:20"}, nil, nil, exp{logWarning: true}, nil},
+	{mostFlagSets(), []string{"-u", "3", "-i", "5", "-s", "10s:20"}, nil, nil, exp{logWarning: true}, nil},
+	{mostFlagSets(), []string{"-u", "3", "-d", "0"}, nil, nil, exp{logWarning: true}, nil},
+	// Test if environment variable shortcuts are working as expected
+	{mostFlagSets(), nil, []string{"K6_VUS=5", "K6_ITERATIONS=15"}, nil, exp{}, verifySharedIters(5, 15)},
+	{mostFlagSets(), nil, []string{"K6_VUS=10", "K6_DURATION=20s"}, nil, exp{}, verifyConstantLoopingVUs(10, 20*time.Second)},
+
+	//TODO: test combinations between options and levels
+	//TODO: test the future full overwriting of the duration/iterations/stages/execution options
+
+	// Just in case, verify that no options will result in the same 1 vu 1 iter config
+	{mostFlagSets(), nil, nil, nil, exp{}, verifyOneIterPerOneVU},
+	//TODO: test for differences between flagsets
+	//TODO: more tests in general...
+}
+
+func TestConfigConsolidation(t *testing.T) {
+	logHook := simpleLogrusHook{levels: []log.Level{log.WarnLevel}}
+	log.AddHook(&logHook)
+	log.SetOutput(ioutil.Discard)
+	defer log.SetOutput(os.Stderr)
+
+	runTestCase := func(t *testing.T, testCase configConsolidationTestCase, flagSet *pflag.FlagSet) {
+		logHook.drain()
+
+		restoreEnv := setEnv(t, testCase.env)
+		defer restoreEnv()
+
+		//TODO: also remove these hacks when we improve the configuration...
+		getTestCaseCliConf := func() (Config, error) {
+			if err := flagSet.Parse(testCase.cliFlagValues); err != nil {
+				return Config{}, err
+			}
+			if flagSet.Lookup("out") != nil {
+				return getConfig(flagSet)
+			}
+			opts, errOpts := getOptions(flagSet)
+			return Config{Options: opts}, errOpts
+		}
+
+		cliConf, err := getTestCaseCliConf()
+		if testCase.expected.cliError {
+			require.Error(t, err)
+			return
+		}
+		require.NoError(t, err)
+
+		var runner lib.Runner
+		if testCase.runnerOptions != nil {
+			runner = &lib.MiniRunner{Options: *testCase.runnerOptions}
+		}
+		fs := afero.NewMemMapFs() //TODO: test JSON configs as well!
+		result, err := getConsolidatedConfig(fs, cliConf, runner)
+		if testCase.expected.consolidationError {
+			require.Error(t, err)
+			return
+		}
+		require.NoError(t, err)
 
+		warnings := logHook.drain()
+		if testCase.expected.logWarning {
+			assert.NotEmpty(t, warnings)
+		} else {
+			assert.Empty(t, warnings)
+		}
+
+		validationErrors := result.Validate()
+		if testCase.expected.validationErrors {
+			assert.NotEmpty(t, validationErrors)
+		} else {
+			assert.Empty(t, validationErrors)
+		}
+
+		if testCase.customValidator != nil {
+			testCase.customValidator(t, result)
+		}
+	}
+
+	for tcNum, testCase := range configConsolidationTestCases {
+		for fsNum, flagSet := range testCase.cliFlagSets {
+			// I want to paralelize this, but I cannot... due to global variables and other
+			// questionable architectural choices... :|
+			t.Run(
+				fmt.Sprintf("TestCase#%d_FlagSet#%d", tcNum, fsNum),
+				func(t *testing.T) { runTestCase(t, testCase, flagSet) },
+			)
+		}
+	}
+}
 func TestConfigEnv(t *testing.T) {
 	testdata := map[struct{ Name, Key string }]map[string]func(Config){
 		{"Linger", "K6_LINGER"}: {
@@ -134,8 +367,3 @@ func TestConfigApply(t *testing.T) {
 		assert.Equal(t, []string{"influxdb", "json"}, conf.Out)
 	})
 }
-
-func TestBuildExecutionConfig(t *testing.T) {
-	//TODO: test the current config building and constructing of the execution plan, and the emitted warnings
-	//TODO: test the future full overwriting of the duration/iterations/stages/execution options
-}

cmd/root.go

@@ -100,12 +100,13 @@ func init() {
 		defaultConfigPathMsg = fmt.Sprintf(" (default %s)", filepath.Join(configFolders[0].Path, configFilename))
 	}
 
+	//TODO: figure out a better way to handle the CLI flags - global variables are not very testable... :/
 	RootCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "enable debug logging")
 	RootCmd.PersistentFlags().BoolVarP(&quiet, "quiet", "q", false, "disable progress updates")
 	RootCmd.PersistentFlags().BoolVar(&noColor, "no-color", false, "disable colored output")
 	RootCmd.PersistentFlags().StringVar(&logFmt, "logformat", "", "log output format")
 	RootCmd.PersistentFlags().StringVarP(&address, "address", "a", "localhost:6565", "address for the api server")
-	RootCmd.PersistentFlags().StringVarP(&cfgFile, "config", "c", "", "config file"+defaultConfigPathMsg)
+	RootCmd.PersistentFlags().StringVarP(&cfgFile, "config", "c", "", "config file"+defaultConfigPathMsg) //TODO: figure out and fix?
 	must(cobra.MarkFlagFilename(RootCmd.PersistentFlags(), "config"))
 }