Skip to content

Conversation

@dcantah
Copy link
Member

@dcantah dcantah commented Oct 3, 2023

Pass the passed in context into some nested function calls, and wrap errors instead of %+v.

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]>
@dcantah dcantah force-pushed the cri-snapshotter-platform branch from dbd8547 to e3cb747 Compare October 3, 2023 09:10
assert.Equal(t, tt.expectSnapshotter, snapshotter)
assert.Equal(t, tt.expectErr, err)
if tt.expectErr {
assert.Error(t, err)
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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 :)

Copy link
Member Author

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

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants