Merge pull request #130543 from thockin/error_matcher_and_origin

Fix up ErrorMatcher from feedback

pkg/apis/core/validation/validation_test.go

@@ -20894,8 +20894,8 @@ func TestValidateEndpointsCreate(t *testing.T) {
 		t.Run(k, func(t *testing.T) {
 			errs := ValidateEndpointsCreate(&v.endpoints)
 			// TODO: set .RequireOriginWhenInvalid() once metadata is done
-			matcher := fldtest.Match().ByType().ByField().ByOrigin()
+			matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin()
-			fldtest.MatchErrors(t, v.expectedErrs, errs, matcher)
+			matcher.Test(t, v.expectedErrs, errs)
 		})
 	}
 }
@@ -22891,8 +22891,8 @@ func TestValidateTopologySpreadConstraints(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			errs := validateTopologySpreadConstraints(tc.constraints, fieldPath, tc.opts)
-			matcher := fldtest.Match().ByType().ByField().ByOrigin().RequireOriginWhenInvalid()
+			matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid()
-			fldtest.MatchErrors(t, tc.wantFieldErrors, errs, matcher)
+			matcher.Test(t, tc.wantFieldErrors, errs)
 		})
 	}
 }

staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go

@@ -1089,7 +1089,8 @@ func TestRegisterValidate(t *testing.T) {
 			} else {
 				results = s.ValidateUpdate(ctx, tc.options, tc.object, tc.oldObject, tc.subresource...)
 			}
-			fieldtesting.MatchErrors(t, tc.expected, results, fieldtesting.Match().ByType().ByField().ByOrigin())
+			matcher := fieldtesting.ErrorMatcher{}.ByType().ByField().ByOrigin()
+			matcher.Test(t, tc.expected, results)
 		})
 	}
 }

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

@@ -26,50 +26,16 @@ import (
 	field "k8s.io/apimachinery/pkg/util/validation/field"
 )
 
