-
Notifications
You must be signed in to change notification settings - Fork 3.7k
CRI: Snapshotter per runtime handler adjustments #9183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Pass the passed in context into some nested function calls, wrap errors instead of %+v, and change some tests to strictly just test for an error and not an exact error. Signed-off-by: Danny Canter <[email protected]>
dbd8547 to
e3cb747
Compare
| assert.Equal(t, tt.expectSnapshotter, snapshotter) | ||
| assert.Equal(t, tt.expectErr, err) | ||
| if tt.expectErr { | ||
| assert.Error(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder will this be a relaxation for this test? because the original test was specifically validating the returned error (or error msg). but this change simply checks the existence of an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a sense, but the truth is we really care about IF there was an error. We weren't returning a constant errors.New style error so this was already kind of awkward. I don't think we're losing much test coverage with this change, but I could be convinced otherwise. expectErr being a bool also makes this match a bunch of other times we use this pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. My only concern is that the errors returned by this block could also pass this test. Or am I worrying to much here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that's completely fair, quite a bit of our tests fall into the same category though 😅. We could change back to checking the error if you want. It's much easier if it's just a global errors.New style we need to check against though (or if we just care what type the error is, but I don't think this warrants a new error type), I'm not a fan of reconstructing some fmt.Errorf
fuweid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…h upstream containerd/main update fork-external/main branch to upstream containerd/main at commit f90f80d Related work items: containerd#8736, containerd#8861, containerd#8924, containerd#8934, containerd#9027, containerd#9076, containerd#9104, containerd#9118, containerd#9141, containerd#9155, containerd#9177, containerd#9183, containerd#9184, containerd#9186, containerd#9187, containerd#9191, containerd#9200, containerd#9205, containerd#9211, containerd#9214, containerd#9215, containerd#9221, containerd#9223, containerd#9228, containerd#9231, containerd#9234, containerd#9242, containerd#9246, containerd#9247, containerd#9251, containerd#9253, containerd#9254, containerd#9255, containerd#9268
Pass the passed in context into some nested function calls, and wrap errors instead of %+v.