Merge pull request #2358 from grafana/noGlobalLogrus

Remove global logrus usage from DeriveScenariosFromShortcuts

cmd/archive.go

@@ -78,7 +78,7 @@ An archive is a fully self-contained test run, and can be executed identically e
 				return err
 			}
 
-			_, err = deriveAndValidateConfig(conf, r.IsExecutable)
+			_, err = deriveAndValidateConfig(conf, r.IsExecutable, logger)
 			if err != nil {
 				return err
 			}

cmd/cloud.go

@@ -120,7 +120,7 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth
 				return err
 			}
 
-			derivedConf, err := deriveAndValidateConfig(conf, r.IsExecutable)
+			derivedConf, err := deriveAndValidateConfig(conf, r.IsExecutable, logger)
 			if err != nil {
 				return err
 			}

cmd/config.go

@@ -29,6 +29,7 @@ import (
 	"strings"
 
 	"github.com/mstoykov/envconfig"
+	"github.com/sirupsen/logrus"
 	"github.com/spf13/afero"
 	"github.com/spf13/pflag"
 	"gopkg.in/guregu/null.v3"
@@ -233,9 +234,11 @@ func applyDefault(conf Config) Config {
 	return conf
 }
 
-func deriveAndValidateConfig(conf Config, isExecutable func(string) bool) (result Config, err error) {
+func deriveAndValidateConfig(
+	conf Config, isExecutable func(string) bool, logger logrus.FieldLogger,
+) (result Config, err error) {
 	result = conf
-	result.Options, err = executor.DeriveScenariosFromShortcuts(conf.Options)
+	result.Options, err = executor.DeriveScenariosFromShortcuts(conf.Options, logger)
 	if err == nil {
 		err = validateConfig(result, isExecutable)
 	}

cmd/config_consolidation_test.go

@@ -21,8 +21,6 @@ package cmd
 
 import (
 	"fmt"
-	"io/ioutil"
-	"os"
 	"testing"
 	"time"
 
@@ -534,13 +532,18 @@ func runTestCase(
 	t *testing.T,
 	testCase configConsolidationTestCase,
 	newFlagSet flagSetInit,
-	logHook *testutils.SimpleLogrusHook,
 ) {
 	t.Helper()
 	t.Logf("Test with opts=%#v and exp=%#v\n", testCase.options, testCase.expected)
 	output := testutils.NewTestOutput(t)
-	logrus.SetOutput(output)
+	logHook := &testutils.SimpleLogrusHook{
+		HookedLevels: []logrus.Level{logrus.WarnLevel},
+	}
+
 	logHook.Drain()
+	logger := logrus.New()
+	logger.AddHook(logHook)
+	logger.SetOutput(output)
 
 	flagSet := newFlagSet()
 	defer resetStickyGlobalVars()
@@ -589,7 +592,7 @@ func runTestCase(
 	require.NoError(t, err)
 
 	derivedConfig := consolidatedConfig
-	derivedConfig.Options, err = executor.DeriveScenariosFromShortcuts(consolidatedConfig.Options)
+	derivedConfig.Options, err = executor.DeriveScenariosFromShortcuts(consolidatedConfig.Options, logger)
 	if testCase.expected.derivationError {
 		require.Error(t, err)
 		return
@@ -617,11 +620,7 @@ func runTestCase(
 //nolint:paralleltest // see comments in test
 func TestConfigConsolidation(t *testing.T) {
 	// This test and its subtests shouldn't be ran in parallel, since they unfortunately have
-	// to mess with shared global objects (env vars, variables, the log, ... santa?)
-	logHook := testutils.SimpleLogrusHook{HookedLevels: []logrus.Level{logrus.WarnLevel}}
-	logrus.AddHook(&logHook)
-	logrus.SetOutput(ioutil.Discard)
-	defer logrus.SetOutput(os.Stderr)
+	// to mess with shared global objects (variables, ... santa?)
 
 	for tcNum, testCase := range getConfigConsolidationTestCases() {
 		tcNum, testCase := tcNum, testCase
@@ -635,7 +634,7 @@ func TestConfigConsolidation(t *testing.T) {
 			fsNum, flagSet := fsNum, flagSet
 			t.Run(
 				fmt.Sprintf("TestCase#%d_FlagSet#%d", tcNum, fsNum),
-				func(t *testing.T) { runTestCase(t, testCase, flagSet, &logHook) },
+				func(t *testing.T) { runTestCase(t, testCase, flagSet) },
 			)
 		}
 	}

cmd/config_test.go

@@ -203,7 +203,7 @@ func TestDeriveAndValidateConfig(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			_, err := deriveAndValidateConfig(tc.conf,
-				func(_ string) bool { return tc.isExec })
+				func(_ string) bool { return tc.isExec }, nil)
 			if tc.err != "" {
 				var ecerr errext.HasExitCode
 				assert.ErrorAs(t, err, &ecerr)

cmd/inspect.go

@@ -124,7 +124,7 @@ func addExecRequirements(b *js.Bundle,
 		return nil, err
 	}
 
-	conf, err = deriveAndValidateConfig(conf, runner.IsExecutable)
+	conf, err = deriveAndValidateConfig(conf, runner.IsExecutable, logger)
 	if err != nil {
 		return nil, err
 	}

cmd/run.go

@@ -128,7 +128,7 @@ a commandline interface for interacting with it.`,
 				return err
 			}
 
-			conf, err = deriveAndValidateConfig(conf, initRunner.IsExecutable)
+			conf, err = deriveAndValidateConfig(conf, initRunner.IsExecutable, logger)
 			if err != nil {
 				return err
 			}

core/engine_test.go

@@ -65,17 +65,16 @@ func newTestEngine( //nolint:golint
 		runCtx, runCancel = context.WithCancel(globalCtx)
 	}
 
+	logger := logrus.New()
+	logger.SetOutput(testutils.NewTestOutput(t))
 	newOpts, err := executor.DeriveScenariosFromShortcuts(lib.Options{
 		MetricSamplesBufferSize: null.NewInt(200, false),
-	}.Apply(runner.GetOptions()).Apply(opts))
+	}.Apply(runner.GetOptions()).Apply(opts), logger)
 	require.NoError(t, err)
 	require.Empty(t, newOpts.Validate())
 
 	require.NoError(t, runner.SetOptions(newOpts))
 
-	logger := logrus.New()
-	logger.SetOutput(testutils.NewTestOutput(t))
-
 	execScheduler, err := local.NewExecutionScheduler(runner, logger)
 	require.NoError(t, err)
 
@@ -820,7 +819,7 @@ func TestVuInitException(t *testing.T) {
 	)
 	require.NoError(t, err)
 
-	opts, err := executor.DeriveScenariosFromShortcuts(runner.GetOptions())
+	opts, err := executor.DeriveScenariosFromShortcuts(runner.GetOptions(), nil)
 	require.NoError(t, err)
 	require.Empty(t, opts.Validate())
 	require.NoError(t, runner.SetOptions(opts))
@@ -1206,7 +1205,7 @@ func TestActiveVUsCount(t *testing.T) {
 
 	opts, err := executor.DeriveScenariosFromShortcuts(lib.Options{
 		MetricSamplesBufferSize: null.NewInt(200, false),
-	}.Apply(runner.GetOptions()))
+	}.Apply(runner.GetOptions()), nil)
 	require.NoError(t, err)
 	require.Empty(t, opts.Validate())
 	require.NoError(t, runner.SetOptions(opts))

core/local/local_test.go

@@ -63,7 +63,7 @@ func newTestExecutionScheduler(
 	ctx, cancel = context.WithCancel(context.Background())
 	newOpts, err := executor.DeriveScenariosFromShortcuts(lib.Options{
 		MetricSamplesBufferSize: null.NewInt(200, false),
-	}.Apply(runner.GetOptions()).Apply(opts))
+	}.Apply(runner.GetOptions()).Apply(opts), nil)
 	require.NoError(t, err)
 	require.Empty(t, newOpts.Validate())
 
@@ -940,7 +940,7 @@ func TestExecutionSchedulerEndIterations(t *testing.T) {
 	options, err := executor.DeriveScenariosFromShortcuts(lib.Options{
 		VUs:        null.IntFrom(1),
 		Iterations: null.IntFrom(100),
-	})
+	}, nil)
 	require.NoError(t, err)
 	require.Empty(t, options.Validate())
 
@@ -1168,7 +1168,7 @@ func TestRealTimeAndSetupTeardownMetrics(t *testing.T) {
 		SystemTags:      &stats.DefaultSystemTagSet,
 		SetupTimeout:    types.NullDurationFrom(4 * time.Second),
 		TeardownTimeout: types.NullDurationFrom(4 * time.Second),
-	}))
+	}), nil)
 	require.NoError(t, err)
 	require.NoError(t, runner.SetOptions(options))
 
@@ -1351,7 +1351,7 @@ func TestSetPaused(t *testing.T) {
 		options, err := executor.DeriveScenariosFromShortcuts(lib.Options{
 			Iterations: null.IntFrom(2),
 			VUs:        null.IntFrom(1),
-		}.Apply(runner.GetOptions()))
+		}.Apply(runner.GetOptions()), nil)
 		require.NoError(t, err)
 		require.NoError(t, runner.SetOptions(options))
 

lib/executor/execution_config_shortcuts.go

@@ -69,7 +69,7 @@ func getSharedIterationsScenario(iters null.Int, duration types.NullDuration, vu
 // DeriveScenariosFromShortcuts checks for conflicting options and turns any
 // shortcut options (i.e. duration, iterations, stages) into the proper
 // long-form scenario/executor configuration in the scenarios property.
-func DeriveScenariosFromShortcuts(opts lib.Options) (lib.Options, error) {
+func DeriveScenariosFromShortcuts(opts lib.Options, logger logrus.FieldLogger) (lib.Options, error) {
 	result := opts
 
 	switch {
@@ -98,7 +98,7 @@ func DeriveScenariosFromShortcuts(opts lib.Options) (lib.Options, error) {
 			)
 		}
 		if opts.Duration.Duration <= 0 {
-			//TODO: move this validation to Validate()?
+			// TODO: move this validation to Validate()?
 			return result, ExecutionConflictError(
 				"`duration` should be more than 0, for infinite duration use the externally-controlled executor",
 			)
@@ -119,18 +119,18 @@ func DeriveScenariosFromShortcuts(opts lib.Options) (lib.Options, error) {
 	default:
 		// Check if we should emit some warnings
 		if opts.VUs.Valid && opts.VUs.Int64 != 1 {
-			logrus.Warnf(
+			logger.Warnf(
 				"the `vus=%d` option will be ignored, it only works in conjunction with `iterations`, `duration`, or `stages`",
 				opts.VUs.Int64,
 			)
 		}
 		if opts.Stages != nil && len(opts.Stages) == 0 {
 			// No someone explicitly set stages to empty
-			logrus.Warnf("`stages` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")
+			logger.Warnf("`stages` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")
 		}
 		if opts.Scenarios != nil && len(opts.Scenarios) == 0 {
 			// No shortcut, and someone explicitly set execution to empty
-			logrus.Warnf("`scenarios` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")
+			logger.Warnf("`scenarios` was explicitly set to an empty value, running the script with 1 iteration in 1 VU")
 		}
 		// No execution parameters whatsoever were specified, so we'll create a per-VU iterations config
 		// with 1 VU and 1 iteration.
@@ -139,7 +139,7 @@ func DeriveScenariosFromShortcuts(opts lib.Options) (lib.Options, error) {
 		}
 	}
 
-	//TODO: validate the config; questions:
+	// TODO: validate the config; questions:
 	// - separately validate the duration, iterations and stages for better error messages?
 	// - or reuse the execution validation somehow, at the end? or something mixed?
 	// - here or in getConsolidatedConfig() or somewhere else?