Make ErrorMatcher more strict about multi-match

As discussed in a proposed PR, multi-match makes sense when origin is
specified and matched, but otherwise it feels "loose" to multi-match.
And in fact it was hiding bugs in declarative validation tests and real
bugs (in subsequent commits) in implementation.

Now we allow multi-match if and only if:
  1) ErrorMatcher.ByOrigin() was specified
  2) The origin string is not empty
  3) The multiple "got" errors are not exactly identical

This last part is key -- if some change to DV codegens logic which
(errantly) calls the same function twice, we want to know about it.

staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go

@@ -32,7 +32,7 @@ type ErrorMatcher struct {
 	// "want" error has a nil field, don't match on field.
 	matchField bool
 	// TODO(thockin): consider whether value could be assumed - if the
-	// "want" error has a nil value, don't match on field.
+	// "want" error has a nil value, don't match on value.
 	matchValue               bool
 	matchOrigin              bool
 	matchDetail              func(want, got string) bool
@@ -138,6 +138,13 @@ func (m ErrorMatcher) ByValue() ErrorMatcher {
 }
 
 // ByOrigin returns a derived ErrorMatcher which also matches by the origin.
+// When this is used and an origin is set in the error, the matcher will
+// consider all expected errors with the same origin to be a match. The only
+// expception to this is when it finds two errors which are exactly identical,
+// which is too suspicious to ignore. This multi-matching allows tests to
+// express a single expectation ("I set the X field to an invalid value, and I
+// expect an error from origin Y") without having to know exactly how many
+// errors might be returned, or in what order, or with what wording.
 func (m ErrorMatcher) ByOrigin() ErrorMatcher {
 	m.matchOrigin = true
 	return m
@@ -184,40 +191,61 @@ func (m ErrorMatcher) ByDetailRegexp() ErrorMatcher {
 type TestIntf interface {
 	Helper()
 	Errorf(format string, args ...any)
-	Logf(format string, args ...any)
 }
 
 // Test compares two ErrorLists by the criteria configured in this matcher, and
-// fails the test if they don't match. If a given "want" error matches multiple
-// "got" errors, they will all be consumed. This might be OK (e.g. if there are
-// multiple errors on the same field from the same origin) or it might be an
-// insufficiently specific matcher, so these will be logged.
+// fails the test if they don't match. If matching by origin is enabled and the
+// error has a non-empty origin, a given "want" error can match multiple
+// "got" errors, and they will all be consumed. The only exception to this is
+// if the matcher got multiple identical (in every way, even those not being
+// matched on) errors, which is likely to indicate a bug.
 func (m ErrorMatcher) Test(tb TestIntf, want, got ErrorList) {
 	tb.Helper()
 
+	exactly := m.Exactly() // makes a copy
+
+	// If we ever find an EXACT duplicate error, it's almost certainly a bug
+	// worth reporting. If we ever find a use-case where this is not a bug, we
+	// can revisit this assumption.
+	seen := map[string]bool{}
+	for _, g := range got {
+		key := exactly.Render(g)
+		if seen[key] {
+			tb.Errorf("exact duplicate error:\n%s", key)
+		}
+		seen[key] = true
+	}
+
 	remaining := got
 	for _, w := range want {
 		tmp := make(ErrorList, 0, len(remaining))
-		n := 0
-		for _, g := range remaining {
+		matched := false
+		for i, g := range remaining {
 			if m.Matches(w, g) {
-				n++
+				matched = true
+				if m.matchOrigin && w.Origin != "" {
+					// When origin is included in the match, we allow multiple
+					// matches against the same wanted error, so that tests
+					// can be insulated from the exact number, order, and
+					// wording of cases that might return more than one error.
+					continue
+				} else {
+					// Single-match, save the rest of the "got" errors and move
+					// on to the next "want" error.
+					tmp = append(tmp, remaining[i+1:]...)
+					break
+				}
 			} else {
 				tmp = append(tmp, g)
 			}
 		}
-		if n == 0 {
+		if !matched {
 			tb.Errorf("expected an error matching:\n%s", m.Render(w))
-		} else if n > 1 {
-			// This is not necessarily and error, but it's worth logging in
-			// case it's not what the test author intended.
-			tb.Logf("multiple errors matched:\n%s", m.Render(w))
 		}
 		remaining = tmp
 	}
 	if len(remaining) > 0 {
 		for _, e := range remaining {
-			exactly := m.Exactly() // makes a copy
 			tb.Errorf("unmatched error:\n%s", exactly.Render(e))
 		}
 	}

staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher_test.go

@@ -17,11 +17,358 @@ limitations under the License.
 package field
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 )
 
-func TestErrorMatcherRender(t *testing.T) {
-	tests := []struct {
+func TestErrorMatcher_Matches(t *testing.T) {
+	baseErr := func() *Error {
+		return &Error{
+			Type:     ErrorTypeInvalid,
+			Field:    "field",
+			BadValue: "value",
+			Detail:   "detail",
+			Origin:   "origin",
+		}
+	}
+
+	testCases := []struct {
+		name      string
+		matcher   ErrorMatcher
+		wantedErr func() *Error
+		actualErr *Error
+		matches   bool
+	}{{
+		name:      "ByType: match",
+		matcher:   ErrorMatcher{}.ByType(),
+		wantedErr: baseErr,
+		actualErr: &Error{Type: ErrorTypeInvalid},
+		matches:   true,
+	}, {
+		name:      "ByType: no match",
+		matcher:   ErrorMatcher{}.ByType(),
+		wantedErr: baseErr,
+		actualErr: &Error{Type: ErrorTypeRequired},
+		matches:   false,
+	}, {
+		name:      "ByField: match",
+		matcher:   ErrorMatcher{}.ByField(),
+		wantedErr: baseErr,
+		actualErr: &Error{Field: "field"},
+		matches:   true,
+	}, {
+		name:      "ByField: no match",
+		matcher:   ErrorMatcher{}.ByField(),
+		wantedErr: baseErr,
+		actualErr: &Error{Field: "other"},
+		matches:   false,
+	}, {
+		name:      "ByValue: match",
+		matcher:   ErrorMatcher{}.ByValue(),
+		wantedErr: baseErr,
+		actualErr: &Error{BadValue: "value"},
+		matches:   true,
+	}, {
+		name:      "ByValue: no match",
+		matcher:   ErrorMatcher{}.ByValue(),
+		wantedErr: baseErr,
+		actualErr: &Error{BadValue: "other"},
+		matches:   false,
+	}, {
+		name:      "ByOrigin: match",
+		matcher:   ErrorMatcher{}.ByOrigin(),
+		wantedErr: baseErr,
+		actualErr: &Error{Origin: "origin"},
+		matches:   true,
+	}, {
+		name:      "ByOrigin: no match",
+		matcher:   ErrorMatcher{}.ByOrigin(),
+		wantedErr: baseErr,
+		actualErr: &Error{Origin: "other"},
+		matches:   false,
+	}, {
+		name:      "ByDetailExact: match",
+		matcher:   ErrorMatcher{}.ByDetailExact(),
+		wantedErr: baseErr,
+		actualErr: &Error{Detail: "detail"},
+		matches:   true,
+	}, {
+		name:      "ByDetailExact: no match",
+		matcher:   ErrorMatcher{}.ByDetailExact(),
+		wantedErr: baseErr,
+		actualErr: &Error{Detail: "other"},
+		matches:   false,
+	}, {
+		name:    "ByDetailSubstring: match empty",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = ""
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailSubstring: match full",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "is the"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailSubstring: match start",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "this is"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailSubstring: match middle",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "is the"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailSubstring: match end",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "the detail"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailSubstring: no match",
+		matcher: ErrorMatcher{}.ByDetailSubstring(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "is not the"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   false,
+	}, {
+		name:    "ByDetailRegexp: match empty",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = ".*"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: match full",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "^this is the detail$"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: match start",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "^this is"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: match middle",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "is the"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: match end",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "the detail$"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: match parts",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "^this .* .* detail$"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   true,
+	}, {
+		name:    "ByDetailRegexp: no match",
+		matcher: ErrorMatcher{}.ByDetailRegexp(),
+		wantedErr: func() *Error {
+			e := baseErr()
+			e.Detail = "is not the"
+			return e
+		},
+		actualErr: &Error{Detail: "this is the detail"},
+		matches:   false,
+	}, {
+		name:      "Exactly: match",
+		matcher:   ErrorMatcher{}.Exactly(),
+		wantedErr: baseErr,
+		actualErr: baseErr(),
+		matches:   true,
+	}, {
+		name:      "Exactly: no match (type)",
+		matcher:   ErrorMatcher{}.Exactly(),
+		wantedErr: baseErr,
+		actualErr: &Error{Type: ErrorTypeRequired, Field: "field", BadValue: "value", Detail: "detail", Origin: "origin"},
+		matches:   false,
+	}, {
+		name:      "RequireOriginWhenInvalid: match",
+		matcher:   ErrorMatcher{}.ByOrigin().RequireOriginWhenInvalid(),
+		wantedErr: baseErr,
+		actualErr: &Error{Type: ErrorTypeInvalid, Origin: "origin"},
+		matches:   true,
+	}, {
+		name:      "RequireOriginWhenInvalid: no match (missing origin)",
+		matcher:   ErrorMatcher{}.ByOrigin().RequireOriginWhenInvalid(),
+		wantedErr: baseErr,
+		actualErr: &Error{Type: ErrorTypeInvalid},
+		matches:   false,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.matcher.Matches(tc.wantedErr(), tc.actualErr) != tc.matches {
+				t.Errorf("Matches() = %v, want %v", !tc.matches, tc.matches)
+			}
+		})
+	}
+}
+
+// fakeTestIntf is used to test the testing support.
+type fakeTestIntf struct {
+	errs []string
+}
+
+var _ TestIntf = &fakeTestIntf{}
+
+func (*fakeTestIntf) Helper() {}
+
+func (ft *fakeTestIntf) Errorf(format string, args ...any) {
+	ft.errs = append(ft.errs, fmt.Sprintf(format, args...))
+}
+
+func TestErrorMatcher_Test(t *testing.T) {
+	testCases := []struct {
+		name           string
+		matcher        ErrorMatcher
+		want           ErrorList
+		got            ErrorList
+		expectedErrors []string
+		expectedLogs   []string
+	}{{
+		name:    "no origin: perfect match",
+		matcher: ErrorMatcher{}.ByField(),
+		want:    ErrorList{Invalid(NewPath("f"), nil, "")},
+		got:     ErrorList{Invalid(NewPath("f"), "v", "d")},
+	}, {
+		name:           "no origin: got too few errors",
+		matcher:        ErrorMatcher{}.ByField(),
+		want:           ErrorList{Invalid(NewPath("f"), nil, "")},
+		got:            ErrorList{},
+		expectedErrors: []string{"expected an error matching:"},
+	}, {
+		name:           "no origin: got too many errors",
+		matcher:        ErrorMatcher{}.ByField(),
+		want:           ErrorList{},
+		got:            ErrorList{Invalid(NewPath("f"), "v", "d")},
+		expectedErrors: []string{"unmatched error:"},
+	}, {
+		name:           "no origin: got wrong errors",
+		matcher:        ErrorMatcher{}.ByField(),
+		want:           ErrorList{Invalid(NewPath("f1"), nil, "")},
+		got:            ErrorList{Invalid(NewPath("f2"), "v", "d")},
+		expectedErrors: []string{"expected an error matching:", "unmatched error:"},
+	}, {
+		name:    "with origin: single match",
+		matcher: ErrorMatcher{}.ByField().ByOrigin(),
+		want:    ErrorList{Invalid(NewPath("f"), nil, "").WithOrigin("o")},
+		got:     ErrorList{Invalid(NewPath("f"), "v", "d").WithOrigin("o")},
+	}, {
+		name:    "with origin: multiple matches, different details",
+		matcher: ErrorMatcher{}.ByField().ByOrigin(),
+		want: ErrorList{
+			Invalid(NewPath("f1"), nil, "").WithOrigin("o"),
+			Invalid(NewPath("f2"), nil, "").WithOrigin("o"),
+		},
+		got: ErrorList{
+			Invalid(NewPath("f1"), "v", "d1").WithOrigin("o"),
+			Invalid(NewPath("f2"), "v", "d1").WithOrigin("o"),
+			Invalid(NewPath("f1"), "v", "d2").WithOrigin("o"),
+			Invalid(NewPath("f2"), "v", "d2").WithOrigin("o"),
+		},
+	}, {
+		name:    "with origin: multiple matches, same exact error",
+		matcher: ErrorMatcher{}.ByField().ByOrigin(),
+		want: ErrorList{
+			Invalid(NewPath("f1"), nil, "").WithOrigin("o"),
+			Invalid(NewPath("f2"), nil, "").WithOrigin("o"),
+		},
+		got: ErrorList{
+			Invalid(NewPath("f1"), "v", "d").WithOrigin("o"),
+			Invalid(NewPath("f1"), "v", "d").WithOrigin("o"),
+			Invalid(NewPath("f2"), "v", "d").WithOrigin("o"),
+			Invalid(NewPath("f2"), "v", "d").WithOrigin("o"),
+		},
+		expectedErrors: []string{"exact duplicate error:", "exact duplicate error:"},
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeT := &fakeTestIntf{}
+			tc.matcher.Test(fakeT, tc.want, tc.got)
+			if want, got := len(tc.expectedErrors), len(fakeT.errs); got != want {
+				if got == 0 {
+					t.Errorf("expected %d errors, got %d", want, got)
+				} else {
+					q := make([]string, len(fakeT.errs))
+					for i, err := range fakeT.errs {
+						q[i] = fmt.Sprintf("%q", err)
+					}
+					t.Errorf("expected %d errors, got %d:\n%s", want, got, strings.Join(q, "\n"))
+				}
+			} else {
+				for i := range tc.expectedErrors {
+					if !strings.HasPrefix(fakeT.errs[i], tc.expectedErrors[i]) {
+						t.Errorf("error %d: expected prefix %q, got %q", i, tc.expectedErrors[i], fakeT.errs[i])
+					}
+				}
+			}
+		})
+	}
+}
+
+func TestErrorMatcher_Render(t *testing.T) {
+	testCases := []struct {
 		name     string
 		matcher  ErrorMatcher
 		err      *Error
@@ -77,11 +424,11 @@ func TestErrorMatcherRender(t *testing.T) {
 		},
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := tt.matcher.Render(tt.err)
-			if result != tt.expected {
-				t.Errorf("Render() = %v, want %v", result, tt.expected)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := tc.matcher.Render(tc.err)
+			if result != tc.expected {
+				t.Errorf("Render() = %v, want %v", result, tc.expected)
 			}
 		})
 	}

staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go

@@ -48,10 +48,10 @@ func Test_StructSlice(t *testing.T) {
 	}
 
 	st := localSchemeBuilder.Test(t)
-	st.Value(mkTest()).ExpectMatches(field.ErrorMatcher{}.ByType().ByField(), field.ErrorList{
-		field.Invalid(field.NewPath("sliceField"), "", ""),
-		field.Invalid(field.NewPath("sliceField[0]"), "", ""),
-		field.Invalid(field.NewPath("typedefSliceField"), "", ""),
+	st.Value(mkTest()).ExpectValidateFalseByPath(map[string][]string{
+		"sliceField":        {"field sliceField"},
+		"sliceField[0]":     {"type S"},
+		"typedefSliceField": {"field typedefSliceField", "type MySlice"},
 	})
 
 	st.Value(mkTest()).OldValue(mkTest()).ExpectValid()
@@ -68,13 +68,13 @@ func Test_StructMap(t *testing.T) {
 	}
 
 	st := localSchemeBuilder.Test(t)
-	st.Value(mkTest()).ExpectMatches(field.ErrorMatcher{}.ByType().ByField(), field.ErrorList{
-		field.Invalid(field.NewPath("mapKeyField"), "", ""),
-		field.Invalid(field.NewPath("mapValueField"), "", ""),
-		field.Invalid(field.NewPath("mapValueField[k]"), "", ""),
-		field.Invalid(field.NewPath("aliasMapKeyTypeField"), "", ""),
-		field.Invalid(field.NewPath("aliasMapValueTypeField"), "", ""),
-		field.Invalid(field.NewPath("aliasMapValueTypeField[k]"), "", ""),
+	st.Value(mkTest()).ExpectValidateFalseByPath(map[string][]string{
+		"aliasMapKeyTypeField":      {"field aliasMapKeyTypeField", "type MapKeyType", "type S"},
+		"aliasMapValueTypeField":    {"field aliasMapValueTypeField", "type MapValueType"},
+		"aliasMapValueTypeField[k]": {"type S"},
+		"mapKeyField":               {"field mapKeyField", "type S"},
+		"mapValueField":             {"field mapValueField"},
+		"mapValueField[k]":          {"type S"},
 	})
 
 	st.Value(mkTest()).OldValue(mkTest()).ExpectValid()

staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/maps/eachkey/doc_test.go

@@ -22,8 +22,6 @@ package eachkey
 
 import (
 	"testing"
-
-	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 func Test_Struct(t *testing.T) {
@@ -42,11 +40,12 @@ func Test_Struct(t *testing.T) {
 		}
 	}
 	st := localSchemeBuilder.Test(t)
-	st.Value(mkTest()).ExpectMatches(field.ErrorMatcher{}.ByType().ByField(), field.ErrorList{
-		field.Invalid(field.NewPath("mapField"), "", ""),
-		field.Invalid(field.NewPath("mapTypedefField"), "", ""),
-		field.Invalid(field.NewPath("mapValidatedTypedefField"), "", ""),
-		field.Invalid(field.NewPath("validatedMapTypeField"), "", ""),
+	st.Value(mkTest()).ExpectValidateFalseByPath(map[string][]string{
+		"mapField":                 {"field Struct.MapField(keys)"},
+		"mapTypedefField":          {"field Struct.MapTypedefField(keys)"},
+		"mapValidatedTypedefField": {"ValidatedStringType", "field Struct.MapValidatedTypedefField(keys)"},
+		"validatedMapTypeField":    {"field Struct.ValidatedMapTypeField(keys)", "type ValidatedMapType(keys)"},
 	})
+
 	st.Value(mkTest()).OldValue(mkTest()).ExpectValid()
 }