From 2ccf01842a05b0cbb32318332838f1c213c676ba Mon Sep 17 00:00:00 2001 From: Jay Pipes Date: Sat, 6 Jul 2024 09:40:43 -0400 Subject: [PATCH] check go test -timeout before running scenario Adds code that totals up wait times and max timeout values and checks that the `go test -timeout` value (which defaults to 10m) is smaller than either the total wait time or largest timeout and returns a RuntimeError if so, allowing the user to adjust their `go test -timeout` value or scenario/spec timeouts/waits. Issue #45 Signed-off-by: Jay Pipes --- Makefile | 2 +- api/error.go | 37 ++++++++++ api/timings.go | 71 +++++++++++++++++++ scenario/defaults.go | 18 +++-- scenario/parse.go | 29 +++++++- scenario/run.go | 36 ++++++++-- scenario/run_test.go | 54 ++++++++++++++ scenario/scenario.go | 7 +- scenario/testdata/fixture-start-error.yaml | 1 - .../timeout-conflict-default-timeout.yaml | 6 ++ .../timeout-conflict-spec-timeout.yaml | 5 ++ .../testdata/timeout-conflict-total-wait.yaml | 6 ++ 12 files changed, 256 insertions(+), 16 deletions(-) create mode 100644 api/timings.go create mode 100644 scenario/testdata/timeout-conflict-default-timeout.yaml create mode 100644 scenario/testdata/timeout-conflict-spec-timeout.yaml create mode 100644 scenario/testdata/timeout-conflict-total-wait.yaml diff --git a/Makefile b/Makefile index 266aaf1..6dd033b 100644 --- a/Makefile +++ b/Makefile @@ -3,4 +3,4 @@ VERSION ?= $(shell git describe --tags --always --dirty) .PHONY: test test: - go test -v ./... + go test -timeout 10s -v ./... diff --git a/api/error.go b/api/error.go index 4a1c7c0..bbd4008 100644 --- a/api/error.go +++ b/api/error.go @@ -7,6 +7,7 @@ package api import ( "errors" "fmt" + "time" "gopkg.in/yaml.v3" ) @@ -294,6 +295,12 @@ var ( "%w: required fixture missing", RuntimeError, ) + // ErrTimeoutConflict is returned when the Go test tool's timeout conflicts + // with either a total wait time or a timeout in a scenario or test spec + ErrTimeoutConflict = fmt.Errorf( + "%w: timeout conflict", + RuntimeError, + ) ) // RequiredFixtureMissing returns an ErrRequiredFixture with the supplied @@ -301,3 +308,33 @@ var ( func RequiredFixtureMissing(name string) error { return fmt.Errorf("%w: %s", ErrRequiredFixture, name) } + +// TimeoutConflict returns an ErrTimeoutConflict describing how the Go test +// 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, +) error { + msg := fmt.Sprintf( + "go test -timeout value of %s ", + (gotestDeadline + time.Second).Round(time.Second), + ) + if totalWait > 0 { + msg += fmt.Sprintf( + "is shorter than the total wait time in the scenario: %s. "+ + "either decrease the wait times or increase the "+ + "go test -timeout value.", + totalWait.Round(time.Second), + ) + } else { + msg += fmt.Sprintf( + "is shorter than the maximum timeout specified in the "+ + "scenario: %s. either decrease the scenario or spec "+ + "timeout or increase the go test -timeout value.", + maxTimeout.Round(time.Second), + ) + } + return fmt.Errorf("%w: %s", ErrTimeoutConflict, msg) +} diff --git a/api/timings.go b/api/timings.go new file mode 100644 index 0000000..cc10a95 --- /dev/null +++ b/api/timings.go @@ -0,0 +1,71 @@ +// Use and distribution licensed under the Apache license version 2. +// +// See the COPYING file in the root project directory for full text. + +package api + +import ( + "time" +) + +type SetOn int + +const ( + SetOnNone SetOn = iota + SetOnSpec // a test spec override + SetOnPlugin // a plugin override + SetOnPluginDefault // a plugin default + SetOnDefault // a scenario default +) + +// Timings contains information about a test scenario's maximum wait and +// timeout duration and what aspect of the scenario (the scenario defaults, a +// plugin default, a test spec override, etc) had the maximum timeout or wait +// value. +// +// We use this information when initially assessing whether the Go test tool's +// overall timeout is shorter than this maximum in order to inform the user to +// increase the Go test tool timeout. +type Timings struct { + // GoTestTimeout will be the duration of the timeout specified (or + // defaulted) by the Go test tool + GoTestTimeout time.Duration + // TotalWait will be non-zero when there is a wait specified for either the + // scenario or a test spec and will contain the aggregate duration of all + // waits + TotalWait time.Duration + // MaxTimeout will be non-zero when there is a timeout specified for either + // the scenario or a test spec and will contain the duration of the maximum + // timeout + MaxTimeout time.Duration + //TimeoutSetOn indicates where the MaxTimeout value was found + MaxTimeoutSetOn SetOn + // TimeoutSpecIndex indicates the test spec's index within the scenario where + // the max timeout was found + MaxTimeoutSpecIndex int +} + +// AddWait adds a wait duration to the Timings and (re)-calculates the Timings' +// MaxWait attributes +func (t *Timings) AddWait( + d time.Duration, +) { + t.TotalWait += d +} + +// AddTimeout adds a timeout duration to the Timings and (re)-calculates the +// Timings' MaxTimeout attributes +func (t *Timings) AddTimeout( + d time.Duration, + on SetOn, + specIndex int, +) { + if d == 0 { + return + } + if d.Abs() > t.MaxTimeout { + t.MaxTimeout = d + t.MaxTimeoutSetOn = on + t.MaxTimeoutSpecIndex = specIndex + } +} diff --git a/scenario/defaults.go b/scenario/defaults.go index 95bb250..803055a 100644 --- a/scenario/defaults.go +++ b/scenario/defaults.go @@ -45,12 +45,20 @@ func (d *Defaults) UnmarshalYAML(node *yaml.Node) error { valNode := node.Content[i+1] switch key { case "timeout": - if valNode.Kind != yaml.MappingNode { - return api.ExpectedMapAt(valNode) - } var to *api.Timeout - if err := valNode.Decode(&to); err != nil { - return api.ExpectedTimeoutAt(valNode) + switch valNode.Kind { + case yaml.MappingNode: + // We support the old-style timeout:after + if err := valNode.Decode(&to); err != nil { + return api.ExpectedTimeoutAt(valNode) + } + case yaml.ScalarNode: + // We also support a straight string duration + to = &api.Timeout{ + After: valNode.Value, + } + default: + return api.ExpectedScalarOrMapAt(valNode) } _, err := time.ParseDuration(to.After) if err != nil { diff --git a/scenario/parse.go b/scenario/parse.go index 8a1e486..92b7c07 100644 --- a/scenario/parse.go +++ b/scenario/parse.go @@ -19,6 +19,7 @@ func (s *Scenario) UnmarshalYAML(node *yaml.Node) error { if node.Kind != yaml.MappingNode { return api.ExpectedMapAt(node) } + s.Timings = &api.Timings{} plugins := plugin.Registered() defaults := api.Defaults{} // maps/structs are stored in a top-level Node.Content field which is a @@ -78,6 +79,13 @@ func (s *Scenario) UnmarshalYAML(node *yaml.Node) error { if err := valNode.Decode(&scenDefaults); err != nil { return err } + if scenDefaults.Timeout != nil { + s.Timings.AddTimeout( + scenDefaults.Timeout.Duration(), + api.SetOnDefault, + -1, + ) + } defaults[DefaultsKey] = &scenDefaults s.Defaults = defaults } @@ -107,7 +115,7 @@ func (s *Scenario) UnmarshalYAML(node *yaml.Node) error { pluginSpecs[p] = p.Specs() } for plugin, specs := range pluginSpecs { - for _, sp := range specs { + for idx, sp := range specs { if err := testNode.Decode(sp); err != nil { if errors.Is(err, api.ErrUnknownField) { continue @@ -115,6 +123,25 @@ func (s *Scenario) UnmarshalYAML(node *yaml.Node) error { return err } base.Plugin = plugin + if base.Wait != nil { + if base.Wait.Before != "" { + s.Timings.AddWait( + base.Wait.BeforeDuration(), + ) + } + if base.Wait.After != "" { + s.Timings.AddWait( + base.Wait.AfterDuration(), + ) + } + } + if base.Timeout != nil { + s.Timings.AddTimeout( + base.Timeout.Duration(), + api.SetOnSpec, + idx, + ) + } sp.SetBase(base) s.Tests = append(s.Tests, sp) parsed = true diff --git a/scenario/run.go b/scenario/run.go index d843f75..89cfa87 100644 --- a/scenario/run.go +++ b/scenario/run.go @@ -27,6 +27,38 @@ import ( // which will mark the test units failed or skipped if a test unit evaluates to // false. func (s *Scenario) Run(ctx context.Context, t *testing.T) error { + ctx = gdtcontext.PushTrace(ctx, s.Title()) + 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 len(s.Fixtures) > 0 { fixtures := gdtcontext.Fixtures(ctx) for _, fname := range s.Fixtures { @@ -62,10 +94,6 @@ func (s *Scenario) Run(ctx context.Context, t *testing.T) error { scDefaults := s.getScenarioDefaults() t.Run(s.Title(), func(t *testing.T) { - ctx = gdtcontext.PushTrace(ctx, s.Title()) - defer func() { - ctx = gdtcontext.PopTrace(ctx) - }() for idx, spec := range s.Tests { sb := spec.Base() diff --git a/scenario/run_test.go b/scenario/run_test.go index ec75a7c..9b8ed35 100644 --- a/scenario/run_test.go +++ b/scenario/run_test.go @@ -74,6 +74,60 @@ func TestMissingFixtures(t *testing.T) { assert.ErrorIs(err, api.RuntimeError) } +func TestTimeoutConflictTotalWait(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + fp := filepath.Join("testdata", "timeout-conflict-total-wait.yaml") + f, err := os.Open(fp) + require.Nil(err) + + s, err := scenario.FromReader(f, scenario.WithPath(fp)) + require.Nil(err) + require.NotNil(s) + + err = s.Run(context.TODO(), t) + assert.NotNil(err) + assert.ErrorIs(err, api.ErrTimeoutConflict) + assert.ErrorIs(err, api.RuntimeError) +} + +func TestTimeoutConflictSpecTimeout(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + fp := filepath.Join("testdata", "timeout-conflict-spec-timeout.yaml") + f, err := os.Open(fp) + require.Nil(err) + + s, err := scenario.FromReader(f, scenario.WithPath(fp)) + require.Nil(err) + require.NotNil(s) + + err = s.Run(context.TODO(), t) + assert.NotNil(err) + assert.ErrorIs(err, api.ErrTimeoutConflict) + assert.ErrorIs(err, api.RuntimeError) +} + +func TestTimeoutConflictDefaultTimeout(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + fp := filepath.Join("testdata", "timeout-conflict-default-timeout.yaml") + f, err := os.Open(fp) + require.Nil(err) + + s, err := scenario.FromReader(f, scenario.WithPath(fp)) + require.Nil(err) + require.NotNil(s) + + err = s.Run(context.TODO(), t) + assert.NotNil(err) + assert.ErrorIs(err, api.ErrTimeoutConflict) + assert.ErrorIs(err, api.RuntimeError) +} + func TestFixtureStartError(t *testing.T) { require := require.New(t) assert := assert.New(t) diff --git a/scenario/scenario.go b/scenario/scenario.go index c12b337..fd079bf 100644 --- a/scenario/scenario.go +++ b/scenario/scenario.go @@ -13,10 +13,9 @@ import ( // Scenario is a generalized gdt test case file. It contains a set of Runnable // test units. type Scenario struct { - // evalPlugins stores the plugin that will evaluate the test spec at a - // particular index - evalPlugins map[int]api.Plugin - // Path is the filepath to the test case. + // Timings is the collection of max wait/timeout values for the scenario. + Timings *api.Timings `yaml:"-"` + // Path is the filepath to the test scenario YAML file. Path string `yaml:"-"` // Name is the short name for the test case. If empty, defaults to the base // filename in Path. diff --git a/scenario/testdata/fixture-start-error.yaml b/scenario/testdata/fixture-start-error.yaml index 2c625a0..b9c023f 100644 --- a/scenario/testdata/fixture-start-error.yaml +++ b/scenario/testdata/fixture-start-error.yaml @@ -4,4 +4,3 @@ fixtures: - start-error tests: - foo: bar - diff --git a/scenario/testdata/timeout-conflict-default-timeout.yaml b/scenario/testdata/timeout-conflict-default-timeout.yaml new file mode 100644 index 0000000..f86c166 --- /dev/null +++ b/scenario/testdata/timeout-conflict-default-timeout.yaml @@ -0,0 +1,6 @@ +name: timeout-conflict-default-timeout +description: a scenario with a default timeout greater than the go test tool timeout +defaults: + timeout: 1m +tests: + - foo: bar diff --git a/scenario/testdata/timeout-conflict-spec-timeout.yaml b/scenario/testdata/timeout-conflict-spec-timeout.yaml new file mode 100644 index 0000000..5991d7a --- /dev/null +++ b/scenario/testdata/timeout-conflict-spec-timeout.yaml @@ -0,0 +1,5 @@ +name: timeout-conflict-spec-timeout +description: a scenario with a spec timeout greater than the go test tool timeout +tests: + - timeout: 1m + foo: bar diff --git a/scenario/testdata/timeout-conflict-total-wait.yaml b/scenario/testdata/timeout-conflict-total-wait.yaml new file mode 100644 index 0000000..77917ec --- /dev/null +++ b/scenario/testdata/timeout-conflict-total-wait.yaml @@ -0,0 +1,6 @@ +name: timeout-conflict-total-wait +description: a scenario with total wait times greater than the go test tool timeout +tests: + - wait: + before: 1m + foo: bar