diff --git a/cmd/archive.go b/cmd/archive.go index c3bb61e3503..d17096c0fd8 100644 --- a/cmd/archive.go +++ b/cmd/archive.go @@ -28,6 +28,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" + "go.k6.io/k6/errext" + "go.k6.io/k6/errext/exitcodes" "go.k6.io/k6/lib/metrics" ) @@ -75,6 +77,18 @@ An archive is a fully self-contained test run, and can be executed identically e return err } + // Parse the thresholds, only if the --no-threshold flag is not set. + // If parsing the threshold expressions failed, consider it as an + // invalid configuration error. + if !runtimeOptions.NoThresholds.Bool { + for _, thresholds := range conf.Options.Thresholds { + err = thresholds.Parse() + if err != nil { + return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig) + } + } + } + _, err = deriveAndValidateConfig(conf, r.IsExecutable, logger) if err != nil { return err diff --git a/cmd/archive_test.go b/cmd/archive_test.go new file mode 100644 index 00000000000..0b53a651866 --- /dev/null +++ b/cmd/archive_test.go @@ -0,0 +1,70 @@ +package cmd + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.k6.io/k6/errext" + "go.k6.io/k6/errext/exitcodes" + "go.k6.io/k6/lib/testutils" +) + +func TestArchiveThresholds(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + noThresholds bool + testFilename string + + wantErr bool + }{ + { + name: "archive should fail with exit status 104 on a malformed threshold expression", + noThresholds: false, + testFilename: "testdata/thresholds/malformed_expression.js", + wantErr: true, + }, + { + name: "archive should on a malformed threshold expression but --no-thresholds flag set", + noThresholds: true, + testFilename: "testdata/thresholds/malformed_expression.js", + wantErr: false, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + cmd := getArchiveCmd(testutils.NewLogger(t), newCommandFlags()) + filename, err := filepath.Abs(testCase.testFilename) + require.NoError(t, err) + args := []string{filename} + if testCase.noThresholds { + args = append(args, "--no-thresholds") + } + cmd.SetArgs(args) + wantExitCode := exitcodes.InvalidConfig + + var gotErrExt errext.HasExitCode + gotErr := cmd.Execute() + + assert.Equal(t, + testCase.wantErr, + gotErr != nil, + "archive command error = %v, wantErr %v", gotErr, testCase.wantErr, + ) + + if testCase.wantErr { + require.ErrorAs(t, gotErr, &gotErrExt) + assert.Equalf(t, wantExitCode, gotErrExt.ExitCode(), + "status code must be %d", wantExitCode, + ) + } + }) + } +} diff --git a/cmd/cloud.go b/cmd/cloud.go index fac936ce076..6eb7366024b 100644 --- a/cmd/cloud.go +++ b/cmd/cloud.go @@ -114,6 +114,18 @@ This will execute the test on the k6 cloud service. Use "k6 login cloud" to auth return err } + // Parse the thresholds, only if the --no-threshold flag is not set. + // If parsing the threshold expressions failed, consider it as an + // invalid configuration error. + if !runtimeOptions.NoThresholds.Bool { + for _, thresholds := range conf.Options.Thresholds { + err = thresholds.Parse() + if err != nil { + return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig) + } + } + } + derivedConf, err := deriveAndValidateConfig(conf, r.IsExecutable, logger) if err != nil { return err diff --git a/cmd/run.go b/cmd/run.go index 95885724809..6e168823991 100644 --- a/cmd/run.go +++ b/cmd/run.go @@ -125,6 +125,18 @@ a commandline interface for interacting with it.`, return err } + // Parse the thresholds, only if the --no-threshold flag is not set. + // If parsing the threshold expressions failed, consider it as an + // invalid configuration error. + if !runtimeOptions.NoThresholds.Bool { + for _, thresholds := range conf.Options.Thresholds { + err = thresholds.Parse() + if err != nil { + return errext.WithExitCodeIfNone(err, exitcodes.InvalidConfig) + } + } + } + conf, err = deriveAndValidateConfig(conf, initRunner.IsExecutable, logger) if err != nil { return err diff --git a/cmd/run_test.go b/cmd/run_test.go index 0cd9fde24e0..2c33d0e365d 100644 --- a/cmd/run_test.go +++ b/cmd/run_test.go @@ -211,3 +211,63 @@ func TestInitErrExitCode(t *testing.T) { "Status code must be %d", exitcodes.ScriptException) assert.Contains(t, err.Error(), "ReferenceError: someUndefinedVar is not defined") } + +func TestRunThresholds(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + noThresholds bool + testFilename string + + wantErr bool + }{ + { + name: "run should fail with exit status 104 on a malformed threshold expression", + noThresholds: false, + testFilename: "testdata/thresholds/malformed_expression.js", + wantErr: true, + }, + { + name: "run should on a malformed threshold expression but --no-thresholds flag set", + noThresholds: true, + testFilename: "testdata/thresholds/malformed_expression.js", + wantErr: false, + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cmd := getRunCmd(ctx, testutils.NewLogger(t), newCommandFlags()) + filename, err := filepath.Abs(testCase.testFilename) + require.NoError(t, err) + args := []string{filename} + if testCase.noThresholds { + args = append(args, "--no-thresholds") + } + cmd.SetArgs(args) + wantExitCode := exitcodes.InvalidConfig + + var gotErrExt errext.HasExitCode + gotErr := cmd.Execute() + + assert.Equal(t, + testCase.wantErr, + gotErr != nil, + "run command error = %v, wantErr %v", gotErr, testCase.wantErr, + ) + + if testCase.wantErr { + require.ErrorAs(t, gotErr, &gotErrExt) + assert.Equalf(t, wantExitCode, gotErrExt.ExitCode(), + "status code must be %d", wantExitCode, + ) + } + }) + } +} diff --git a/cmd/testdata/thresholds/malformed_expression.js b/cmd/testdata/thresholds/malformed_expression.js new file mode 100644 index 00000000000..99f0e235c34 --- /dev/null +++ b/cmd/testdata/thresholds/malformed_expression.js @@ -0,0 +1,11 @@ +export const options = { + thresholds: { + http_reqs: ["foo&0"], // Counter + }, +}; + +export default function () { + console.log( + "asserting that a malformed threshold fails with exit code 104 (Invalid config)" + ); +} diff --git a/core/engine_test.go b/core/engine_test.go index 8b9224ab7d1..685faa94d18 100644 --- a/core/engine_test.go +++ b/core/engine_test.go @@ -263,8 +263,9 @@ func TestEngine_processSamples(t *testing.T) { }) t.Run("submetric", func(t *testing.T) { t.Parallel() - ths, err := stats.NewThresholds([]string{`value<2`}) - assert.NoError(t, err) + ths := stats.NewThresholds([]string{`value<2`}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) e, _, wait := newTestEngine(t, nil, nil, nil, lib.Options{ Thresholds: map[string]stats.Thresholds{ @@ -294,8 +295,9 @@ func TestEngineThresholdsWillAbort(t *testing.T) { // The incoming samples for the metric set it to 1.25. Considering // the metric is of type Gauge, value > 1.25 should always fail, and // trigger an abort. - ths, err := stats.NewThresholds([]string{"value>1.25"}) - assert.NoError(t, err) + ths := stats.NewThresholds([]string{"value>1.25"}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = true thresholds := map[string]stats.Thresholds{metric.Name: ths} @@ -317,8 +319,9 @@ func TestEngineAbortedByThresholds(t *testing.T) { // the metric is of type Gauge, value > 1.25 should always fail, and // trigger an abort. // **N.B**: a threshold returning an error, won't trigger an abort. - ths, err := stats.NewThresholds([]string{"value>1.25"}) - assert.NoError(t, err) + ths := stats.NewThresholds([]string{"value>1.25"}) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = true thresholds := map[string]stats.Thresholds{metric.Name: ths} @@ -373,8 +376,9 @@ func TestEngine_processThresholds(t *testing.T) { t.Parallel() thresholds := make(map[string]stats.Thresholds, len(data.ths)) for m, srcs := range data.ths { - ths, err := stats.NewThresholds(srcs) - assert.NoError(t, err) + ths := stats.NewThresholds(srcs) + gotParseErr := ths.Parse() + require.NoError(t, gotParseErr) ths.Thresholds[0].AbortOnFail = data.abort thresholds[m] = ths } diff --git a/output/json/json_test.go b/output/json/json_test.go index c788006d8b3..e38a3ebe47b 100644 --- a/output/json/json_test.go +++ b/output/json/json_test.go @@ -197,8 +197,7 @@ func setThresholds(t *testing.T, out output.Output) { jout, ok := out.(*Output) require.True(t, ok) - ts, err := stats.NewThresholds([]string{"rate<0.01", "p(99)<250"}) - require.NoError(t, err) + ts := stats.NewThresholds([]string{"rate<0.01", "p(99)<250"}) jout.SetThresholds(map[string]stats.Thresholds{"my_metric1": ts}) } diff --git a/stats/thresholds.go b/stats/thresholds.go index 1b7cbaea53f..3b513dc57b0 100644 --- a/stats/thresholds.go +++ b/stats/thresholds.go @@ -44,18 +44,13 @@ type Threshold struct { parsed *thresholdExpression } -func newThreshold(src string, abortOnFail bool, gracePeriod types.NullDuration) (*Threshold, error) { - parsedExpression, err := parseThresholdExpression(src) - if err != nil { - return nil, err - } - +func newThreshold(src string, abortOnFail bool, gracePeriod types.NullDuration) *Threshold { return &Threshold{ Source: src, AbortOnFail: abortOnFail, AbortGracePeriod: gracePeriod, - parsed: parsedExpression, - }, nil + parsed: nil, + } } func (t *Threshold) runNoTaint(sinks map[string]float64) (bool, error) { @@ -143,7 +138,7 @@ type Thresholds struct { } // NewThresholds returns Thresholds objects representing the provided source strings -func NewThresholds(sources []string) (Thresholds, error) { +func NewThresholds(sources []string) Thresholds { tcs := make([]thresholdConfig, len(sources)) for i, source := range sources { tcs[i].Threshold = source @@ -152,19 +147,16 @@ func NewThresholds(sources []string) (Thresholds, error) { return newThresholdsWithConfig(tcs) } -func newThresholdsWithConfig(configs []thresholdConfig) (Thresholds, error) { +func newThresholdsWithConfig(configs []thresholdConfig) Thresholds { thresholds := make([]*Threshold, len(configs)) sinked := make(map[string]float64) for i, config := range configs { - t, err := newThreshold(config.Threshold, config.AbortOnFail, config.AbortGracePeriod) - if err != nil { - return Thresholds{}, fmt.Errorf("threshold %d error: %w", i, err) - } + t := newThreshold(config.Threshold, config.AbortOnFail, config.AbortGracePeriod) thresholds[i] = t } - return Thresholds{thresholds, false, sinked}, nil + return Thresholds{thresholds, false, sinked} } func (ts *Thresholds) runAll(timeSpentInTest time.Duration) (bool, error) { @@ -237,17 +229,30 @@ func (ts *Thresholds) Run(sink Sink, duration time.Duration) (bool, error) { return ts.runAll(duration) } +// Parse parses the Thresholds and fills each Threshold.parsed field with the result. +// It effectively asserts they are syntaxically correct. +func (ts *Thresholds) Parse() error { + for _, t := range ts.Thresholds { + parsed, err := parseThresholdExpression(t.Source) + if err != nil { + return err + } + + t.parsed = parsed + } + + return nil +} + // UnmarshalJSON is implementation of json.Unmarshaler func (ts *Thresholds) UnmarshalJSON(data []byte) error { var configs []thresholdConfig if err := json.Unmarshal(data, &configs); err != nil { return err } - newts, err := newThresholdsWithConfig(configs) - if err != nil { - return err - } - *ts = newts + + *ts = newThresholdsWithConfig(configs) + return nil } @@ -279,5 +284,7 @@ func MarshalJSONWithoutHTMLEscape(t interface{}) ([]byte, error) { return bytes, err } -var _ json.Unmarshaler = &Thresholds{} -var _ json.Marshaler = &Thresholds{} +var ( + _ json.Unmarshaler = &Thresholds{} + _ json.Marshaler = &Thresholds{} +) diff --git a/stats/thresholds_parser.go b/stats/thresholds_parser.go index 72b0f82e2a7..cdb0a28190b 100644 --- a/stats/thresholds_parser.go +++ b/stats/thresholds_parser.go @@ -73,17 +73,23 @@ func parseThresholdExpression(input string) (*thresholdExpression, error) { // checks that the expression has the right format. method, operator, value, err := scanThresholdExpression(input) if err != nil { - return nil, fmt.Errorf("failed parsing threshold expression; reason: %w", err) + return nil, fmt.Errorf("failed parsing threshold expression %q; reason: %w", input, err) } parsedMethod, parsedMethodValue, err := parseThresholdAggregationMethod(method) if err != nil { - return nil, fmt.Errorf("failed parsing threshold expression's left hand side; reason: %w", err) + err = fmt.Errorf("failed parsing threshold expression's %q left hand side; "+ + "reason: %w", input, err, + ) + return nil, err } parsedValue, err := strconv.ParseFloat(value, 64) if err != nil { - return nil, fmt.Errorf("failed parsing threshold expresion's right hand side; reason: %w", err) + err = fmt.Errorf("failed parsing threshold expresion's %q right hand side; "+ + "reason: %w", input, err, + ) + return nil, err } condition := &thresholdExpression{ diff --git a/stats/thresholds_test.go b/stats/thresholds_test.go index 9e381bf75fb..91ff5cf89c4 100644 --- a/stats/thresholds_test.go +++ b/stats/thresholds_test.go @@ -37,29 +37,14 @@ func TestNewThreshold(t *testing.T) { src := `rate<0.01` abortOnFail := false gracePeriod := types.NullDurationFrom(2 * time.Second) - wantParsed := &thresholdExpression{tokenRate, null.Float{}, tokenLess, 0.01} - gotThreshold, err := newThreshold(src, abortOnFail, gracePeriod) + gotThreshold := newThreshold(src, abortOnFail, gracePeriod) - assert.NoError(t, err) assert.Equal(t, src, gotThreshold.Source) assert.False(t, gotThreshold.LastFailed) assert.Equal(t, abortOnFail, gotThreshold.AbortOnFail) assert.Equal(t, gracePeriod, gotThreshold.AbortGracePeriod) - assert.Equal(t, wantParsed, gotThreshold.parsed) -} - -func TestNewThreshold_InvalidThresholdConditionExpression(t *testing.T) { - t.Parallel() - - src := "1+1==2" - abortOnFail := false - gracePeriod := types.NullDurationFrom(2 * time.Second) - - gotThreshold, err := newThreshold(src, abortOnFail, gracePeriod) - - assert.Error(t, err, "instantiating a threshold with an invalid expression should fail") - assert.Nil(t, gotThreshold, "instantiating a threshold with an invalid expression should return a nil Threshold") + assert.Nil(t, gotThreshold.parsed) } func TestThreshold_runNoTaint(t *testing.T) { @@ -219,8 +204,10 @@ func TestThresholdRun(t *testing.T) { t.Parallel() sinks := map[string]float64{"rate": 0.0001} - threshold, err := newThreshold(`rate<0.01`, false, types.NullDuration{}) - assert.NoError(t, err) + parsed, parseErr := parseThresholdExpression("rate<0.01") + require.NoError(t, parseErr) + threshold := newThreshold(`rate<0.01`, false, types.NullDuration{}) + threshold.parsed = parsed t.Run("no taint", func(t *testing.T) { b, err := threshold.runNoTaint(sinks) @@ -243,8 +230,10 @@ func TestThresholdRun(t *testing.T) { t.Parallel() sinks := map[string]float64{"rate": 1} - threshold, err := newThreshold(`rate<0.01`, false, types.NullDuration{}) - assert.NoError(t, err) + parsed, parseErr := parseThresholdExpression("rate<0.01") + require.NoError(t, parseErr) + threshold := newThreshold(`rate<0.01`, false, types.NullDuration{}) + threshold.parsed = parsed t.Run("no taint", func(t *testing.T) { b, err := threshold.runNoTaint(sinks) @@ -262,22 +251,103 @@ func TestThresholdRun(t *testing.T) { }) } +func TestThresholdsParse(t *testing.T) { + t.Parallel() + + t.Run("valid threshold expressions", func(t *testing.T) { + t.Parallel() + + // Prepare a Thresholds collection containing syntaxically + // correct thresholds + ts := Thresholds{ + Thresholds: []*Threshold{ + newThreshold("rate<1", false, types.NullDuration{}), + }, + } + + // Collect the result of the parsing operation + gotErr := ts.Parse() + + assert.NoError(t, gotErr, "Parse shouldn't fail parsing valid expressions") + assert.Condition(t, func() bool { + for _, threshold := range ts.Thresholds { + if threshold.parsed == nil { + return false + } + } + + return true + }, "Parse did not fail, but some Thresholds' parsed field is left empty") + }) + + t.Run("invalid threshold expressions", func(t *testing.T) { + t.Parallel() + + // Prepare a Thresholds collection containing syntaxically + // correct thresholds + ts := Thresholds{ + Thresholds: []*Threshold{ + newThreshold("foo&1", false, types.NullDuration{}), + }, + } + + // Collect the result of the parsing operation + gotErr := ts.Parse() + + assert.Error(t, gotErr, "Parse should fail parsing invalid expressions") + assert.Condition(t, func() bool { + for _, threshold := range ts.Thresholds { + if threshold.parsed == nil { + return true + } + } + + return false + }, "Parse failed, but some Thresholds' parsed field was not empty") + }) + + t.Run("mixed valid/invalid threshold expressions", func(t *testing.T) { + t.Parallel() + + // Prepare a Thresholds collection containing syntaxically + // correct thresholds + ts := Thresholds{ + Thresholds: []*Threshold{ + newThreshold("rate<1", false, types.NullDuration{}), + newThreshold("foo&1", false, types.NullDuration{}), + }, + } + + // Collect the result of the parsing operation + gotErr := ts.Parse() + + assert.Error(t, gotErr, "Parse should fail parsing invalid expressions") + assert.Condition(t, func() bool { + for _, threshold := range ts.Thresholds { + if threshold.parsed == nil { + return true + } + } + + return false + }, "Parse failed, but some Thresholds' parsed field was not empty") + }) +} + func TestNewThresholds(t *testing.T) { t.Parallel() t.Run("empty", func(t *testing.T) { t.Parallel() - ts, err := NewThresholds([]string{}) - assert.NoError(t, err) + ts := NewThresholds([]string{}) assert.Len(t, ts.Thresholds, 0) }) t.Run("two", func(t *testing.T) { t.Parallel() sources := []string{`rate<0.01`, `p(95)<200`} - ts, err := NewThresholds(sources) - assert.NoError(t, err) + ts := NewThresholds(sources) assert.Len(t, ts.Thresholds, 2) for i, th := range ts.Thresholds { assert.Equal(t, sources[i], th.Source) @@ -293,8 +363,7 @@ func TestNewThresholdsWithConfig(t *testing.T) { t.Run("empty", func(t *testing.T) { t.Parallel() - ts, err := NewThresholds([]string{}) - assert.NoError(t, err) + ts := NewThresholds([]string{}) assert.Len(t, ts.Thresholds, 0) }) t.Run("two", func(t *testing.T) { @@ -304,8 +373,7 @@ func TestNewThresholdsWithConfig(t *testing.T) { {`rate<0.01`, false, types.NullDuration{}}, {`p(95)<200`, true, types.NullDuration{}}, } - ts, err := newThresholdsWithConfig(configs) - assert.NoError(t, err) + ts := newThresholdsWithConfig(configs) assert.Len(t, ts.Thresholds, 2) for i, th := range ts.Thresholds { assert.Equal(t, configs[i].Threshold, th.Source) @@ -342,15 +410,15 @@ func TestThresholdsRunAll(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - thresholds, err := NewThresholds(data.sources) + thresholds := NewThresholds(data.sources) + gotParseErr := thresholds.Parse() + require.NoError(t, gotParseErr) thresholds.sinked = map[string]float64{"rate": 0.0001, "p(95)": 500} thresholds.Thresholds[0].AbortOnFail = data.abort thresholds.Thresholds[0].AbortGracePeriod = data.grace runDuration := 1500 * time.Millisecond - assert.NoError(t, err) - succeeded, err := thresholds.runAll(runDuration) if data.err { @@ -411,8 +479,9 @@ func TestThresholds_Run(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { t.Parallel() - thresholds, err := NewThresholds([]string{"p(95)<2000"}) - require.NoError(t, err, "Initializing new thresholds should not fail") + thresholds := NewThresholds([]string{"p(95)<2000"}) + gotParseErr := thresholds.Parse() + require.NoError(t, gotParseErr) gotOk, gotErr := thresholds.Run(testCase.args.sink, testCase.args.duration) assert.Equal(t, gotErr != nil, testCase.wantErr, "Thresholds.Run() error = %v, wantErr %v", gotErr, testCase.wantErr) @@ -536,7 +605,6 @@ func TestThresholdsJSON(t *testing.T) { t.Parallel() var ts Thresholds - assert.Error(t, json.Unmarshal([]byte(`["="]`), &ts)) assert.Nil(t, ts.Thresholds) assert.False(t, ts.Abort) })