From a916cff197b05934a2efc1e4e5a36d701c63edc1 Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Mon, 8 Jul 2024 07:58:19 -0400 Subject: [PATCH] break run spec and exec spec into separate methods Cleans up the (large) Scenario.Run and runSpec methods by separating the "preparation" phase of the test spec execution from the execution itself into a different method. This allows us to clean up the manual `defer`s and make the code more readable. Signed-off-by: Jay Pipes --- api/error.go | 7 +- scenario/run.go | 231 +++++++++++++++++++++++-------------------- scenario/run_test.go | 6 +- 3 files changed, 130 insertions(+), 114 deletions(-) diff --git a/api/error.go b/api/error.go index bbd4008..07875c9 100644 --- a/api/error.go +++ b/api/error.go @@ -313,10 +313,11 @@ func RequiredFixtureMissing(name string) error { // tool's timeout conflicts with either a total wait time or a timeout value // from a scenario or spec. func TimeoutConflict( - gotestDeadline time.Duration, - totalWait time.Duration, - maxTimeout time.Duration, + ti *Timings, ) error { + gotestDeadline := ti.GoTestTimeout + totalWait := ti.TotalWait + maxTimeout := ti.MaxTimeout msg := fmt.Sprintf( "go test -timeout value of %s ", (gotestDeadline + time.Second).Round(time.Second), diff --git a/scenario/run.go b/scenario/run.go index 89cfa87..cc2514b 100644 --- a/scenario/run.go +++ b/scenario/run.go @@ -31,32 +31,9 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error { defer func() { ctx = gdtcontext.PopTrace(ctx) }() - now := time.Now() - d, ok := t.Deadline() - if ok && !d.IsZero() { - s.Timings.GoTestTimeout = d.Sub(now) - debug.Println( - ctx, "scenario/run: go test tool deadline: %s", - (s.Timings.GoTestTimeout + time.Second).Round(time.Second), - ) - if s.Timings.TotalWait > 0 { - if s.Timings.TotalWait.Abs() > s.Timings.GoTestTimeout.Abs() { - return api.TimeoutConflict( - s.Timings.GoTestTimeout, - s.Timings.TotalWait, - s.Timings.MaxTimeout, - ) - } - } - if s.Timings.MaxTimeout > 0 { - if s.Timings.MaxTimeout.Abs() > s.Timings.GoTestTimeout.Abs() { - return api.TimeoutConflict( - s.Timings.GoTestTimeout, - s.Timings.TotalWait, - s.Timings.MaxTimeout, - ) - } - } + + if s.hasTimeoutConflict(ctx, t) { + return api.TimeoutConflict(s.Timings) } if len(s.Fixtures) > 0 { @@ -73,6 +50,7 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error { defer fix.Stop(ctx) } } + // If the test author has specified any pre-flight checks in the `skip-if` // collection, evaluate those first and if any failed, skip the scenario's // tests. @@ -90,79 +68,25 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error { } } + var res *api.Result var err error - scDefaults := s.getScenarioDefaults() - - t.Run(s.Title(), func(t *testing.T) { - for idx, spec := range s.Tests { - sb := spec.Base() - // Create a brand new context that inherits the top-level context's - // cancel func. We want to set deadlines for each test spec and if - // we mutate the single supplied top-level context, then only the - // first deadline/timeout will be used. - specCtx, specCancel := context.WithCancel(ctx) - - specTraceMsg := strconv.Itoa(idx) - if sb.Name != "" { - specTraceMsg += ":" + sb.Name - } - specCtx = gdtcontext.PushTrace(specCtx, specTraceMsg) - popTracer := func() { - specCtx = gdtcontext.PopTrace(specCtx) - } - - plugin := sb.Plugin - - rt := getRetry(specCtx, scDefaults, plugin, spec) - - to := getTimeout(specCtx, scDefaults, plugin, spec) - - var res *api.Result - ch := make(chan runSpecRes, 1) - - wait := sb.Wait - if wait != nil && wait.Before != "" { - debug.Println(specCtx, "wait: %s before", wait.Before) - time.Sleep(wait.BeforeDuration()) - } - - if to != nil { - specCtx, specCancel = context.WithTimeout(specCtx, to.Duration()) - } - - go s.runSpec(specCtx, ch, rt, idx, spec) - - select { - case <-specCtx.Done(): - popTracer() - t.Fatalf("assertion failed: timeout exceeded (%s)", to.After) - break - case runres := <-ch: - res = runres.r - err = runres.err - } + t.Run(s.Title(), func(tt *testing.T) { + for idx := range s.Tests { + res, err = s.runSpec(ctx, tt, idx) if err != nil { - popTracer() - specCancel() break } - if wait != nil && wait.After != "" { - debug.Println(specCtx, "wait: %s after", wait.After) - time.Sleep(wait.AfterDuration()) - } - // Results can have arbitrary run data stored in them and we // save this prior run data in the top-level context (and pass // that context to the next Run invocation). if res.HasData() { ctx = gdtcontext.StorePriorRun(ctx, res.Data()) } - popTracer() - specCancel() + for _, fail := range res.Failures() { - t.Fatal(fail) + tt.Fatal(fail) } } }) @@ -174,8 +98,71 @@ type runSpecRes struct { err error } -// runSpec executes an individual test spec +// runSpec wraps the execution of a single test spec func (s *Scenario) runSpec( + ctx context.Context, // this is the overall scenario's context + t *testing.T, // T specific to the goroutine running this test spec + idx int, // index of the test spec within Scenario.Tests +) (res *api.Result, err error) { + // Create a brand new context that inherits the top-level context's + // cancel func. We want to set deadlines for each test spec and if + // we mutate the single supplied top-level context, then only the + // first deadline/timeout will be used. + specCtx, specCancel := context.WithCancel(ctx) + defer specCancel() + + defaults := s.getDefaults() + spec := s.Tests[idx] + sb := spec.Base() + + specTraceMsg := strconv.Itoa(idx) + if sb.Name != "" { + specTraceMsg += ":" + sb.Name + } + specCtx = gdtcontext.PushTrace(specCtx, specTraceMsg) + defer func() { + specCtx = gdtcontext.PopTrace(specCtx) + }() + + plugin := sb.Plugin + rt := getRetry(specCtx, defaults, plugin, spec) + to := getTimeout(specCtx, defaults, plugin, spec) + ch := make(chan runSpecRes, 1) + + wait := sb.Wait + if wait != nil && wait.Before != "" { + debug.Println(specCtx, "wait: %s before", wait.Before) + time.Sleep(wait.BeforeDuration()) + } + + if to != nil { + specCtx, specCancel = context.WithTimeout(specCtx, to.Duration()) + defer specCancel() + } + + go s.execSpec(specCtx, ch, rt, idx, spec) + + select { + case <-specCtx.Done(): + t.Fatalf("assertion failed: timeout exceeded (%s)", to.After) + case runres := <-ch: + res = runres.r + err = runres.err + } + if err != nil { + return nil, err + } + + if wait != nil && wait.After != "" { + debug.Println(specCtx, "wait: %s after", wait.After) + time.Sleep(wait.AfterDuration()) + } + return res, nil +} + +// execSpec executes an individual test spec, performing any retries as +// necessary until a timeout is exceeded or the test spec succeeds +func (s *Scenario) execSpec( ctx context.Context, ch chan runSpecRes, retry *api.Retry, @@ -190,7 +177,7 @@ func (s *Scenario) runSpec( return } debug.Println( - ctx, "run: single-shot (no retries) ok: %v", + ctx, "spec/run: single-shot (no retries) ok: %v", !res.Failed(), ) ch <- runSpecRes{res, nil} @@ -229,7 +216,7 @@ func (s *Scenario) runSpec( for tick := range ticker.C { if (maxAttempts > 0) && (attempts > maxAttempts) { debug.Println( - ctx, "run: exceeded max attempts %d. stopping.", + ctx, "spec/run: exceeded max attempts %d. stopping.", maxAttempts, ) ticker.Stop() @@ -244,7 +231,7 @@ func (s *Scenario) runSpec( } success = !res.Failed() debug.Println( - ctx, "run: attempt %d after %s ok: %v", + ctx, "spec/run: attempt %d after %s ok: %v", attempts, after, success, ) if success { @@ -253,7 +240,7 @@ func (s *Scenario) runSpec( } for _, f := range res.Failures() { debug.Println( - ctx, "run: attempt %d failure: %s", + ctx, "spec/run: attempt %d failure: %s", attempts, f, ) } @@ -262,6 +249,34 @@ func (s *Scenario) runSpec( ch <- runSpecRes{res, nil} } +// hasTimeoutConflict returns true if the scenario or any of its test specs has +// a wait or timeout that exceeds the go test tool's specified timeout value +func (s *Scenario) hasTimeoutConflict( + ctx context.Context, + t *testing.T, +) bool { + d, ok := t.Deadline() + if ok && !d.IsZero() { + now := time.Now() + s.Timings.GoTestTimeout = d.Sub(now) + debug.Println( + ctx, "scenario/run: go test tool timeout: %s", + (s.Timings.GoTestTimeout + time.Second).Round(time.Second), + ) + if s.Timings.TotalWait > 0 { + if s.Timings.TotalWait.Abs() > s.Timings.GoTestTimeout.Abs() { + return true + } + } + if s.Timings.MaxTimeout > 0 { + if s.Timings.MaxTimeout.Abs() > s.Timings.GoTestTimeout.Abs() { + return true + } + } + } + return false +} + // getTimeout returns the timeout configuration for the test spec. We check for // overrides in timeout configuration using the following precedence: // @@ -271,7 +286,7 @@ func (s *Scenario) runSpec( // * Plugin's default func getTimeout( ctx context.Context, - scenDefaults *Defaults, + defaults *Defaults, plugin api.Plugin, eval api.Evaluable, ) *api.Timeout { @@ -294,12 +309,12 @@ func getTimeout( return baseTimeout } - if scenDefaults != nil && scenDefaults.Timeout != nil { + if defaults != nil && defaults.Timeout != nil { debug.Println( ctx, "using timeout of %s [scenario default]", - scenDefaults.Timeout.After, + defaults.Timeout.After, ) - return scenDefaults.Timeout + return defaults.Timeout } pluginInfo := plugin.Info() @@ -324,7 +339,7 @@ func getTimeout( // * Plugin's default func getRetry( ctx context.Context, - scenDefaults *Defaults, + defaults *Defaults, plugin api.Plugin, eval api.Evaluable, ) *api.Retry { @@ -363,21 +378,21 @@ func getRetry( return baseRetry } - if scenDefaults != nil && scenDefaults.Retry != nil { - scenRetry := scenDefaults.Retry - if scenRetry == api.NoRetry { - return scenRetry + if defaults != nil && defaults.Retry != nil { + defaultRetry := defaults.Retry + if defaultRetry == api.NoRetry { + return defaultRetry } msg := "using retry" - if scenRetry.Attempts != nil { - msg += fmt.Sprintf(" (attempts: %d)", *scenRetry.Attempts) + if defaultRetry.Attempts != nil { + msg += fmt.Sprintf(" (attempts: %d)", *defaultRetry.Attempts) } - if scenRetry.Interval != "" { - msg += fmt.Sprintf(" (interval: %s)", scenRetry.Interval) + if defaultRetry.Interval != "" { + msg += fmt.Sprintf(" (interval: %s)", defaultRetry.Interval) } - msg += fmt.Sprintf(" (exponential: %t) [scenario default]", scenRetry.Exponential) + msg += fmt.Sprintf(" (exponential: %t) [scenario default]", defaultRetry.Exponential) debug.Println(ctx, msg) - return scenRetry + return defaultRetry } pluginInfo := plugin.Info() @@ -401,9 +416,9 @@ func getRetry( return nil } -// getScenarioDefaults returns the Defaults parsed from the scenario's YAML +// getDefaults returns the Defaults parsed from the scenario's YAML // file's `defaults` field, or nil if none were specified. -func (s *Scenario) getScenarioDefaults() *Defaults { +func (s *Scenario) getDefaults() *Defaults { scDefaultsAny, found := s.Defaults[DefaultsKey] if found { return scDefaultsAny.(*Defaults) diff --git a/scenario/run_test.go b/scenario/run_test.go index 9b8ed35..440477e 100644 --- a/scenario/run_test.go +++ b/scenario/run_test.go @@ -193,7 +193,7 @@ func TestNoRetry(t *testing.T) { w.Flush() require.NotEqual(b.Len(), 0) debugout := b.String() - require.Contains(debugout, "[gdt] [no-retry/0:bar] run: single-shot (no retries) ok: true") + require.Contains(debugout, "[gdt] [no-retry/0:bar] spec/run: single-shot (no retries) ok: true") } func TestNoRetryEvaluableOverride(t *testing.T) { @@ -217,7 +217,7 @@ func TestNoRetryEvaluableOverride(t *testing.T) { w.Flush() require.NotEqual(b.Len(), 0) debugout := b.String() - require.Contains(debugout, "[gdt] [no-retry-evaluable-override/0:bar] run: single-shot (no retries) ok: true") + require.Contains(debugout, "[gdt] [no-retry-evaluable-override/0:bar] spec/run: single-shot (no retries) ok: true") } func TestFailRetryTestOverride(t *testing.T) { @@ -253,7 +253,7 @@ func TestRetryTestOverride(t *testing.T) { require.NotNil(err) debugout := string(outerr) - require.Contains(debugout, "[gdt] [retry-test-override/0:baz] run: exceeded max attempts 2. stopping.") + require.Contains(debugout, "[gdt] [retry-test-override/0:baz] spec/run: exceeded max attempts 2. stopping.") } func TestSkipIf(t *testing.T) {