Add explicit check results. (#21)

The problem with using the name of the rule to classify the result is
that it becomes hard to write generally useful checks. For example,
if we need a HTTP response to have a 404 status, we can test for the
status in a policy module, but we still need to wrap that in an error
rule to propagate the result, which makes tests more verbose than they
ought to be.

This change introduces the `check` rule type, where the result of a
rule can be a map with an explicit `result` field. This allows test
authors to write policy modules that are completely self-contained
and can return results independently of the rule type. For example,
it would be possible to have a single policy function that skips a
test if `cert-manager` is not installed.

Signed-off-by: James Peach <[email protected]>

README.md

@@ -110,6 +110,96 @@ creates the stub and will update its copy when it changes.
 
 ## Writing Rego Tests
 
+## Rego test rules
+
+In a Rego fragment,  `integration-tester` evaluates all the rules
+named `skip`, `error`, `fatal` or `check`. Other names can be used
+if you prefix the rule name with one of the special result tokens,
+followed by an underscore, e.g. `error_if_not_present`.
+
+Any `skip`, `error` or `fatal` rules that evaluate to true have the
+corresponding test result. `skip` results cause the remainder of
+the test to be passed over, but it will not report an overall
+failure. `error` results cause a specific check to fail. The test
+will continue, and other errors may be detected.  `fatal` results
+cause the test to fail and end immediately.
+
+A `check` result is one that can cause a check to either pass or
+fail. For example:
+
+```Rego
+import data.builtin.results
+
+check_it_is_time[r] {
+    time.now_ns() > 10
+    r := results.Pass("it is time")
+}
+```
+
+Checks are useful for building libraries of tests that can simply
+emit results without needing to depend on the naming rules of the
+top-level query. The `data.builtin.results` package contains a set
+of helper functions that make constructing results easier:
+
+| Name | Args | Description |
+| -- | -- | -- |
+| Pass(msg) | *string* | Construct a `pass` result with the message string. |
+| Passf(msg, args) | *string*, *array* | Construct a `pass` result with a `sprintf` format string. |
+| Skip(msg) | *string* | Construct a `skip` result with the message string. |
+| Skipf(msg, args) | *string*, *array* | Construct a `skip` result with a `sprintf` format string. |
+| Error(msg) | *string* | Construct a `error` result with the message string. |
+| Errorf(msg, args) | *string*, *array* | Construct a `error` result with a `sprintf` format string. |
+| Fatal(msg) | *string* | Construct a `fatal` result with the message string. |
+| Fatal(msg, args) | *string*, *array* | Construct a `fatal` result with a `sprintf` format string. |
+
+## Rego rule results
+
+`integration-tester` supports a number of result formats for Rego
+rules. The recommended format is that used by the `data.builtin.result`
+module, which is a map with well-known keys `result` and `msg`:
+
+```
+{
+    "result": "Pass",
+    "msg": "This test passes",
+}
+```
+
+`integration-tester` also supports the following result types:
+
+* **boolean:** The rule triggers with no additional information.
+* **string:** The rule triggers and the string result gives an additional reason
+* **string array:** The rule triggers and the elements of the string result are joined with `\n`
+* **map with `msg` key:** The rule triggers and the string result comes from the `msg` key
+
+This Rego sample demonstrates the supported result formats:
+
+```
+error_if_true = e {
+    e := true
+}
+
+error_with_message[m] {
+    m = "message 1"
+}
+
+error_with_message[m] {
+    m := "message 2"
+}
+
+error_with_long_message[m] {
+    m := [
+        "line one",
+        "line 2",
+    ]
+}
+
+fatal_if_message[{"msg": m}] {
+    m := "fatal check"
+}
+
+```
+
 ## Skipping tests
 
 If there is a skip rule (any rule whose name begins with the string

pkg/builtin/checkResult.rego

@@ -0,0 +1,46 @@
+package builtin.result
+
+PassResult := "Pass"
+SkipResult := "Skip"
+ErrorResult := "Error"
+FatalResult := "Fatal"
+
+Pass(msg) = {
+    "result": PassResult,
+    "msg": msg,
+}
+
+Passf(fmt, args) = {
+    "result": PassResult,
+    "msg": sprintf(fmt, args),
+}
+
+Skip(msg) = {
+    "result": SkipResult,
+    "msg": msg,
+}
+
+Skipf(fmt, args) = {
+    "result": SkipResult,
+    "msg": sprintf(fmt, args),
+}
+
+Error(msg) = {
+    "result": ErrorResult,
+    "msg": msg,
+}
+
+Errorf(fmt, args) = {
+    "result": ErrorResult,
+    "msg": sprintf(fmt, args),
+}
+
+Fatal(msg) = {
+    "result": FatalResult,
+    "msg": msg,
+}
+
+Fatalf(fmt, args) = {
+    "result": FatalResult,
+    "msg": sprintf(fmt, args),
+}

pkg/driver/rego.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"log"
 	"strings"
 
 	"github.com/projectcontour/integration-tester/pkg/must"
@@ -241,10 +240,8 @@ func (r *regoDriver) Eval(m *ast.Module, opts ...RegoOpt) ([]result.Result, erro
 		// In each result, the Text is the expression that we
 		// queried, and value is one or more bound messages.
 		for _, r := range resultSet {
-			for _, e := range r.Expressions {
-				if r := extractResult(e); r != nil {
-					checkResults = append(checkResults, *r)
-				}
+			for _, expr := range r.Expressions {
+				checkResults = append(checkResults, extractResult(expr)...)
 			}
 		}
 
@@ -266,79 +263,112 @@ func (r *regoDriver) Eval(m *ast.Module, opts ...RegoOpt) ([]result.Result, erro
 // "msg". In the future, we could accept other types, but
 //
 // See also https://github.com/instrumenta/conftest/pull/243.
-func extractResult(expr *rego.ExpressionValue) *result.Result {
-	res := result.Result{
-		Severity: severityForRuleName(expr.Text),
-		Message:  fmt.Sprintf("raised predicate %q", expr.Text),
-	}
+func extractResult(expr *rego.ExpressionValue) []result.Result {
+	var results []result.Result
 
 	switch value := expr.Value.(type) {
+	case []interface{}:
+		for _, v := range value {
+			results = append(results,
+				extractOneResult(severityForRuleName(expr.Text), v),
+			)
+		}
+
+	default:
+		results = append(results,
+			extractOneResult(severityForRuleName(expr.Text), value),
+		)
+	}
+
+	// Prefix any results with the name of the query predicate that emitted them.
+	for i := range results {
+		prefix := fmt.Sprintf("raised predicate %q", expr.Text)
+		if results[i].Message == "" {
+			results[i].Message = prefix
+		} else {
+			results[i].Message = utils.JoinLines(prefix, results[i].Message)
+		}
+	}
+
+	return results
+}
+
+func extractOneResult(severity result.Severity, v interface{}) result.Result {
+	// If this is a []string, then we have the result already.
+	if s, ok := utils.AsStringSlice(v); ok {
+		return result.Result{
+			Severity: severity,
+			Message:  utils.JoinLines(s...),
+		}
+	}
+
+	switch value := v.(type) {
+	// This might be a boolean if the rule was this:
+	//	`error { ... }`
+	//
+	// Rego only returns the results of boolean rules
+	// if the rule was true, so the value of the bool
+	// result doesn't matter. We just know there's no
+	// message.
 	case bool:
-		// This might be a boolean if the rule was this:
-		//	`error { ... }`
-		//
-		// Rego only returns the results of boolean rules
-		// if the rule was true, so the value of the bool
-		// result doesn't matter. We just know there's no
-		// message.
-		return &res
+		return result.Result{
+			Severity: severity,
+		}
 
+	// This might be a string if the rule was this:
+	//	`error = msg {
+	//	 	...
+	//		msg := "this is a failing thing"
+	//	}`
 	case string:
-		// This might be a string if the rule was this:
-		//	`error = msg {
-		//	 	...
-		//		msg := "this is a failing thing"
-		//	}`
-		//
-		res.Message = utils.JoinLines(res.Message, value)
-		return &res
-
-	case []interface{}:
-		// Extract messages from the value slice. The reason there is
-		// a slice is that there can be many matching cases for this
-		// rule and the query evaluates them all simultaneously. Each
-		// matching case might emit a message.
+		return result.Result{
+			Severity: severity,
+			Message:  value,
+		}
 
-		if len(value) == 0 {
-			return nil
+	// This might be a string if the rule was this:
+	//	`error = { "msg": msg} {
+	//	 	...
+	//		msg := "this is a failing thing"
+	//	}`
+	// or
+	//	`error = { "msg": msg, "result": "Error"} {
+	//	 	...
+	//		msg := "this is a failing thing"
+	//	}`
+	case map[string]interface{}:
+		res := result.Result{
+			Severity: severity,
 		}
 
-		for _, v := range value {
-			// First, see if the value is a slice of strings. We
-			// do this manually, because there's not enough type
-			// information for the `[]string` case to match below.
-			if s, ok := utils.AsStringSlice(v); ok {
-				res.Message = utils.JoinLines(res.Message,
-					utils.JoinLines(s...))
-				continue
+		if _, ok := value["msg"]; ok {
+			if m, ok := value["msg"].(string); ok {
+				res.Message = m
 			}
+		}
 
-			switch value := v.(type) {
-			case string:
-				res.Message = utils.JoinLines(res.Message, value)
-			case []string:
-				res.Message = utils.JoinLines(res.Message,
-					utils.JoinLines(value...))
-			case map[string]interface{}:
-				if _, ok := value["msg"]; ok {
-					if m, ok := value["msg"].(string); ok {
-						res.Message = utils.JoinLines(res.Message, m)
-					}
+		if _, ok := value["result"]; ok {
+			if r, ok := value["result"].(string); ok {
+				switch result.Severity(r) {
+				case result.SeverityError,
+					result.SeverityFatal,
+					result.SeveritySkip,
+					result.SeverityPass:
+					res.Severity = result.Severity(r)
 				}
-			default:
-				log.Printf("slice value of non-string %T: %v", value, value)
 			}
 		}
 
-		return &res
+		return res
 
-	default:
 		// We don't know how to deal with this kind of result, so just puke it out as YAML.
-		res.Message = utils.JoinLines(res.Message,
-			fmt.Sprintf("unhandled result value type '%T'", expr.Value),
-			string(must.Bytes(yaml.Marshal(expr.Value))),
-		)
-
-		return &res
+	default:
+		return result.Result{
+			Severity: severity,
+			Message: utils.JoinLines(
+				fmt.Sprintf("unhandled result value type '%T'", v),
+				string(must.Bytes(yaml.Marshal(v))),
+			),
+		}
 	}
 }