From 28c06296e5f6e92544500a5b69d80f32db0dd9c9 Mon Sep 17 00:00:00 2001 From: Nedyalko Andreev Date: Fri, 2 Dec 2022 13:36:30 +0200 Subject: [PATCH] Start to execute setup() and teardown() only if they were exported Previous to this commit, k6 would initialize a transient VU and try to execute setup() and teardown() even if we knew they were not defined and exported by the script. That VU was created and then immediately discarded afterwards. It was probably done like that because, when setup() and teardown() were implemented in k6 v0.20.0, we didn't keep track of what JS functions were exported from the script. We only started doing that in k6 v0.27.0, when we added support for multiple scenarios, each with a potentially different `exec` function. We just didn't go back and improve the setup() and teardown() efficiency afterwards and they continued to use the "try to run it and see if it works" approach. While it was inefficient, it didn't use to be a big problem before. However, now that we want VU initialization to be interruptible by cancelling a context, it became a problem for fine-grained unit tests. Because the setupTimeout and teardownTimeout default values are defined in the cmd/ package, tests in sub-packages like js/ and core/ saw their values as 0. So, they created contexts with 0 timeouts, which immediately expired and the transient VUs for setup() and teardown() couldn't even be initialized without an error. The config mess strikes again... I thought about fixing the config somehow, but actually not running the setup() and teardown() functions if they were not defined seems both simpler and cleaner. Any test low-level that wants to use them can just manually define their timeouts, which is usually a good idea to do anyway. --- cmd/integration_test.go | 2 +- core/local/local.go | 2 -- js/runner.go | 16 ++++++++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/cmd/integration_test.go b/cmd/integration_test.go index 8fde44b493f..7767cd7eb23 100644 --- a/cmd/integration_test.go +++ b/cmd/integration_test.go @@ -108,7 +108,7 @@ func TestStdoutAndStderrAreEmptyWithQuietAndLogsForwarded(t *testing.T) { // Instead it should be in the log file logContents, err := afero.ReadFile(ts.fs, logFilePath) require.NoError(t, err) - assert.Equal(t, "init\ninit\ninit\nfoo\ninit\n", string(logContents)) //nolint:dupword + assert.Equal(t, "init\ninit\nfoo\n", string(logContents)) //nolint:dupword } func TestRelativeLogPathWithSetupAndTeardown(t *testing.T) { diff --git a/core/local/local.go b/core/local/local.go index cb2a3109854..0029c1dc560 100644 --- a/core/local/local.go +++ b/core/local/local.go @@ -420,7 +420,6 @@ func (e *ExecutionScheduler) Run(globalCtx, runCtx context.Context, engineOut ch // Run setup() before any executors, if it's not disabled if !e.state.Test.Options.NoSetup.Bool { - logger.Debug("Running setup()") e.state.SetExecutionStatus(lib.ExecutionStatusSetup) e.initProgress.Modify(pb.WithConstProgress(1, "setup()")) if err := e.state.Test.Runner.Setup(runSubCtx, engineOut); err != nil { @@ -456,7 +455,6 @@ func (e *ExecutionScheduler) Run(globalCtx, runCtx context.Context, engineOut ch // Run teardown() after all executors are done, if it's not disabled if !e.state.Test.Options.NoTeardown.Bool { - logger.Debug("Running teardown()") e.state.SetExecutionStatus(lib.ExecutionStatusTeardown) e.initProgress.Modify(pb.WithConstProgress(1, "teardown()")) diff --git a/js/runner.go b/js/runner.go index bd6a66c44a9..101f9acbd29 100644 --- a/js/runner.go +++ b/js/runner.go @@ -274,6 +274,14 @@ func forceHTTP1() bool { // Setup runs the setup function if there is one and sets the setupData to the returned value func (r *Runner) Setup(ctx context.Context, out chan<- metrics.SampleContainer) error { + if !r.IsExecutable(consts.SetupFn) { + // do not init a new transient VU or execute setup() if it wasn't + // actually defined and exported in the script + r.preInitState.Logger.Debugf("%s() is not defined or not exported, skipping!", consts.SetupFn) + return nil + } + r.preInitState.Logger.Debugf("Running %s()...", consts.SetupFn) + setupCtx, setupCancel := context.WithTimeout(ctx, r.getTimeoutFor(consts.SetupFn)) defer setupCancel() @@ -307,6 +315,14 @@ func (r *Runner) SetSetupData(data []byte) { // Teardown runs the teardown function if there is one. func (r *Runner) Teardown(ctx context.Context, out chan<- metrics.SampleContainer) error { + if !r.IsExecutable(consts.TeardownFn) { + // do not init a new transient VU or execute teardown() if it wasn't + // actually defined and exported in the script + r.preInitState.Logger.Debugf("%s() is not defined or not exported, skipping!", consts.TeardownFn) + return nil + } + r.preInitState.Logger.Debugf("Running %s()...", consts.TeardownFn) + teardownCtx, teardownCancel := context.WithTimeout(ctx, r.getTimeoutFor(consts.TeardownFn)) defer teardownCancel()