Ensure thresholds parsing is skipped on --no-thresholds

This commit ensures that thresholds parsing is a separate step, done
only when the `--no-threshold` flag is not set.

Note that this commit deactivates tests that assumed
`Thresholds.UnmarshalJSON` to effectively parse the thresholds.

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

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,
+				)
+			}
+		})
+	}
+}

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

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

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,
+				)
+			}
+		})
+	}
+}

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)"
+	);
+}

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
 			}

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})
 }

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{}
+)

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{