From d9d0bbac59020ee929f4e7e94d0ae8cfe63e8a8f Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Tue, 27 Feb 2024 01:11:00 +0100 Subject: [PATCH 1/2] tests: add test on ExcludeRule and SeverityRule --- pkg/config/issues.go | 15 +++++ pkg/config/issues_test.go | 113 +++++++++++++++++++++++++++++++++++- pkg/config/severity_test.go | 77 ++++++++++++++++++++++++ 3 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 pkg/config/severity_test.go diff --git a/pkg/config/issues.go b/pkg/config/issues.go index 27dcf81056ad..b81c888bfbd8 100644 --- a/pkg/config/issues.go +++ b/pkg/config/issues.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "regexp" ) @@ -141,34 +142,47 @@ func (b *BaseRule) Validate(minConditionsCount int) error { if err := validateOptionalRegex(b.Path); err != nil { return fmt.Errorf("invalid path regex: %w", err) } + if err := validateOptionalRegex(b.PathExcept); err != nil { return fmt.Errorf("invalid path-except regex: %w", err) } + if err := validateOptionalRegex(b.Text); err != nil { return fmt.Errorf("invalid text regex: %w", err) } + if err := validateOptionalRegex(b.Source); err != nil { return fmt.Errorf("invalid source regex: %w", err) } + + if b.Path != "" && b.PathExcept != "" { + return errors.New("path and path-except should be set at the same time") + } + nonBlank := 0 if len(b.Linters) > 0 { nonBlank++ } + // Filtering by path counts as one condition, regardless how it is done (one or both). // Otherwise, a rule with Path and PathExcept set would pass validation // whereas before the introduction of path-except that wouldn't have been precise enough. if b.Path != "" || b.PathExcept != "" { nonBlank++ } + if b.Text != "" { nonBlank++ } + if b.Source != "" { nonBlank++ } + if nonBlank < minConditionsCount { return fmt.Errorf("at least %d of (text, source, path[-except], linters) should be set", minConditionsCount) } + return nil } @@ -176,6 +190,7 @@ func validateOptionalRegex(value string) error { if value == "" { return nil } + _, err := regexp.Compile(value) return err } diff --git a/pkg/config/issues_test.go b/pkg/config/issues_test.go index f870bfb9b982..2c17fce2c4ba 100644 --- a/pkg/config/issues_test.go +++ b/pkg/config/issues_test.go @@ -4,11 +4,16 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGetExcludePatterns(t *testing.T) { - assert.Equal(t, GetExcludePatterns(nil), DefaultExcludePatterns) + patterns := GetExcludePatterns(nil) + assert.Equal(t, patterns, DefaultExcludePatterns) +} + +func TestGetExcludePatterns_includes(t *testing.T) { include := []string{DefaultExcludePatterns[0].ID, DefaultExcludePatterns[1].ID} exclude := GetExcludePatterns(include) @@ -19,3 +24,109 @@ func TestGetExcludePatterns(t *testing.T) { assert.Contains(t, DefaultExcludePatterns, p) } } + +func TestExcludeRule_Validate(t *testing.T) { + testCases := []struct { + desc string + rule *ExcludeRule + expected string + }{ + { + desc: "empty rule", + rule: &ExcludeRule{}, + expected: "at least 2 of (text, source, path[-except], linters) should be set", + }, + { + desc: "only path rule", + rule: &ExcludeRule{ + BaseRule{ + Path: "test", + }, + }, + expected: "at least 2 of (text, source, path[-except], linters) should be set", + }, + { + desc: "only path-except rule", + rule: &ExcludeRule{ + BaseRule{ + PathExcept: "test", + }, + }, + expected: "at least 2 of (text, source, path[-except], linters) should be set", + }, + { + desc: "only text rule", + rule: &ExcludeRule{ + BaseRule{ + Text: "test", + }, + }, + expected: "at least 2 of (text, source, path[-except], linters) should be set", + }, + { + desc: "only source rule", + rule: &ExcludeRule{ + BaseRule{ + Source: "test", + }, + }, + expected: "at least 2 of (text, source, path[-except], linters) should be set", + }, + { + desc: "invalid path rule", + rule: &ExcludeRule{ + BaseRule{ + Path: "**test", + }, + }, + expected: "invalid path regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid path-except rule", + rule: &ExcludeRule{ + BaseRule{ + PathExcept: "**test", + }, + }, + expected: "invalid path-except regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid text rule", + rule: &ExcludeRule{ + BaseRule{ + Text: "**test", + }, + }, + expected: "invalid text regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid source rule", + rule: &ExcludeRule{ + BaseRule{ + Source: "**test", + }, + }, + expected: "invalid source regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "path and path-expect", + rule: &ExcludeRule{ + BaseRule{ + Path: "test", + PathExcept: "test", + }, + }, + expected: "path and path-except should be set at the same time", + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + err := test.rule.Validate() + require.EqualError(t, err, test.expected) + }) + } +} diff --git a/pkg/config/severity_test.go b/pkg/config/severity_test.go new file mode 100644 index 000000000000..ddcc6d6a9440 --- /dev/null +++ b/pkg/config/severity_test.go @@ -0,0 +1,77 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSeverity_Validate(t *testing.T) { + testCases := []struct { + desc string + rule *SeverityRule + expected string + }{ + { + desc: "empty rule", + rule: &SeverityRule{}, + expected: "at least 1 of (text, source, path[-except], linters) should be set", + }, + { + desc: "invalid path rule", + rule: &SeverityRule{ + BaseRule: BaseRule{ + Path: "**test", + }, + }, + expected: "invalid path regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid path-except rule", + rule: &SeverityRule{ + BaseRule: BaseRule{ + PathExcept: "**test", + }, + }, + expected: "invalid path-except regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid text rule", + rule: &SeverityRule{ + BaseRule: BaseRule{ + Text: "**test", + }, + }, + expected: "invalid text regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "invalid source rule", + rule: &SeverityRule{ + BaseRule: BaseRule{ + Source: "**test", + }, + }, + expected: "invalid source regex: error parsing regexp: missing argument to repetition operator: `*`", + }, + { + desc: "path and path-expect", + rule: &SeverityRule{ + BaseRule: BaseRule{ + Path: "test", + PathExcept: "test", + }, + }, + expected: "path and path-except should be set at the same time", + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + err := test.rule.Validate() + require.EqualError(t, err, test.expected) + }) + } +} From 0d2ad5fee233ddff7fc1b15dc5f31a35b9426783 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Tue, 27 Feb 2024 14:28:16 +0100 Subject: [PATCH 2/2] review --- pkg/config/issues.go | 2 +- pkg/config/issues_test.go | 77 ++++++++++++++++++++++++++++++++++++- pkg/config/severity_test.go | 13 ++++++- 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/pkg/config/issues.go b/pkg/config/issues.go index b81c888bfbd8..8725bde82e97 100644 --- a/pkg/config/issues.go +++ b/pkg/config/issues.go @@ -156,7 +156,7 @@ func (b *BaseRule) Validate(minConditionsCount int) error { } if b.Path != "" && b.PathExcept != "" { - return errors.New("path and path-except should be set at the same time") + return errors.New("path and path-except should not be set at the same time") } nonBlank := 0 diff --git a/pkg/config/issues_test.go b/pkg/config/issues_test.go index 2c17fce2c4ba..9d590ea85e57 100644 --- a/pkg/config/issues_test.go +++ b/pkg/config/issues_test.go @@ -10,7 +10,7 @@ import ( func TestGetExcludePatterns(t *testing.T) { patterns := GetExcludePatterns(nil) - assert.Equal(t, patterns, DefaultExcludePatterns) + assert.Equal(t, DefaultExcludePatterns, patterns) } func TestGetExcludePatterns_includes(t *testing.T) { @@ -116,7 +116,7 @@ func TestExcludeRule_Validate(t *testing.T) { PathExcept: "test", }, }, - expected: "path and path-except should be set at the same time", + expected: "path and path-except should not be set at the same time", }, } @@ -130,3 +130,76 @@ func TestExcludeRule_Validate(t *testing.T) { }) } } + +func TestExcludeRule_Validate_error(t *testing.T) { + testCases := []struct { + desc string + rule *ExcludeRule + }{ + { + desc: "path and linter", + rule: &ExcludeRule{ + BaseRule{ + Path: "test", + Linters: []string{"a"}, + }, + }, + }, + { + desc: "path-except and linter", + rule: &ExcludeRule{ + BaseRule{ + PathExcept: "test", + Linters: []string{"a"}, + }, + }, + }, + { + desc: "text and linter", + rule: &ExcludeRule{ + BaseRule{ + Text: "test", + Linters: []string{"a"}, + }, + }, + }, + { + desc: "source and linter", + rule: &ExcludeRule{ + BaseRule{ + Source: "test", + Linters: []string{"a"}, + }, + }, + }, + { + desc: "path and text", + rule: &ExcludeRule{ + BaseRule{ + Path: "test", + Text: "test", + }, + }, + }, + { + desc: "path and text and linter", + rule: &ExcludeRule{ + BaseRule{ + Path: "test", + Text: "test", + Linters: []string{"a"}, + }, + }, + }, + } + + for _, test := range testCases { + test := test + t.Run(test.desc, func(t *testing.T) { + t.Parallel() + + err := test.rule.Validate() + require.NoError(t, err) + }) + } +} diff --git a/pkg/config/severity_test.go b/pkg/config/severity_test.go index ddcc6d6a9440..60c2ad25707d 100644 --- a/pkg/config/severity_test.go +++ b/pkg/config/severity_test.go @@ -7,6 +7,17 @@ import ( ) func TestSeverity_Validate(t *testing.T) { + rule := &SeverityRule{ + BaseRule: BaseRule{ + Path: "test", + }, + } + + err := rule.Validate() + require.NoError(t, err) +} + +func TestSeverity_Validate_error(t *testing.T) { testCases := []struct { desc string rule *SeverityRule @@ -61,7 +72,7 @@ func TestSeverity_Validate(t *testing.T) { PathExcept: "test", }, }, - expected: "path and path-except should be set at the same time", + expected: "path and path-except should not be set at the same time", }, }