WIP: Add an error matcher, convert 1 test #130388
Conversation
This change introducing a new field in Error. It would be used in testing to compare the expected errors without matching the detail strings. Co-authored-by: Tim Hockin <thockin@google.com>
Replace manual error logging with cmp.Diff for more precise error comparisons, using cmpopts to ignore Origin field and support UniqueString comparison.
Update ValidateEndpointsCreate validation tests to use the new Origin field for more precise error comparisons. It leverage the Origin field instead of detailed error messages, improving test robustness and readability. Co-authored-by: Tim Hockin <thockin@google.com>
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-triage
k8s-ci-robot
added
kind/api-change
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
I picked TestValidateTopologySpreadConstraints because it was the last
failing test on my screen when I changed on of the commonly hard-coded
error strings. I fixed exactly those validation errors that were needed
to make this test pass. Some of the Origin values can be debated.
The `field/testing.Matcher` interface allows tests to configure the
criteria by which they want to match expected and actual errors. The
hope is that everyone will use Origin for Invalid errors.
TODO: I do not love that we have `Error.SetOrigin()` (useful when you
have a single error and `ErrorList.SetError()` (useful when you want to
set origin for a whole list) and `ErrorList.AppendWithOrigin()` (useful
when you want to set origin for a whole list).
In particular, the last two feel redundant, but you can't call a pointer
method on a temporary value:
```
// Works:
allErrs.AppendWithOrigin("foobar", validateSomething(fldPath, &obj.Something)...)
// Fails:
allErrs = append(allErrs, validateSomething(fldPath, &obj.Something).SetOrigin("foobar")...)
```
Having an Append() method feels un-idiomatic. Will explore in next
commit.
Instead of having SetOrigin() and AppendWithOrigin() and Append(), just have ErrorList.WithOrigin, which returns a new slice (but since it is a slice of pointers and does not change len(), it's effectively in-place mutation. TODO: we always use `Error` as a pointer, so I left `.SetOrigin()` as a pointer method, but I think it would be cleaner as `.WithOrigin()` if we want to convert all users of Error back to a non-pointer (different effort!)
thockin
force-pushed
the
error_matcher_and_origin
branch
from
f9b997e to
81f9b02
Compare
|
@thockin: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This builds on #130355.
The goal of the matcher is to make writing tests easier and more consistent, and less brittle in the face of validation strings changing. With Origin, it gets even easier.
I picked TestValidateTopologySpreadConstraints because it was the last failing test on my screen when I changed on of the commonly hard-coded error strings. I fixed exactly those validation errors that were needed to make this test pass. Some of the Origin values can be debated.
The
field/testing.Matcherinterface allows tests to configure the criteria by which they want to match expected and actual errors. The hope is that everyone will use Origin for Invalid errors./kind cleanup