Merge pull request #2400 from grafana/fix/1443_remove_js_runtime_from…

…_threshold_calculation

Fix/1443 remove js runtime from threshold calculation

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{`1+1==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{
@@ -291,8 +292,12 @@ func TestEngineThresholdsWillAbort(t *testing.T) {
 	t.Parallel()
 	metric := stats.New("my_metric", stats.Gauge)
 
-	ths, err := stats.NewThresholds([]string{"1+1==3"})
-	assert.NoError(t, err)
+	// 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 := 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}
@@ -310,8 +315,13 @@ func TestEngineAbortedByThresholds(t *testing.T) {
 	t.Parallel()
 	metric := stats.New("my_metric", stats.Gauge)
 
-	ths, err := stats.NewThresholds([]string{"1+1==3"})
-	assert.NoError(t, err)
+	// The MiniRunner sets the value of the metric to 1.25. Considering
+	// 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 := 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}
@@ -350,14 +360,14 @@ func TestEngine_processThresholds(t *testing.T) {
 		ths   map[string][]string
 		abort bool
 	}{
-		"passing":  {true, map[string][]string{"my_metric": {"1+1==2"}}, false},
-		"failing":  {false, map[string][]string{"my_metric": {"1+1==3"}}, false},
-		"aborting": {false, map[string][]string{"my_metric": {"1+1==3"}}, true},
-
-		"submetric,match,passing":   {true, map[string][]string{"my_metric{a:1}": {"1+1==2"}}, false},
-		"submetric,match,failing":   {false, map[string][]string{"my_metric{a:1}": {"1+1==3"}}, false},
-		"submetric,nomatch,passing": {true, map[string][]string{"my_metric{a:2}": {"1+1==2"}}, false},
-		"submetric,nomatch,failing": {true, map[string][]string{"my_metric{a:2}": {"1+1==3"}}, false},
+		"passing":  {true, map[string][]string{"my_metric": {"value<2"}}, false},
+		"failing":  {false, map[string][]string{"my_metric": {"value>1.25"}}, false},
+		"aborting": {false, map[string][]string{"my_metric": {"value>1.25"}}, true},
+
+		"submetric,match,passing":   {true, map[string][]string{"my_metric{a:1}": {"value<2"}}, false},
+		"submetric,match,failing":   {false, map[string][]string{"my_metric{a:1}": {"value>1.25"}}, false},
+		"submetric,nomatch,passing": {true, map[string][]string{"my_metric{a:2}": {"value<2"}}, false},
+		"submetric,nomatch,failing": {true, map[string][]string{"my_metric{a:2}": {"value>1.25"}}, false},
 	}
 
 	for name, data := range testdata {
@@ -366,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})
 }