feat: improve file loading robustness (#310)

Until now, go-ftw didn't know what to do with YAML files that were not test files. With the new platform override files (also YAML), it is very easy to also load them when globbing. This commit improves the output for such issues and continues execution, ignoring any files that can't be loaded.
coreruleset · Jun 1, 2024 · f35690f · f35690f
1 parent 2c0286e
commit f35690f
Show file tree

Hide file tree

Showing 22 changed files with 100 additions and 136 deletions.
diff --git a/cmd/check_test.go b/cmd/check_test.go
@@ -16,8 +16,6 @@ import (
 var checkFileContents = `---
 meta:
   author: "go-ftw"
-  enabled: true
-  name: "mock-TestRunTests_Run.yaml"
   description: "Test file for go-ftw"
 tests:
   - # Standard GET request

diff --git a/cmd/run_test.go b/cmd/run_test.go
@@ -23,8 +23,6 @@ import (
 var testFileContentsTemplate = `---
 meta:
   author: "go-ftw"
-  enabled: true
-  name: "mock-TestRunTests_Run.yaml"
   description: "Test file for go-ftw"
 tests:
   - # Standard GET request

diff --git a/go.sum b/go.sum
@@ -84,8 +84,6 @@ github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/rs/xid v1.5.0/go.mod h1:trrq9SKmegXys3aeAKXMUTdJsYXVwGY3RLcfgqegfbg=
-github.com/rs/zerolog v1.32.0 h1:keLypqrlIjaFsbmJOBdB/qvyF8KEtCWHwobLp5l/mQ0=
-github.com/rs/zerolog v1.32.0/go.mod h1:/7mN4D5sKwJLZQ2b/znpjC3/GQWY/xaDXUM0kKWRHss=
 github.com/rs/zerolog v1.33.0 h1:1cU2KZkvPxNyfgEmhHAz/1A9Bz+llsdYzklWFzgp0r8=
 github.com/rs/zerolog v1.33.0/go.mod h1:/7mN4D5sKwJLZQ2b/znpjC3/GQWY/xaDXUM0kKWRHss=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=

diff --git a/runner/run.go b/runner/run.go
@@ -86,7 +86,6 @@ func RunTest(runContext *TestRunContext, ftwTest *test.FTWTest) error {
 		// if we received a particular test ID, skip until we find it
 		if needToSkipTest(runContext.Include, runContext.Exclude, &testCase) {
 			runContext.Stats.addResultToStats(Skipped, &testCase)
-			log.Trace().Msgf("\tskipping %s", testCase.IdString())
 			continue
 		}
 		test.ApplyPlatformOverrides(runContext.Config, &testCase)

diff --git a/runner/run_test.go b/runner/run_test.go
@@ -34,9 +34,6 @@ var testConfigMap = map[string]string{
 testoverride:
   ignore:
     "920400-1": "This test result must be ignored"
-`,
-	"TestDisabledRun": `---
-mode: 'cloud'
 `,
 	"TestBrokenOverrideRun": `---
 testoverride:
@@ -96,7 +93,6 @@ testoverride:
 
 var destinationMap = map[string]string{
 	"TestBrokenOverrideRun": "http://example.com:1234",
-	"TestDisabledRun":       "http://example.com:1234",
 }
 
 type runTestSuite struct {
@@ -352,12 +348,6 @@ func (s *runTestSuite) TestBrokenPortOverrideRun() {
 	s.LessOrEqual(0, res.Stats.TotalFailed(), "Oops, test run failed!")
 }
 
-func (s *runTestSuite) TestDisabledRun() {
-	res, err := Run(s.cfg, s.ftwTests, RunnerConfig{}, s.out)
-	s.Require().NoError(err)
-	s.LessOrEqual(0, res.Stats.TotalFailed(), "Oops, test run failed!")
-}
-
 func (s *runTestSuite) TestLogsRun() {
 	res, err := Run(s.cfg, s.ftwTests, RunnerConfig{}, s.out)
 	s.Require().NoError(err)

diff --git a/runner/testdata/TestBrokenOverrideRun.yaml b/runner/testdata/TestBrokenOverrideRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestBrokenOverrideRun.yaml"
   description: "Example Override Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestBrokenPortOverrideRun.yaml b/runner/testdata/TestBrokenPortOverrideRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestBrokenPortOverrideRun.yaml"
   description: "Example Override Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestCloudRun.yaml b/runner/testdata/TestCloudRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestCloudRun.yaml"
   description: "Example Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestDisabledRun.yaml b/runner/testdata/TestDisabledRun.yaml
diff --git a/runner/testdata/TestFailedTestsRun.yaml b/runner/testdata/TestFailedTestsRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestFailedTestsRun.yaml"
   description: "Example Test"
 tests:
   - test_id: 990

diff --git a/runner/testdata/TestIgnoredTestsRun.yaml b/runner/testdata/TestIgnoredTestsRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestIgnoredTestsRun.yaml"
   description: "Example Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestLogsRun.yaml b/runner/testdata/TestLogsRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestLogsRun.yaml"
   description: "Example Test"
 tests:
   - test_id: 200

diff --git a/runner/testdata/TestOverrideRun.yaml b/runner/testdata/TestOverrideRun.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestOverrideRun.yaml"
   description: "Example Override Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestRetryOnce.yaml b/runner/testdata/TestRetryOnce.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestRetryOnce.yaml"
   description: "Example Test"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestRunMultipleMatches.yaml b/runner/testdata/TestRunMultipleMatches.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestRunMultipleMatches.yaml"
   description: "Example Test with multiple expected outputs per single rule"
 tests:
   - test_id: 1

diff --git a/runner/testdata/TestRunTests_Run.yaml b/runner/testdata/TestRunTests_Run.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "tester"
-  enabled: true
-  name: "TestRunTests_Run.yaml"
   description: "Example Test"
 tests:
   - test_id: 1

diff --git a/test/files.go b/test/files.go
@@ -30,16 +30,18 @@ func GetTestsFromFiles(globPattern string) ([]*FTWTest, error) {
 	}
 
 	for _, filePath := range testFiles {
+		fileName := path.Base(filePath)
+		log.Trace().Msgf("Loading %s", fileName)
 		yamlString, err := readFileContents(filePath)
 		if err != nil {
 			return tests, err
 		}
-		ftwTest, err := GetTestFromYaml(yamlString, path.Base(filePath))
+		ftwTest, err := GetTestFromYaml(yamlString, fileName)
 		if err != nil {
-			log.Error().Msgf("Problem detected in file %s:\n%s\n%s",
+			log.Warn().Msgf("Problem detected in file %s:\n%s\n%s",
 				filePath, yaml.FormatError(err, true, true),
 				DescribeYamlError(err))
-			return tests, err
+			continue
 		}
 
 		tests = append(tests, ftwTest)

diff --git a/test/files_test.go b/test/files_test.go
@@ -14,36 +14,33 @@ import (
 
 var yamlTest = `
 ---
-  meta:
-    author: "tester"
-    enabled: true
-    name: "911100.yaml"
-    description: "Description"
-  rule_id: 911100
-  tests:
-    - test_id: 1
-      stages:
-        - input:
-            autocomplete_headers: false
-            dest_addr: "127.0.0.1"
-            port: 80
-            headers:
-              User-Agent: "ModSecurity CRS 3 Tests"
-              Host: "localhost"
-          output:
-            no_log_contains: "id \"911100\""
-    -
-      test_id: 2
-      stages:
-        - input:
-            dest_addr: "127.0.0.1"
-            port: 80
-            method: "OPTIONS"
-            headers:
-              User-Agent: "ModSecurity CRS 3 Tests"
-              Host: "localhost"
-          output:
-            no_log_contains: "id \"911100\""
+meta:
+  author: "tester"
+  description: "Description"
+rule_id: 911100
+tests:
+  - test_id: 1
+    stages:
+      - input:
+          autocomplete_headers: false
+          dest_addr: "127.0.0.1"
+          port: 80
+          headers:
+            User-Agent: "ModSecurity CRS 3 Tests"
+            Host: "localhost"
+        output:
+          no_log_contains: "id \"911100\""
+  - test_id: 2
+    stages:
+      - input:
+          dest_addr: "127.0.0.1"
+          port: 80
+          method: "OPTIONS"
+          headers:
+            User-Agent: "ModSecurity CRS 3 Tests"
+            Host: "localhost"
+        output:
+          no_log_contains: "id \"911100\""
 `
 
 var wrongYamlTest = `
@@ -64,7 +61,7 @@ func (s *filesTestSuite) TestGetTestFromYAML() {
 
 	for _, ft := range tests {
 		s.Equal("tester", ft.Meta.Author)
-		s.Equal("911100.yaml", ft.Meta.Name)
+		s.Equal("Description", ft.Meta.Description)
 
 		re := regexp.MustCompile("911100.*")
 

diff --git a/test/testdata/TestCheckBenchmarkCheckFiles.yaml b/test/testdata/TestCheckBenchmarkCheckFiles.yaml
@@ -1,8 +1,6 @@
 ---
 meta:
   author: "csanders-git, Franziska Bühler"
-  enabled: true
-  name: "920420.yaml"
   description: "Description"
 rule_id: 920420
 tests:

diff --git a/test/types.go b/test/types.go
@@ -4,6 +4,7 @@
 package test
 
 import (
+	"fmt"
 	"regexp"
 	"strconv"
 
@@ -125,34 +126,36 @@ func applyHeadersOverride(overrides *config.Overrides, input *Input) {
 	}
 }
 
-func postLoadTestFTWTest(ftwTest *FTWTest, fileName string) {
+func postLoadTestFTWTest(ftwTest *FTWTest, fileName string) error {
 	ftwTest.FileName = fileName
-	postLoadRuleId(ftwTest)
+	if err := postLoadRuleId(ftwTest); err != nil {
+		return err
+	}
 	for index := 0; index < len(ftwTest.Tests); index++ {
 		postLoadTest(ftwTest.RuleId, uint(index+1), &ftwTest.Tests[index])
 	}
+	return nil
 }
 
-func postLoadRuleId(ftwTest *FTWTest) {
+func postLoadRuleId(ftwTest *FTWTest) error {
 	if ftwTest.RuleId > 0 {
-		return
+		return nil
 	}
 
 	if len(ftwTest.FileName) == 0 {
-		log.Fatal().Msg("The rule_id field is required for the top-level test structure")
+		return fmt.Errorf("the rule_id field is required for the top-level test structure")
 	} else {
 		ruleIdString := regexp.MustCompile(`\d+`).FindString(ftwTest.FileName)
 		if len(ruleIdString) == 0 {
-			log.Fatal().Msg("Failed to fall back on filename to find rule ID of test. The rule_id field is required for the top-level test structure")
-			return
+			return fmt.Errorf("failed to fall back on filename to find rule ID of test. The rule_id field is required for the top-level test structure")
 		}
 		ruleId, err := strconv.ParseUint(ruleIdString, 10, 0)
 		if err != nil {
-			log.Fatal().Msgf("failed to parse rule ID from filename '%s'", ftwTest.FileName)
-			return
+			return fmt.Errorf("failed to parse rule ID from filename '%s'", ftwTest.FileName)
 		}
 		ftwTest.RuleId = uint(ruleId)
 	}
+	return nil
 }
 func postLoadTest(ruleId uint, testId uint, test *schema.Test) {
 	test.RuleId = ruleId

diff --git a/test/types_test.go b/test/types_test.go
@@ -20,8 +20,6 @@ func TestTypesTestSuite(t *testing.T) {
 var autocompleteHeadersDefaultYaml = `---
 meta:
   author: "tester"
-  enabled: true
-  name: "gotest-ftw.yaml"
   description: "Example Test"
 rule_id: 123456
 tests:
@@ -68,8 +66,6 @@ tests:
 var autocompleteHeadersFalseYaml = `---
 meta:
   author: "tester"
-  enabled: true
-  name: "gotest-ftw.yaml"
   description: "Example Test"
 rule_id: 123456
 tests:
@@ -119,8 +115,6 @@ tests:
 var autocompleteHeadersTrueYaml = `---
 meta:
   author: "tester"
-  enabled: true
-  name: "gotest-ftw.yaml"
   description: "Example Test"
 rule_id: 123456
 tests: