From 08c6cc0511034cef76806a519eb511126b72b75c Mon Sep 17 00:00:00 2001 From: Vladyslav Diachenko Date: Mon, 5 Aug 2024 17:28:29 +0300 Subject: [PATCH] fixed test by asserting the matchers without FastRegexMatcher Signed-off-by: Vladyslav Diachenko --- clients/pkg/logentry/logql/parser_test.go | 30 ++++----- pkg/logql/matchers_test.go | 11 +--- pkg/logql/syntax/ast_test.go | 14 +---- pkg/logql/syntax/parser_test.go | 18 +----- pkg/logql/syntax/serialize_test.go | 4 +- pkg/logql/syntax/test_utils.go | 75 +++++++++++++++++++++++ pkg/loki/runtime_config_test.go | 19 +++--- 7 files changed, 104 insertions(+), 67 deletions(-) create mode 100644 pkg/logql/syntax/test_utils.go diff --git a/clients/pkg/logentry/logql/parser_test.go b/clients/pkg/logentry/logql/parser_test.go index f1bde80189bda..4ca51dc55b77a 100644 --- a/clients/pkg/logentry/logql/parser_test.go +++ b/clients/pkg/logentry/logql/parser_test.go @@ -152,24 +152,20 @@ func TestParse(t *testing.T) { ast, err := ParseExpr(tc.in) require.Equal(t, tc.err, err) - if tc.exp == nil { - require.Nil(t, ast) - } else { - require.NotNil(t, ast) - - // TODO this test is just comparing the matchers, I don't have a smart idea on how to compare the whole expression. - // Maybe we can use the visitor pattern? Alternatively, we may try to use reflection to manipulate the matchers - // to make them comparable with deep equal. + // Prometheus label matchers are not comparable with a deep equal because of the internal + // fast regexp implementation. For this reason, we compare them without FastRegexMatchers. + require.Equal(t, removeFastRegexMatcher(tc.exp), removeFastRegexMatcher(ast)) + }) + } +} - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - expectedMatchers := tc.exp.Matchers() - actualMatchers := ast.Matchers() - require.Len(t, actualMatchers, len(expectedMatchers)) - for i, expected := range expectedMatchers { - require.Equal(t, expected.String(), actualMatchers[i].String()) - } +func removeFastRegexMatcher(exp Expr) Expr { + if typed, ok := exp.(*matchersExpr); ok { + for i, matcher := range typed.matchers { + if matcher.Type == labels.MatchNotRegexp || matcher.Type == labels.MatchRegexp { + typed.matchers[i] = &labels.Matcher{Type: matcher.Type, Name: matcher.Name, Value: matcher.Value} } - }) + } } + return exp } diff --git a/pkg/logql/matchers_test.go b/pkg/logql/matchers_test.go index 840538b4cc5ae..4336e2c08793c 100644 --- a/pkg/logql/matchers_test.go +++ b/pkg/logql/matchers_test.go @@ -5,6 +5,8 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/stretchr/testify/require" + + "github.com/grafana/loki/v3/pkg/logql/syntax" ) func Test_match(t *testing.T) { @@ -51,16 +53,9 @@ func Test_match(t *testing.T) { if tt.wantErr { require.Error(t, err) } else { - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. require.Len(t, got, len(tt.want)) - for i, expectedMatchers := range tt.want { - require.Len(t, got[i], len(expectedMatchers)) - - for j, expectedMatcher := range expectedMatchers { - require.Equal(t, expectedMatcher.String(), got[i][j].String()) - } + syntax.AssertMatchers(t, expectedMatchers, got[i]) } } }) diff --git a/pkg/logql/syntax/ast_test.go b/pkg/logql/syntax/ast_test.go index 4142a4bc2c3d0..62feb88636c2c 100644 --- a/pkg/logql/syntax/ast_test.go +++ b/pkg/logql/syntax/ast_test.go @@ -196,9 +196,7 @@ func Test_SampleExpr_String(t *testing.T) { expr2, err := ParseExpr(expr.String()) require.Nil(t, err) - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - require.Equal(t, expr.String(), expr2.String()) + AssertExpressions(t, expr, expr2) }) } } @@ -595,15 +593,7 @@ func Test_FilterMatcher(t *testing.T) { t.Parallel() expr, err := ParseLogSelector(tt.q, true) assert.Nil(t, err) - - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - actualMatchers := expr.Matchers() - require.Len(t, actualMatchers, len(tt.expectedMatchers)) - for i, expected := range tt.expectedMatchers { - assert.Equal(t, expected.String(), actualMatchers[i].String()) - } - + AssertMatchers(t, tt.expectedMatchers, expr.Matchers()) p, err := expr.Pipeline() assert.Nil(t, err) if tt.lines == nil { diff --git a/pkg/logql/syntax/parser_test.go b/pkg/logql/syntax/parser_test.go index 6608a32269163..f6c919317f5d3 100644 --- a/pkg/logql/syntax/parser_test.go +++ b/pkg/logql/syntax/parser_test.go @@ -3239,16 +3239,7 @@ func TestParse(t *testing.T) { t.Run(tc.in, func(t *testing.T) { ast, err := ParseExpr(tc.in) require.Equal(t, tc.err, err) - - if tc.exp != nil { - require.NotNil(t, ast) - - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - require.Equal(t, tc.exp.String(), ast.String()) - } else { - require.Nil(t, ast) - } + AssertExpressions(t, tc.exp, ast) }) } } @@ -3298,12 +3289,7 @@ func TestParseMatchers(t *testing.T) { if tt.want == nil { require.Nil(t, got) } else { - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - require.Len(t, got, len(tt.want)) - for i, expected := range tt.want { - require.Equal(t, expected.String(), got[i].String()) - } + AssertMatchers(t, tt.want, got) } }) } diff --git a/pkg/logql/syntax/serialize_test.go b/pkg/logql/syntax/serialize_test.go index b0df4606ce3f5..51469b74da1dd 100644 --- a/pkg/logql/syntax/serialize_test.go +++ b/pkg/logql/syntax/serialize_test.go @@ -99,9 +99,7 @@ func TestJSONSerializationParseTestCases(t *testing.T) { t.Log(buf.String()) - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. - require.Equal(t, tc.exp.String(), actual.String()) + AssertExpressions(t, tc.exp, actual) }) } } diff --git a/pkg/logql/syntax/test_utils.go b/pkg/logql/syntax/test_utils.go new file mode 100644 index 0000000000000..2083cec0ac43b --- /dev/null +++ b/pkg/logql/syntax/test_utils.go @@ -0,0 +1,75 @@ +package syntax + +import ( + "testing" + + "github.com/prometheus/prometheus/model/labels" + "github.com/stretchr/testify/require" + + "github.com/grafana/loki/v3/pkg/logql/log" +) + +// AssertExpressions function removes FastRegexMatchers from all Regexp matchers to allow simple objects comparison. +// See removeFastRegexMatcherFromExpr function for the details. +func AssertExpressions(t *testing.T, expected, actual Expr) { + require.Equal(t, removeFastRegexMatcherFromExpr(expected), removeFastRegexMatcherFromExpr(actual)) +} + +// AssertMatchers function removes FastRegexMatchers from all Regexp matchers to allow simple objects comparison. +func AssertMatchers(t *testing.T, expected, actual []*labels.Matcher) { + require.Equal(t, RemoveFastRegexMatchers(expected), RemoveFastRegexMatchers(actual)) +} + +// RemoveFastRegexMatchers iterates over the matchers and recreates the matchers +// without *FastRegexMatcher, because Prometheus labels matcher sets a new instance each time it's created, +// and it prevents simple object assertions. +func RemoveFastRegexMatchers(matchers []*labels.Matcher) []*labels.Matcher { + result := make([]*labels.Matcher, 0, len(matchers)) + for _, matcher := range matchers { + if matcher.Type == labels.MatchNotRegexp || matcher.Type == labels.MatchRegexp { + matcher = &labels.Matcher{Type: matcher.Type, Name: matcher.Name, Value: matcher.Value} + } + result = append(result, matcher) + } + return result +} + +func removeFastRegexMatcherFromExpr(expr Expr) Expr { + if expr == nil { + return nil + } + expr.Walk(func(e Expr) { + switch typed := e.(type) { + case *MatchersExpr: + typed.Mts = RemoveFastRegexMatchers(typed.Mts) + case *LabelFilterExpr: + typed.LabelFilterer = removeFastRegexMatcherFromLabelFilterer(typed.LabelFilterer) + case *LogRange: + if typed.Unwrap == nil { + return + } + cleaned := make([]log.LabelFilterer, 0, len(typed.Unwrap.PostFilters)) + for _, filter := range typed.Unwrap.PostFilters { + cleaned = append(cleaned, removeFastRegexMatcherFromLabelFilterer(filter)) + } + typed.Unwrap.PostFilters = cleaned + default: + return + } + }) + return expr +} + +func removeFastRegexMatcherFromLabelFilterer(filterer log.LabelFilterer) log.LabelFilterer { + if filterer == nil { + return nil + } + switch typed := filterer.(type) { + case *log.LineFilterLabelFilter: + typed.Matcher = RemoveFastRegexMatchers([]*labels.Matcher{typed.Matcher})[0] + case *log.BinaryLabelFilter: + typed.Left = removeFastRegexMatcherFromLabelFilterer(typed.Left) + typed.Right = removeFastRegexMatcherFromLabelFilterer(typed.Left) + } + return filterer +} diff --git a/pkg/loki/runtime_config_test.go b/pkg/loki/runtime_config_test.go index 3c28e5dd6ca3d..ee55dd55b5421 100644 --- a/pkg/loki/runtime_config_test.go +++ b/pkg/loki/runtime_config_test.go @@ -16,6 +16,7 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/stretchr/testify/require" + "github.com/grafana/loki/v3/pkg/logql/syntax" "github.com/grafana/loki/v3/pkg/runtime" "github.com/grafana/loki/v3/pkg/validation" ) @@ -48,8 +49,6 @@ overrides: require.Equal(t, 2*30*24*time.Hour, overrides.RetentionPeriod("29")) // overrides require.Equal(t, []validation.StreamRetention(nil), overrides.StreamRetention("1")) - // Prometheus label matchers are not comparable with a deep equal because of the internal - // fast regexp implementation. For this reason, we compare their string representation. actual := overrides.StreamRetention("29") expected := []validation.StreamRetention{ {Period: model.Duration(48 * time.Hour), Priority: 10, Selector: `{app="foo"}`, Matchers: []*labels.Matcher{ @@ -61,17 +60,15 @@ overrides: }}, } - require.Len(t, actual, len(expected)) - for i, expectedStreamRetention := range expected { - require.Equal(t, expectedStreamRetention.Selector, actual[i].Selector) - require.Equal(t, expectedStreamRetention.Period, actual[i].Period) - require.Equal(t, expectedStreamRetention.Priority, actual[i].Priority) + require.Equal(t, removeFastRegexMatcher(expected), removeFastRegexMatcher(actual)) +} - require.Len(t, actual[i].Matchers, len(expectedStreamRetention.Matchers)) - for j, expectedMatcher := range expectedStreamRetention.Matchers { - require.Equal(t, actual[i].Matchers[j].String(), expectedMatcher.String()) - } +func removeFastRegexMatcher(configs []validation.StreamRetention) []validation.StreamRetention { + result := make([]validation.StreamRetention, 0, len(configs)) + for _, config := range configs { + config.Matchers = syntax.RemoveFastRegexMatchers(config.Matchers) } + return result } func Test_ValidateRules(t *testing.T) {