Repository: mbrt/gmailctl
Pull request #273 (2022-07-22 11:50:52)

Improve config tests output
When running `gmactl test`, do not exit after the first error, but accumulate all the errors.


Patch:
diff --git a/cmd/gmailctl/cmd/config.go b/cmd/gmailctl/cmd/config.go
index b98704b..b6f863b 100644
--- a/cmd/gmailctl/cmd/config.go
+++ b/cmd/gmailctl/cmd/config.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"path"
@@ -10,6 +9,7 @@ import (
 	"github.com/mbrt/gmailctl/internal/engine/cfgtest"
 	"github.com/mbrt/gmailctl/internal/engine/config"
 	"github.com/mbrt/gmailctl/internal/engine/config/v1alpha3"
+	"github.com/mbrt/gmailctl/internal/errors"
 )
 
 type parseResult struct {
@@ -49,17 +49,15 @@ func parseConfig(path, originalPath string, test bool) (parseResult, error) {
 		return res, err
 	}
 	if test && len(res.Config.Tests) > 0 {
-		ts, errs := cfgtest.NewFromParserRules(res.Res.Rules)
-		if len(errs) > 0 {
-			stderrPrintf("WARNING: %d filters are excluded from the tests:\n", len(errs))
-			for _, err := range errs {
-				stderrPrintf("  %v\n", err)
-			}
-			stderrPrintf("\n")
-		}
-		err = ts.ExecTests(res.Config.Tests)
+		ts, err := cfgtest.NewFromParserRules(res.Res.Rules)
 		if err != nil {
-			return res, fmt.Errorf("config tests failed: %w", err)
+			stderrPrintf("WARNING: %d filters are excluded from the tests:\n", len(errors.Errors(err)))
+			stderrPrintf("%+v\n", err)
+		}
+		tres := ts.ExecTests(res.Config.Tests)
+		if !tres.OK {
+			stderrPrintf("Test results: %s\n", tres)
+			return res, fmt.Errorf("%d/%d config tests failed", len(tres.Failed), tres.NumTests)
 		}
 	}
 
diff --git a/internal/errors/err.go b/internal/errors/err.go
index a879413..ec530a5 100644
--- a/internal/errors/err.go
+++ b/internal/errors/err.go
@@ -4,7 +4,8 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"strings"
+	"io"
+	"sync"
 )
 
 // Aliases to the standard errors package.
@@ -14,7 +15,12 @@ var (
 	As  = errors.As
 )
 
-// New is an alias for errors.New.
+// bufferPool is a pool of bytes.Buffers.
+var bufferPool = sync.Pool{
+	New: func() interface{} {
+		return &bytes.Buffer{}
+	},
+}
 
 // WithCause annotates a symptom error with a cause.
 //
@@ -33,21 +39,123 @@ func WithDetails(err error, details ...string) error {
 	return detailed{err, details}
 }
 
