Ensure fixed order in multi-line error returned by change validator #1047
Conversation
…hange validator Signed-off-by: Artur Zych <[email protected]>
azych
force-pushed
the
ensure-order-in-changevalidator-combinederror
branch
from
d5d9267 to
4870ff1
Compare
There was a problem hiding this comment.
It looks good to me!
Do you mind letting it stay up for today? Just in case @everettraven has any thoughts around it? We can work on merging it away if he is not.
The changes seem reasonable, but I will loop him in because he had contributed this pieces 🙌🏼
hey @100mik, I definitely don't mind and thank you taking a look at it! |
| for _, tc := range []struct { | ||
| name string | ||
| changeValidator *crdupgradesafety.ChangeValidator | ||
| old v1.CustomResourceDefinition | ||
| new v1.CustomResourceDefinition | ||
| shouldError bool | ||
| expectedError error |
There was a problem hiding this comment.
In general, I recommend avoiding the use of specific error messages in testing because it makes the tests more fragile to subtle changes in the error message that is returned.
If you must check for an exact error message I would recommend adding a new test that explicitly evaluates the error messages rather than introducing fragility into an existing set of tests.
There was a problem hiding this comment.
I'd argue I'm just introducing a better (== more detailed) validation to this suite rather than fragility.
TestChangeValidator tests Validate() which returns a single object - error. I don't see why adding a more accurate validations on that single returnable would require a separation into a new suite and a duplication of test cases just for that.
PR changes aside, let's look at the current state of TestChangeValidator.
Since Validate() can internally run multiple different validations and there isn't a sensible way of introducing a mock here, what other way of verifying that actually all those different validations run (and failed) than to assert the contents of a multi-line error which is being returned?
With the current TestChangeValidator validation, the only thing that is being checked is if an error happens which could easily be a false positive in case we expect multiple validations to fail, but either those validations did not run at all or they did not fail as expected.
That means it's really easy to make a pretty major bug slip by with the tests still passing.
There was a problem hiding this comment.
Sure. I think it depends on the goal.
These tests were written to be minimalistic using mocks for the validations used in the Validate() method to ensure it's logic is generally working as expected, not to test the underlying validations (which should each have their own unit tests).
I don't think it is worth arguing which approach is better as I think it really comes down to personal preference and the desired end goal. I'll leave it to the maintainers to decide which approach they prefer.
There was a problem hiding this comment.
I think the main issue for me is that the logic isn't really being tested here very much, because of the potential false positives the test cases could be missing. If you're saying that it's covered elsewhere, that's great.
IMHO It's generally best practice to test public interfaces and have those tests close to the tested implementation - ChangeValidator.Validate() is such an implementation, as it does not necessarily has to be used via the validator.Validator middleman.
| @@ -449,7 +451,10 @@ func (cv *ChangeValidator) Validate(old, new v1.CustomResourceDefinition) error | |||
| continue | |||
| } | |||
|
|
|||
| for field, diff := range diffs { | |||
| // ensure order of the potentially multi-line final error | |||
| for _, field := range slices.Sorted(maps.Keys(diffs)) { | |||
There was a problem hiding this comment.
I defer any decision making to @100mik as I'm not sure what versioning semantics are used for kapp and it's associated libraries, but if breaking changes to the function signature are allowed it might be worth considering returning the slice of errors for callers to do with what they please.
I hadn't thought about the deterministic error message when I contributed this code, but adding additional time complexity for sorting feels unnecessary if we just provide the list of errors instead of joining them together on behalf of the caller.
Also a side note, the errors.Join() function would return an error that contains a method signature Unwrap() []error that will return the slice of errors included in it. Technically, a caller could get the slice of errors returned by this function today and do their own sorting but it isn't very user friendly.
All that being said, if @100mik is fine with this change as-is I don't have a strong reason against it. Just proposing a potential alternative :)
There was a problem hiding this comment.
both good points and I will be happy to adjust this PR according to what maintainers think best.
My idea here was to avoid introducing a breaking change and have a predictable / non-volatile error by default for the end user.
Update: just to help visualize the user effort require to parse this with Unwrap() - there are 3 levels of internal nesting here, going from the bottom of the call stack:
ChangeValidator.Validate()doeserrors.Join()and returns combined errorvalidator.Validate()doesfmt.Errorf()and wraps this returned error with additional contextvalidator.Validate()doeserrors.join()on the previous error (together with potential other errors from different validations) and returns the combined result to the user
|
hey @100mik just a friendly nudge to see if you had some time to take another look at this and if it can be moved forward potentially? Thanks! |
What this PR does / why we need it:
This PR introduces order to potentially multi-line error returned by ChangeValidator Validate(). Currently field validations within Validate() happen based on order returned by a map, which is not fixed/deterministic and means that the same set of internal errors can produce a different "unique" error each time, because Validate() combines all errors into a single one before it returns.
This can cause issues on the users side - eg. with errors returned by Validate ending up in objects .Status.Conditions which in turn could mean that a controller could be forced to try and reconcile status/object based on the same (yet different) error message. Please see a real-life example here: operator-framework/operator-controller#1456
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: