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/run.go

@@ -125,6 +125,16 @@ a commandline interface for interacting with it.`,
 				return err
 			}
 
+			// Parse the thresholds, only if the --no-threshold flag is not set.
+			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

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