-func Details(err error) string {
-	var (
-		buffer bytes.Buffer
-		dErr   detailed
-	)
+func WriteDetails(w io.Writer, err error) {
+	var dErr detailed
 	for errors.As(err, &dErr) {
 		// Append all details of this error.
+		iw := indentWriter{w}
 		for _, d := range dErr.details {
-			buffer.WriteString("\n  - ")
-			buffer.WriteString(strings.ReplaceAll(d, "\n", "\n    "))
+			//nolint:errcheck
+			io.WriteString(iw, "\n- ")
+			//nolint:errcheck
+			io.WriteString(indentWriter{iw}, d)
 		}
 		// Continue down the chain.
 		err = dErr.error
 	}
-	return buffer.String()
+}
+
+func Details(err error) string {
+	buffer := bufferPool.Get().(*bytes.Buffer)
+	WriteDetails(buffer, err)
+	res := buffer.String()
+	bufferPool.Put(buffer)
+	return res
+}
+
+func Combine(errs ...error) error {
+	if len(errs) == 0 {
+		return nil
+	}
+
+	var (
+		res    multi
+		others []error
+	)
+	for _, err := range errs {
+		// Combine only the non-nil errors.
+		if err == nil {
+			continue
+		}
+		// Flatten by taking out the multi-error from the list
+		// if any is present.
+		merr, ok := err.(multi) //nolint:errorlint
+		if ok {
+			res = merr
+			continue
+		}
+		others = append(others, err)
+	}
+
+	// If we found a multierror, return that and append the rest.
+	if res != nil {
+		return append(res, others...)
+	}
+
+	// Check the others.
+	if len(others) == 0 {
+		return nil
+	}
+	if len(others) == 1 {
+		return others[0]
+	}
+
+	return multi(others)
+}
+
+func Errors(err error) []error {
+	if err == nil {
+		return nil
+	}
+	merr, ok := err.(multi) //nolint:errorlint
+	if !ok {
+		return []error{err}
+	}
+	return merr
+}
+
+type multi []error
+
+func (m multi) Error() string {
+	return fmt.Sprintf("multiple errors (%d); sample: %v", len(m), m[0])
+}
+
+func (m multi) Is(target error) bool {
+	for _, err := range m {
+		if errors.Is(err, target) {
+			return true
+		}
+	}
+	return false
+}
+
+func (m multi) As(target interface{}) bool {
+	for _, err := range m {
+		if errors.As(err, target) {
+			return true
+		}
+	}
+	return false
+}
+
+func (m multi) Format(f fmt.State, c rune) {
+	if (c == 'v' || c == 'w') && f.Flag('+') {
+		m.writeMultiline(f)
+	} else {
+		//nolint:errcheck
+		io.WriteString(f, m.Error())
+	}
+}
+
+func (m multi) writeMultiline(w io.Writer) {
+	fmt.Fprintf(w, "multiple errors (%d):", len(m))
+	wi := indentWriter{w}
+	for _, err := range m {
+		//nolint:errcheck
+		io.WriteString(w, "\n- ")
+		//nolint:errcheck
+		fmt.Fprintf(wi, "%+v", err)
+	}
 }
 
 type detailed struct {
@@ -55,13 +163,39 @@ type detailed struct {
 	details []string
 }
 
+func (d detailed) Format(f fmt.State, c rune) {
+	if (c == 'v' || c == 'w') && f.Flag('+') {
+		d.writeMultiline(f)
+	} else {
+		//nolint:errcheck
+		io.WriteString(f, d.Error())
+	}
+}
+
+func (d detailed) writeMultiline(w io.Writer) {
+	fmt.Fprintf(w, "%+v", d.error)
+	if len(d.details) == 0 {
+		return
+	}
+
+	//nolint:errcheck
+	io.WriteString(w, "\nNote:")
+	iw := indentWriter{w}
+	for _, d := range d.details {
+		//nolint:errcheck
+		io.WriteString(w, "\n- ")
+		//nolint:errcheck
+		io.WriteString(iw, d)
+	}
+}
+
 type annotated struct {
 	cause   error
 	symptom error
 }
 
 func (e annotated) Error() string {
-	return fmt.Sprintf("%s: %s", e.cause, e.symptom)
+	return fmt.Sprintf("%v: %v", e.cause, e.symptom)
 }
 
 func (e annotated) Unwrap() error {
@@ -78,3 +212,37 @@ func (e annotated) As(target interface{}) bool {
 	}
 	return errors.As(e.cause, target)
 }
+
+type indentWriter struct {
+	io.Writer
+}
+
+func (w indentWriter) Write(b []byte) (int, error) {
+	num := 0
+	write := func(b []byte) error {
+		n, err := w.Writer.Write(b)
+		num += n
+		return err
+	}
+
+	i, j := 0, 0
+	for i < len(b) {
+		if b[i] == '\n' {
+			// Write the indentation.
+			if err := write([]byte("\n  ")); err != nil {
+				return num, err
+			}
+			i++
+		}
+		// Look for the next newline.
+		for j = i; j < len(b) && b[j] != '\n'; j++ {
+		}
+		// Write everything up to the newline, or the end of buffer.
+		if err := write(b[i:j]); err != nil {
+			return num, err
+		}
+		i = j
+	}
+
+	return num, nil
+}


Test patch:
diff --git a/internal/engine/cfgtest/cfgtest.go b/internal/engine/cfgtest/cfgtest.go
index 2d8e251..1d8d397 100644
--- a/internal/engine/cfgtest/cfgtest.go
+++ b/internal/engine/cfgtest/cfgtest.go
@@ -1,8 +1,11 @@
 package cfgtest
 
 import (
+	"bytes"
 	"fmt"
+	"io"
 	"sort"
+	"strings"
 
 	"github.com/pmezard/go-difflib/difflib"
 
@@ -18,18 +21,20 @@ import (
 // This function is best effort. Every criteria that is not convertible is going
 // to be ignored and an error is returned in its place. The resulting rules will
 // contain only the valid rules.
-func NewFromParserRules(rs []parser.Rule) (Rules, []error) {
+func NewFromParserRules(rs []parser.Rule) (Rules, error) {
 	var res Rules
-	var errs []error
+	var errs error
 
 	for i, pr := range rs {
 		re, err := NewEvaluator(pr.Criteria)
 		if err != nil {
-			errs = append(errs, fmt.Errorf(
-				"cannot evaluate criteria #%d: %w", i, err))
-		} else {
-			res = append(res, Rule{re, Actions(pr.Actions)})
+			errs = errors.Combine(
+				errs,
+				fmt.Errorf("cannot evaluate criteria #%d: %w", i, err),
+			)
+			continue
 		}
+		res = append(res, Rule{re, Actions(pr.Actions)})
 	}
 
 	return res, errs
@@ -47,29 +52,42 @@ type Rules []Rule
 // ExecTests evaluates all the rules against the given tests.
 //
 // The evaluation stops at the first failing test.
-func (rs Rules) ExecTests(ts []v1alpha3.Test) error {
+func (rs Rules) ExecTests(ts []v1alpha3.Test) Result {
+	var failed []FailedTest
+
 	for i, t := range ts {
-		if err := rs.ExecTest(t); err != nil {
-			name := t.Name
-			if name == "" {
-				name = fmt.Sprintf("#%d", i)
-			}
-			return fmt.Errorf("test %q: %w", name, err)
+		if errs := rs.ExecTest(t); len(errs) > 0 {
+			failed = append(failed, FailedTest{
+				ID:     i,
+				Name:   t.Name,
+				Errors: errs,
+			})
 		}
 	}
-	return nil
+
+	return Result{
+		OK:       len(failed) == 0,
+		NumTests: len(ts),
+		Failed:   failed,
+	}
 }
 
 // ExecTest evaluates the rules on all the messages of the given test.
 //
 // If the rules apply as expected by the test, no error is returned.
-func (rs Rules) ExecTest(t v1alpha3.Test) error {
+func (rs Rules) ExecTest(t v1alpha3.Test) []error {
+	var res error
+
 	for i, msg := range t.Messages {
 		expected, err := rs.MatchingActions(msg)
 		if err != nil {
-			return errors.WithDetails(
-				fmt.Errorf("message #%d: error evaluating matching filters: %w", i, err),
-				messageDetails(msg))
+			res = errors.Combine(
+				res,
+				errors.WithDetails(
+					fmt.Errorf("message #%d: error evaluating matching filters: %w", i, err),
+					messageDetails(msg)),
+			)
+			continue
 		}
 		if expected.Equal(Actions(t.Actions)) {
 			// All good with this message.
@@ -89,13 +107,17 @@ func (rs Rules) ExecTest(t v1alpha3.Test) error {
 			// something.
 			diff = fmt.Sprintf("<cannot compute diff>: %v", err)
 		}
-		return errors.WithDetails(
-			fmt.Errorf("message #%d is going to get unexpected actions: %s", i,
-				reporting.Prettify(expected, true)),
-			messageDetails(msg),
-			fmt.Sprintf("Actions:\n%s", diff))
+		res = errors.Combine(
+			res,
+			errors.WithDetails(
+				fmt.Errorf("message #%d is going to get unexpected actions: %s", i,
+					reporting.Prettify(expected, true)),
+				messageDetails(msg),
+				fmt.Sprintf("Actions:\n%s", strings.TrimRight(diff, "\n"))),
+		)
 	}
-	return nil
+
+	return errors.Errors(res)
 }
 
 // MatchingActions returns the actions that would be applied by the rules if
@@ -120,6 +142,49 @@ func (rs Rules) MatchingActions(msg v1alpha3.Message) (Actions, error) {
 	return res, nil
 }
 
+// Result represents the result of a series of tests.
+type Result struct {
+	OK       bool
+	NumTests int
+	Failed   []FailedTest
+}
+
+func (r Result) String() string {
+	var buf bytes.Buffer
+
+	if r.OK {
+		fmt.Fprintf(&buf, "Success: %d/%d", r.NumTests, r.NumTests)
+		return buf.String()
+	}
+	fmt.Fprintf(&buf, "Failed: %d/%d\n", len(r.Failed), r.NumTests)
+	for _, t := range r.Failed {
+		t.dump(&buf)
+	}
+
+	return buf.String()
+}
+
+// FailedTest includes all the errors of a failed test.
+type FailedTest struct {
+	ID     int
+	Name   string
+	Errors []error
+}
+
+func (t FailedTest) String() string {
+	var buf bytes.Buffer
+	t.dump(&buf)
+	return buf.String()
+}
+
+func (t FailedTest) dump(w io.Writer) {
+	name := t.Name
+	if name == "" {
+		name = fmt.Sprintf("#%d", t.ID)
+	}
+	fmt.Fprintf(w, "\nFailed test %q:\n%+v\n", name, errors.Combine(t.Errors...))
+}
+
 // Actions represent the actions applied by a filter.
 type Actions parser.Actions
 
diff --git a/internal/engine/cfgtest/exec_test.go b/internal/engine/cfgtest/exec_test.go
index 9969e90..a784c7c 100644
--- a/internal/engine/cfgtest/exec_test.go
+++ b/internal/engine/cfgtest/exec_test.go
@@ -23,13 +23,82 @@ func readConfig(t *testing.T, path string) v1alpha3.Config {
 
 func TestExec(t *testing.T) {
 	tests := []struct {
-		path string
-		pass bool
+		path        string
+		numErrs     int
+		expectedOut string
 	}{
-		{path: "pass.jsonnet", pass: true},
-		{path: "invalid.jsonnet", pass: false},
-		{path: "fail.jsonnet", pass: false},
+		{path: "pass.jsonnet"},
+		{
+			path:    "invalid.jsonnet",
+			numErrs: 1,
+			expectedOut: `Failed: 1/1
+
+Failed test "both filters":
+message #0: error evaluating matching filters: conflicting filters detected: 'markImportant' is applied differently: got true and false
+Note:
+- Message: {
+    "to": [
+      "myalias@gmail.com"
+    ],
+    "lists": [
+      "list2"
+    ]
+  }
+`,
+		},
+		{
+			path:    "fail.jsonnet",
+			numErrs: 2,
+			expectedOut: `Failed: 2/2
+
+Failed test "wrong test":
+message #0: error evaluating matching filters: conflicting filters detected: 'markImportant' is applied differently: got true and false
+Note:
+- Message: {
+    "to": [
+      "myalias@gmail.com"
+    ],
+    "lists": [
+      "list2"
+    ]
+  }
+
+Failed test "another wrong test":
+multiple errors (2):
+- message #0 is going to get unexpected actions: {"markImportant":false}
+  Note:
+  - Message: {
+      "lists": [
+        "list1"
+      ]
+    }
+  - Actions:
+    --- want
+    +++ got
+    @@ -1,3 +1,3 @@
+     {
+    -  "markImportant": false
+    +  "markImportant": true
+     }
+- message #1 is going to get unexpected actions: {"markImportant":false}
+  Note:
+  - Message: {
+      "lists": [
+        "list2"
+      ]
+    }
+  - Actions:
+    --- want
+    +++ got
+    @@ -1,3 +1,3 @@
+     {
+    -  "markImportant": false
+    +  "markImportant": true
+     }
+`,
+		},
 	}
+
 	for _, tc := range tests {
 		t.Run(tc.path, func(t *testing.T) {
 			cfg := readConfig(t, tc.path)
@@ -37,11 +106,10 @@ func TestExec(t *testing.T) {
 			assert.Nil(t, err)
 			rules, errs := NewFromParserRules(pres.Rules)
 			assert.Empty(t, errs)
-			err = rules.ExecTests(cfg.Tests)
-			if tc.pass {
-				assert.Nil(t, err)
-			} else {
-				assert.NotNil(t, err)
+			res := rules.ExecTests(cfg.Tests)
+			assert.Equal(t, len(res.Failed), tc.numErrs)
+			if tc.expectedOut != "" {
+				assert.Equal(t, tc.expectedOut, res.String())
 			}
 		})
 	}
diff --git a/internal/engine/cfgtest/testdata/fail.jsonnet b/internal/engine/cfgtest/testdata/fail.jsonnet
index df61e84..aedbb5e 100644
--- a/internal/engine/cfgtest/testdata/fail.jsonnet
+++ b/internal/engine/cfgtest/testdata/fail.jsonnet
@@ -43,5 +43,19 @@ local filters = {
         archive: true,
       },
     },
+    {
+      name: 'another wrong test',
+      messages: [
+        {
+          lists: ['list1'],
+        },
+        {
+          lists: ['list2'],
+        },
+      ],
+      actions: {
+        markImportant: true,
+      },
+    },
   ],
 }
diff --git a/internal/errors/err_test.go b/internal/errors/err_test.go
index b5c1095..26ba0c0 100644
--- a/internal/errors/err_test.go
+++ b/internal/errors/err_test.go
@@ -59,3 +59,32 @@ func TestErrWithDetails(t *testing.T) {
     descr`
 	assert.Equal(t, details, Details(err4))
 }
+
+func TestCombine(t *testing.T) {
+	err1 := errors.New("err1")
+	err2 := errors.New("err2")
+
+	// Combine should ignore nils.
+	assert.NoError(t, Combine())
+	assert.NoError(t, Combine(nil))
+	assert.NoError(t, Combine(nil, nil))
+	assert.EqualError(t, Combine(err1, nil), err1.Error())
+
+	// Combine two.
+	assert.ElementsMatch(t, Errors(Combine(err1, err2)), []error{err1, err2})
+	assert.ElementsMatch(t, Errors(Combine(err1, nil, err2)), []error{err1, err2})
+
+	// Nesting.
+	err3 := Combine(err1, err2)
+	err4 := Combine(err1, err2, err3, nil)
+	assert.ElementsMatch(t, Errors(err4), []error{err1, err1, err2, err2})
+
+	// Print.
+	assert.EqualError(t, err4, "multiple errors (4); sample: err1")
+	verbose := `multiple errors (4):
+- err1
+- err2
+- err1
+- err2`
+	assert.Equal(t, fmt.Sprintf("%+v", err4), verbose)
+}