-// MatchErrors compares two ErrorLists with the specified matcher, and fails
+// ErrorMatcher is a helper for comparing field.Error objects.
-// the test if they don't match. If a given "want" error matches multiple "got"
+type ErrorMatcher struct {
-// errors, they will all be consumed. This might be OK (e.g. if there are
+	// TODO(thockin): consider whether type is ever NOT required, maybe just
-// multiple errors on the same field from the same origin) or it might be an
+	// assume it.
-// insufficiently specific matcher, so these will be logged.
+	matchType bool
-func MatchErrors(t *testing.T, want, got field.ErrorList, matcher *Matcher) {
+	// TODO(thockin): consider whether field could be assumed - if the
-	t.Helper()
+	// "want" error has a nil field, don't match on field.
-
+	matchField bool
-	remaining := got
+	// TODO(thockin): consider whether value could be assumed - if the
-	for _, w := range want {
+	// "want" error has a nil value, don't match on field.
-		tmp := make(field.ErrorList, 0, len(remaining))
-		n := 0
-		for _, g := range remaining {
-			if matcher.Matches(w, g) {
-				n++
-			} else {
-				tmp = append(tmp, g)
-			}
-		}
-		if n == 0 {
-			t.Errorf("expected an error matching:\n%s", matcher.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.
-			t.Logf("multiple errors matched:\n%s", matcher.Render(w))
-		}
-		remaining = tmp
-	}
-	if len(remaining) > 0 {
-		for _, e := range remaining {
-			t.Errorf("unmatched error:\n%s", Match().Exactly().Render(e))
-		}
-	}
-}
-
-// Match returns a new Matcher.
-func Match() *Matcher {
-	return &Matcher{}
-}
-
-// Matcher is a helper for comparing field.Error objects.
-type Matcher struct {
-	matchType                bool
-	matchField               bool
 	matchValue               bool
 	matchOrigin              bool
 	matchDetail              func(want, got string) bool
@@ -78,7 +44,7 @@ type Matcher struct {
 
 // Matches returns true if the two field.Error objects match according to the
 // configured criteria.
-func (m *Matcher) Matches(want, got *field.Error) bool {
+func (m ErrorMatcher) Matches(want, got *field.Error) bool {
 	if m.matchType && want.Type != got.Type {
 		return false
 	}
@@ -105,8 +71,8 @@ func (m *Matcher) Matches(want, got *field.Error) bool {
 }
 
 // Render returns a string representation of the specified Error object,
-// according to the criteria configured in the Matcher.
+// according to the criteria configured in the ErrorMatcher.
-func (m *Matcher) Render(e *field.Error) string {
+func (m ErrorMatcher) Render(e *field.Error) string {
 	buf := strings.Builder{}
 
 	comma := func() {
@@ -138,66 +104,104 @@ func (m *Matcher) Render(e *field.Error) string {
 	return "{" + buf.String() + "}"
 }
 
-// Exactly configures the matcher to match all fields exactly.
+// Exactly returns a derived ErrorMatcher which matches all fields exactly.
-func (m *Matcher) Exactly() *Matcher {
+func (m ErrorMatcher) Exactly() ErrorMatcher {
 	return m.ByType().ByField().ByValue().ByOrigin().ByDetailExact()
 }
 
-// Exactly configures the matcher to match errors by type.
+// ByType returns a derived ErrorMatcher which also matches by type.
-func (m *Matcher) ByType() *Matcher {
+func (m ErrorMatcher) ByType() ErrorMatcher {
 	m.matchType = true
 	return m
 }
 
-// ByField configures the matcher to match errors by field path.
+// ByField returns a derived ErrorMatcher which also matches by field path.
-func (m *Matcher) ByField() *Matcher {
+func (m ErrorMatcher) ByField() ErrorMatcher {
 	m.matchField = true
 	return m
 }
 
-// ByValue configures the matcher to match errors by the errant value.
+// ByValue returns a derived ErrorMatcher which also matches by the errant
-func (m *Matcher) ByValue() *Matcher {
+// value.
+func (m ErrorMatcher) ByValue() ErrorMatcher {
 	m.matchValue = true
 	return m
 }
 
-// ByOrigin configures the matcher to match errors by the origin.
+// ByOrigin returns a derived ErrorMatcher which also matches by the origin.
-func (m *Matcher) ByOrigin() *Matcher {
+func (m ErrorMatcher) ByOrigin() ErrorMatcher {
 	m.matchOrigin = true
 	return m
 }
 
-// RequireOriginWhenInvalid configures the matcher to require the Origin field
+// RequireOriginWhenInvalid returns a derived ErrorMatcher which also requires
-// to be set when the Type is Invalid and the matcher is matching by Origin.
+// the Origin field to be set when the Type is Invalid and the matcher is
-func (m *Matcher) RequireOriginWhenInvalid() *Matcher {
+// matching by Origin.
+func (m ErrorMatcher) RequireOriginWhenInvalid() ErrorMatcher {
 	m.requireOriginWhenInvalid = true
 	return m
 }
 
-// ByDetailExact configures the matcher to match errors by the exact detail
+// ByDetailExact returns a derived ErrorMatcher which also matches errors by
-// string.
+// the exact detail string.
-func (m *Matcher) ByDetailExact() *Matcher {
+func (m ErrorMatcher) ByDetailExact() ErrorMatcher {
 	m.matchDetail = func(want, got string) bool {
 		return got == want
 	}
 	return m
 }
 
-// ByDetailSubstring configures the matcher to match errors by a substring of
+// ByDetailSubstring returns a derived ErrorMatcher which also matches errors
-// the detail string.
+// by a substring of the detail string.
-func (m *Matcher) ByDetailSubstring() *Matcher {
+func (m ErrorMatcher) ByDetailSubstring() ErrorMatcher {
 	m.matchDetail = func(want, got string) bool {
 		return strings.Contains(got, want)
 	}
 	return m
 }
 
-// ByDetailRegexp configures the matcher to match errors by a regular
+// ByDetailRegexp returns a derived ErrorMatcher which also matches errors by a
-// expression of the detail string, where the "want" string is assumed to be a
+// regular expression of the detail string, where the "want" string is assumed
-// valid regular expression.
+// to be a valid regular expression.
-func (m *Matcher) ByDetailRegexp() *Matcher {
+func (m ErrorMatcher) ByDetailRegexp() ErrorMatcher {
 	m.matchDetail = func(want, got string) bool {
 		return regexp.MustCompile(want).MatchString(got)
 	}
 	return m
 }
+
+// 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.
+func (m ErrorMatcher) Test(tb testing.TB, want, got field.ErrorList) {
+	tb.Helper()
+
+	remaining := got
+	for _, w := range want {
+		tmp := make(field.ErrorList, 0, len(remaining))
+		n := 0
+		for _, g := range remaining {
+			if m.Matches(w, g) {
+				n++
+			} else {
+				tmp = append(tmp, g)
+			}
+		}
+		if n == 0 {
+			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/apiserver/pkg/registry/rest/validate_test.go

@@ -154,7 +154,8 @@ func TestValidateDeclaratively(t *testing.T) {
 			} else {
 				results = ValidateUpdateDeclaratively(ctx, tc.options, scheme, tc.object, tc.oldObject)
 			}
-			fieldtesting.MatchErrors(t, tc.expected, results, fieldtesting.Match().ByType().ByField().ByOrigin())
+			matcher := fieldtesting.ErrorMatcher{}.ByType().ByField().ByOrigin()
+			matcher.Test(t, tc.expected, results)
 		})
 	}
 }