fix(cli): make opa test --exit-zero-on-skipped fail with failing test rules

CHANGELOG.md

@@ -5,6 +5,16 @@ project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+### Breaking changes
+
+Since its introduction in 0.34.0, the `--exit-zero-on-skipped` option always made the `opa test` command return an exit code 0. When used, it now returns the exit code 0 only if no failed tests were found.
+
+Test runs on existing projects using `--exit-zero-on-skipped` will fail if any failed tests were inhibited by this behavior.
+
+### Tooling, SDK, and Runtime
+
+- `opa test`: Fix `--exit-zero-on-skipped` behavior to make test runs fail with failing test rules ([#6126](https://github.com/open-policy-agent/opa/issues/6126)) reported and authored by @fdaguin
+
 ## 0.55.0
 
 > **_NOTES:_**

cmd/test.go

@@ -179,12 +179,10 @@ func runTests(ctx context.Context, txn storage.Transaction, runner *tester.Runne
 	go func() {
 		defer close(dup)
 		for tr := range ch {
-			if !tr.Pass() && !testParams.skipExitZero {
-				exitCode = 2
-			}
-			if tr.Skip && exitCode == 0 && testParams.skipExitZero {
-				// there is a skipped test, adding the flag -z exits 0 if there are no failures
-				exitCode = 0
+			if !tr.Pass() {
+				if !(tr.Skip && testParams.skipExitZero) {
+					exitCode = 2
+				}
 			}
 			tr.Trace = filterTrace(&testParams, tr.Trace)
 			dup <- tr

cmd/test_test.go

@@ -752,6 +752,101 @@ Watching for changes ...`,
 	}
 }
 
+func testExitCode(rego string, skipExitZero bool) (int, error) {
+	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")
+
+		testParams := newTestCommandParams()
+		testParams.count = 1
+		testParams.skipExitZero = skipExitZero
+
+		exitCode, err = opaTest([]string{regoFilePath}, testParams)
+	})
+	return exitCode, err
+}
+
+func TestExitCode(t *testing.T) {
+	testCases := map[string]struct {
+		Test              string
+		ExitZeroOnSkipped bool
+		ExpectedExitCode  int
+	}{
+		"pass when no failed or skipped tests": {
+			Test: `package foo
+			test_pass { true }
+			`,
+			ExitZeroOnSkipped: false,
+			ExpectedExitCode:  0,
+		},
+		"fail when failed tests": {
+			Test: `package foo
+			test_pass { true }
+			test_fail { false }
+			`,
+			ExitZeroOnSkipped: false,
+			ExpectedExitCode:  2,
+		},
+		"fail when skipped tests": {
+			Test: `package foo
+			test_pass { true }
+			todo_test_skip { true }
+			`,
+			ExitZeroOnSkipped: false,
+			ExpectedExitCode:  2,
+		},
+		"fail when failed tests and skipped tests": {
+			Test: `package foo
+			test_pass { true }
+			test_fail { false }
+			todo_test_skip { true }
+			`,
+			ExitZeroOnSkipped: false,
+			ExpectedExitCode:  2,
+		},
+		"pass when skipped tests and exit zero on skipped": {
+			Test: `package foo
+			test_pass { true }
+			todo_test_skip { true }
+			`,
+			ExitZeroOnSkipped: true,
+			ExpectedExitCode:  0,
+		},
+		"fail when failed tests and exit zero on skipped": {
+			Test: `package foo
+			test_pass { true }
+			test_fail { false }
+			`,
+			ExitZeroOnSkipped: true,
+			ExpectedExitCode:  2,
+		},
+		"fail when failed tests, skipped tests and exit zero on skipped": {
+			Test: `package foo
+			test_pass { true }
+			test_fail { false }
+			todo_test_skip { true }
+			`,
+			ExitZeroOnSkipped: true,
+			ExpectedExitCode:  2,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			exitCode, _ := testExitCode(tc.Test, tc.ExitZeroOnSkipped)
+
+			if exitCode != tc.ExpectedExitCode {
+				t.Errorf("Expected exit code to be %d but got %d", tc.ExpectedExitCode, exitCode)
+			}
+		})
+	}
+}
+
 type blockingWriter struct {
 	m   sync.Mutex
 	buf bytes.Buffer