From b922fd9db43ae847e8a0fcce1c6038c693ebb549 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sat, 2 Aug 2025 13:24:17 -0700 Subject: [PATCH] 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. --- .../util/validation/field/error_matcher.go | 58 ++- .../validation/field/error_matcher_test.go | 361 +++++++++++++++++- .../ratcheting/default_behavior/doc_test.go | 22 +- .../ratcheting/maps/eachkey/doc_test.go | 13 +- 4 files changed, 414 insertions(+), 40 deletions(-) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go index afb5f1b07b3..c6638e10ca2 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher.go +++ b/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)) } } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher_test.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher_test.go index 4271cd63373..748b30f7843 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/field/error_matcher_test.go +++ b/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) } }) } diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go index 26c8aec0751..9d840c97d13 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/default_behavior/doc_test.go +++ b/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() diff --git a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/maps/eachkey/doc_test.go b/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/maps/eachkey/doc_test.go index 880adb81a97..b4a5b637fdc 100644 --- a/staging/src/k8s.io/code-generator/cmd/validation-gen/output_tests/ratcheting/maps/eachkey/doc_test.go +++ b/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() }