Skip to content

Commit

Permalink
Start to execute setup() and teardown() only if they were exported
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
na-- committed Dec 2, 2022
1 parent 6edf871 commit 28c0629
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 3 deletions.
2 changes: 1 addition & 1 deletion cmd/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 0 additions & 2 deletions core/local/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()"))

Expand Down
16 changes: 16 additions & 0 deletions js/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down Expand Up @@ -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()

Expand Down

0 comments on commit 28c0629

Please sign in to comment.