From 54fdcc2b8783c1df1f6b8183f79b4c7edcac679e Mon Sep 17 00:00:00 2001 From: sspaink Date: Fri, 9 May 2025 13:11:15 -0500 Subject: [PATCH] refactor: don't return error from opaTest the error is ignored and only printed to `testParams.errOutput` Signed-off-by: sspaink --- cmd/test.go | 59 +++++++++---------- cmd/test_test.go | 145 ++++++++++++++++++++++++++++------------------- 2 files changed, 116 insertions(+), 88 deletions(-) diff --git a/cmd/test.go b/cmd/test.go index 72ea93f042..5b2e09f811 100644 --- a/cmd/test.go +++ b/cmd/test.go @@ -92,21 +92,19 @@ func (p *testCommandParams) RegoVersion() ast.RegoVersion { return ast.DefaultRegoVersion } -func opaTest(args []string, testParams testCommandParams) (int, error) { +func opaTest(args []string, testParams testCommandParams) int { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - var err error - if testParams.outputFormat.String() == benchmarkGoBenchOutput && !testParams.benchmark { errMsg := "cannot use output format %s without running benchmarks (--bench)\n" - fmt.Fprintf(testParams.errOutput, errMsg, benchmarkGoBenchOutput) - return 0, fmt.Errorf(errMsg, benchmarkGoBenchOutput) + _, _ = fmt.Fprintf(testParams.errOutput, errMsg, benchmarkGoBenchOutput) + return 0 } if !isThresholdValid(testParams.threshold) { - fmt.Fprintln(testParams.errOutput, "Code coverage threshold must be between 0 and 100") - return 1, err + _, _ = fmt.Fprintln(testParams.errOutput, "Code coverage threshold must be between 0 and 100") + return 1 } filter := loaderFilter{ @@ -123,41 +121,41 @@ func opaTest(args []string, testParams testCommandParams) (int, error) { ProcessAnnotation: true, } + var err error if testParams.bundleMode { bundles, err = tester.LoadBundlesWithParserOptions(args, filter.Apply, popts) store = inmem.NewWithOpts(inmem.OptRoundTripOnWrite(false)) } else { modules, store, err = tester.LoadWithParserOptions(args, filter.Apply, popts) } - if err != nil { - fmt.Fprintln(testParams.errOutput, err) - return 1, err + _, _ = fmt.Fprintln(testParams.errOutput, err) + return 1 } txn, err := store.NewTransaction(ctx, storage.WriteParams) if err != nil { - fmt.Fprintln(testParams.errOutput, err) - return 1, err + _, _ = fmt.Fprintln(testParams.errOutput, err) + return 1 } runner, reporter, err := compileAndSetupTests(ctx, testParams, store, txn, modules, bundles) if err != nil { store.Abort(ctx, txn) - fmt.Fprintln(testParams.errOutput, err) - return 1, err + _, _ = fmt.Fprintln(testParams.errOutput, err) + return 1 } success := true for range testParams.count { - exitCode, err := runTests(ctx, txn, runner, reporter, testParams) + exitCode, _ := runTests(ctx, txn, runner, reporter, testParams) if exitCode != 0 { success = false store.Abort(ctx, txn) if testParams.watch { break } - return exitCode, err + return exitCode } } @@ -166,7 +164,7 @@ func opaTest(args []string, testParams testCommandParams) (int, error) { } if !testParams.watch { - return 0, nil + return 0 } done := make(chan struct{}) @@ -176,7 +174,7 @@ func opaTest(args []string, testParams testCommandParams) (int, error) { <-testParams.stopChan done <- struct{}{} - return 0, nil + return 0 } func runTests(ctx context.Context, txn storage.Transaction, runner *tester.Runner, reporter tester.Reporter, testParams testCommandParams) (int, error) { @@ -197,7 +195,7 @@ func runTests(ctx context.Context, txn storage.Transaction, runner *tester.Runne } if err != nil { - fmt.Fprintln(testParams.errOutput, err) + _, _ = fmt.Fprintln(testParams.errOutput, err) return 1, err } @@ -218,9 +216,10 @@ func runTests(ctx context.Context, txn storage.Transaction, runner *tester.Runne }() if err := reporter.Report(dup); err != nil { - fmt.Fprintln(testParams.errOutput, err) + _, _ = fmt.Fprintln(testParams.errOutput, err) if !testParams.benchmark { - if _, ok := err.(*cover.CoverageThresholdError); ok { + var coverageThresholdError *cover.CoverageThresholdError + if errors.As(err, &coverageThresholdError) { return 2, err } } @@ -263,7 +262,7 @@ func isThresholdValid(t float64) bool { func startWatcher(ctx context.Context, testParams testCommandParams, paths []string, store storage.Store, done chan struct{}) { watcher, err := pathwatcher.CreatePathWatcher(paths) if err != nil { - fmt.Fprintln(testParams.errOutput, "Error creating path watcher: ", err) + _, _ = fmt.Fprintln(testParams.errOutput, "Error creating path watcher: ", err) os.Exit(1) } readWatcher(ctx, testParams, watcher, paths, store, done) @@ -271,9 +270,8 @@ func startWatcher(ctx context.Context, testParams testCommandParams, paths []str func readWatcher(ctx context.Context, testParams testCommandParams, watcher *fsnotify.Watcher, paths []string, store storage.Store, done chan struct{}) { for { - - fmt.Fprintln(testParams.output, strings.Repeat("*", 80)) - fmt.Fprintln(testParams.output, "Watching for changes ...") + _, _ = fmt.Fprintln(testParams.output, strings.Repeat("*", 80)) + _, _ = fmt.Fprintln(testParams.output, "Watching for changes ...") select { case evt := <-watcher.Events: removalMask := fsnotify.Remove | fsnotify.Rename @@ -286,7 +284,7 @@ func readWatcher(ctx context.Context, testParams testCommandParams, watcher *fsn processWatcherUpdate(ctx, testParams, paths, removed, store) } case <-done: - watcher.Close() + _ = watcher.Close() return } } @@ -313,7 +311,7 @@ func processWatcherUpdate(ctx context.Context, testParams testCommandParams, pat }) if err != nil { - fmt.Fprintln(testParams.output, err) + _, _ = fmt.Fprintln(testParams.output, err) return } @@ -338,7 +336,7 @@ func processWatcherUpdate(ctx context.Context, testParams testCommandParams, pat }) if err != nil { - fmt.Fprintln(testParams.output, err) + _, _ = fmt.Fprintln(testParams.output, err) } } @@ -385,7 +383,7 @@ func compileAndSetupTests(ctx context.Context, testParams testCommandParams, sto if testParams.coverage { if testParams.benchmark { errMsg := "coverage reporting is not supported when benchmarking tests" - fmt.Fprintln(testParams.errOutput, errMsg) + _, _ = fmt.Fprintln(testParams.errOutput, errMsg) return nil, nil, errors.New(errMsg) } cov = cover.New() @@ -539,8 +537,7 @@ recommended as some updates might cause them to be dropped by OPA. }, Run: func(_ *cobra.Command, args []string) { - exitCode, _ := opaTest(args, testParams) - os.Exit(exitCode) + os.Exit(opaTest(args, testParams)) }, } diff --git a/cmd/test_test.go b/cmd/test_test.go index 973905a07b..ffff41d8d2 100644 --- a/cmd/test_test.go +++ b/cmd/test_test.go @@ -335,9 +335,9 @@ FAIL: 1/1 testParams.varValues = tc.includeVars _ = testParams.explain.Set(explainModeFull) - _, err := opaTest([]string{root}, testParams) - if err != nil { - t.Fatalf("Unexpected error: %s", err) + errorCode := opaTest([]string{root}, testParams) + if errorCode != 2 { + t.Fatalf("Unexpected error code: %d", errorCode) } actual := buf.String() @@ -1484,9 +1484,9 @@ FAIL: 1/1 testParams.varValues = true _ = testParams.explain.Set(explainModeFull) - _, err := opaTest([]string{root}, testParams) - if err != nil { - t.Fatalf("Unexpected error: %s", err) + exitCode := opaTest([]string{root}, testParams) + if exitCode != 2 { + t.Fatalf("Unexpected error code: %d", exitCode) } actual := r.ReplaceAllString(buf.String(), "FAIL (%TIME%)") @@ -1520,7 +1520,7 @@ test_p if { testParams.bundleMode = false testParams.ignore = []string{"broken.rego"} - exitCode, _ = opaTest([]string{root}, testParams) + exitCode = opaTest([]string{root}, testParams) }) if exitCode > 0 { @@ -1547,7 +1547,7 @@ test_p if { testParams.errOutput = io.Discard testParams.bundleMode = true testParams.ignore = []string{"broken.rego"} - exitCode, _ = opaTest([]string{root}, testParams) + exitCode = opaTest([]string{root}, testParams) }) if exitCode > 0 { @@ -1555,24 +1555,24 @@ test_p if { } } -func testSchemasAnnotation(rego string) (int, error) { +func testSchemasAnnotation(rego string) (int, []byte) { files := map[string]string{ "test.rego": rego, } var exitCode int - var err error + var buf bytes.Buffer test.WithTempFS(files, func(path string) { regoFilePath := filepath.Join(path, "test.rego") testParams := newTestCommandParams() testParams.count = 1 - testParams.errOutput = io.Discard + testParams.errOutput = &buf - exitCode, err = opaTest([]string{regoFilePath}, testParams) + exitCode = opaTest([]string{regoFilePath}, testParams) }) - return exitCode, err + return exitCode, buf.Bytes() } // Assert that 'schemas' annotations with schema ref are ignored, but not inlined schemas @@ -1612,17 +1612,19 @@ test_p if { p with input.foo as 42 }` - exitCode, err := testSchemasAnnotation(policyWithInlinedSchema) + exitCode, errOutput := testSchemasAnnotation(policyWithInlinedSchema) // We expect an error here, as inlined schemas are always used for type checking if exitCode == 0 { t.Fatalf("didn't get expected error when inlined schema is present") - } else if !strings.Contains(err.Error(), "rego_type_error: match error") { - t.Fatalf("didn't get expected %s error when inlined schema is present; got: %v", ast.TypeErr, err) + } + + if !strings.Contains(string(errOutput), "rego_type_error: match error") { + t.Fatalf("didn't get expected %s error when inlined schema is present; got: %v", ast.TypeErr, string(errOutput)) } } -func testSchemasAnnotationWithJSONFile(rego string, schema string) (int, error) { +func testSchemasAnnotationWithJSONFile(rego string, schema string) (int, []byte) { files := map[string]string{ "test.rego": rego, @@ -1630,18 +1632,19 @@ func testSchemasAnnotationWithJSONFile(rego string, schema string) (int, error) } var exitCode int - var err error + var buf bytes.Buffer test.WithTempFS(files, func(path string) { regoFilePath := filepath.Join(path, "test.rego") testParams := newTestCommandParams() testParams.count = 1 testParams.schema.path = path - testParams.errOutput = io.Discard + testParams.errOutput = &buf - exitCode, err = opaTest([]string{regoFilePath}, testParams) + exitCode = opaTest([]string{regoFilePath}, testParams) }) - return exitCode, err + + return exitCode, buf.Bytes() } func TestJSONSchemaSuccess(t *testing.T) { @@ -1676,9 +1679,9 @@ test_p if { "additionalProperties": false }` - _, err := testSchemasAnnotationWithJSONFile(regoContents, schema) - if err != nil { - t.Fatalf("unexpected error: %s", err) + errorCode, _ := testSchemasAnnotationWithJSONFile(regoContents, schema) + if errorCode != 0 { + t.Fatalf("unexpected error code: %d", errorCode) } } @@ -1715,11 +1718,13 @@ test_p if { "additionalProperties": false }` - exitCode, err := testSchemasAnnotationWithJSONFile(regoContents, schema) + exitCode, errOutput := testSchemasAnnotationWithJSONFile(regoContents, schema) if exitCode == 0 { t.Fatalf("didn't get expected error when schema is present and is defining a different type than being used.") - } else if !strings.Contains(err.Error(), "rego_type_error: match error") { - t.Fatalf("didn't get expected %s error when schema is defining a different type than being used; got: %v", ast.TypeErr, err) + } + + if !strings.Contains(string(errOutput), "rego_type_error: match error") { + t.Fatalf("didn't get expected %s error when schema is defining a different type than being used; got: %v", ast.TypeErr, string(errOutput)) } } @@ -1745,7 +1750,7 @@ test_p if { done := make(chan struct{}) go func() { - _, _ = opaTest([]string{root}, testParams) + _ = opaTest([]string{root}, testParams) <-done }() @@ -1763,7 +1768,10 @@ test_p if { if err != nil { t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } r := regexp.MustCompile(`FAIL \(.*s\)`) expected = `%ROOT%/policy_test.rego: @@ -1789,7 +1797,10 @@ Watching for changes ... t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } expected = `PASS: 1/1 ******************************************************************************** @@ -1852,7 +1863,7 @@ test_p { done := make(chan struct{}) go func() { - _, _ = opaTest([]string{root}, testParams) + _ = opaTest([]string{root}, testParams) <-done }() @@ -1870,7 +1881,10 @@ test_p { if err != nil { t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } r := regexp.MustCompile(`FAIL \(.*s\)`) expected = `%ROOT%/policy_test.rego: @@ -1896,7 +1910,10 @@ Watching for changes ... t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } expected = `PASS: 1/1 ******************************************************************************** @@ -1957,7 +1974,7 @@ test_p if { done := make(chan struct{}) go func() { - _, _ = opaTest([]string{root}, testParams) + _ = opaTest([]string{root}, testParams) <-done }() @@ -1975,7 +1992,10 @@ test_p if { if err != nil { t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } r := regexp.MustCompile(`FAIL \(.*s\)`) expected = `%ROOT%/policy.rego: @@ -2001,7 +2021,10 @@ Watching for changes ... t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } expected = `PASS: 1/1 ******************************************************************************** @@ -2041,7 +2064,7 @@ test_p if { done := make(chan struct{}) go func() { - _, _ = opaTest([]string{root}, testParams) + _ = opaTest([]string{root}, testParams) <-done }() @@ -2059,7 +2082,10 @@ test_p if { if err != nil { t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } r := regexp.MustCompile(`FAIL \(.*s\)`) expected = `%ROOT%/policy.rego: @@ -2084,7 +2110,10 @@ Watching for changes ... if err != nil { t.Fatal(err) } - f.Close() + err = f.Close() + if err != nil { + t.Fatal(err) + } expected = `PASS: 1/1 ******************************************************************************** @@ -2165,7 +2194,7 @@ test_p if { done := make(chan struct{}) go func() { - _, _ = opaTest([]string{root}, testParams) + _ = opaTest([]string{root}, testParams) <-done }() @@ -2185,7 +2214,10 @@ test_p if { t.Fatal(err) } } - f.Close() + err := f.Close() + if err != nil { + t.Fatal(err) + } if !test.Eventually(t, 2*time.Second, func() bool { expected := strings.ReplaceAll(tc.expectedOutput, "%ROOT%", root) @@ -2197,11 +2229,14 @@ test_p if { // write data to empty file f, _ = os.OpenFile(path.Join(root, tc.fileName), os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644) - _, err := f.WriteString(tc.fixedFile) + _, err = f.WriteString(tc.fixedFile) + if err != nil { + t.Fatal(err) + } + err = f.Close() if err != nil { t.Fatal(err) } - f.Close() expected = "Watching for changes ..." if !test.Eventually(t, 2*time.Second, func() bool { @@ -2219,13 +2254,12 @@ test_p if { } } -func testExitCode(rego string, skipExitZero bool) (int, error) { +func testExitCode(rego string, skipExitZero bool) int { files := map[string]string{ "test.rego": rego, } var exitCode int - var err error test.WithTempFS(files, func(path string) { regoFilePath := filepath.Join(path, "test.rego") @@ -2235,9 +2269,9 @@ func testExitCode(rego string, skipExitZero bool) (int, error) { testParams.errOutput = io.Discard testParams.output = io.Discard - exitCode, err = opaTest([]string{regoFilePath}, testParams) + exitCode = opaTest([]string{regoFilePath}, testParams) }) - return exitCode, err + return exitCode } func TestExitCode(t *testing.T) { @@ -2314,7 +2348,7 @@ func TestExitCode(t *testing.T) { for name, tc := range testCases { t.Run(name, func(t *testing.T) { - exitCode, _ := testExitCode(tc.Test, tc.ExitZeroOnSkipped) + exitCode := testExitCode(tc.Test, tc.ExitZeroOnSkipped) if exitCode != tc.ExpectedExitCode { t.Errorf("Expected exit code to be %d but got %d", tc.ExpectedExitCode, exitCode) @@ -2427,7 +2461,7 @@ Lines not covered: testParams.count = 1 testParams.errOutput = &buf - exitCode, _ := opaTest([]string{root}, testParams) + exitCode := opaTest([]string{root}, testParams) if exitCode != tc.expectedExitCode { t.Fatalf("unexpected exit code: %d", exitCode) } @@ -2547,7 +2581,7 @@ test_l if { paths = []string{root} } - exitCode, _ := opaTest(paths, testParams) + exitCode := opaTest(paths, testParams) if len(tc.expErrs) > 0 { if exitCode == 0 { t.Fatalf("expected non-zero exit code") @@ -2896,7 +2930,7 @@ test_l if { paths = []string{root} } - exitCode, _ := opaTest(paths, testParams) + exitCode := opaTest(paths, testParams) if len(tc.expErrs) > 0 { if exitCode == 0 { t.Fatalf("expected non-zero exit code") @@ -3150,7 +3184,7 @@ test_l if { paths = []string{root} } - exitCode, _ := opaTest(paths, testParams) + exitCode := opaTest(paths, testParams) if tc.expErr != "" { if exitCode == 0 { t.Fatalf("expected non-zero exit code") @@ -3482,7 +3516,7 @@ test_l if { testParams.output = &buf testParams.errOutput = &errBuf - exitCode, _ := opaTest([]string{p}, testParams) + exitCode := opaTest([]string{p}, testParams) if tc.expErr != "" { if exitCode == 0 { t.Fatalf("expected non-zero exit code") @@ -3526,12 +3560,9 @@ func TestTestBenchFailingTest(t *testing.T) { tp.benchmark = true tp.count = 1 - exitCode, err := opaTest([]string{fp}, tp) + exitCode := opaTest([]string{fp}, tp) if exitCode == 0 { t.Fatalf("Expected exit code 0, got %d", exitCode) } - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } }) }