Replace deprecated usages of assert.ErrorType
#5312
Conversation
Signed-off-by: Laura Brehm <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5312 +/- ##
==========================================
- Coverage 61.45% 61.01% -0.45%
==========================================
Files 299 296 -3
Lines 20855 20847 -8
==========================================
- Hits 12816 12719 -97
- Misses 7124 7211 +87
- Partials 915 917 +2 |
| var expectedError *pluginError | ||
| assert.Check(t, errors.As(p.Err, &expectedError)) |
There was a problem hiding this comment.
Oh! I recall some fun cases where there's no good replacement, or at least errors.As / errors.Is for sure has a bunch of pitfalls; see the discussion I had with the gotest.tools authors on gotestyourself/gotest.tools#272
There was a problem hiding this comment.
Indeed. This works, but the errors.As "pointer-to-pointer" stuff is.. not nice to use.
There was a problem hiding this comment.
It's really horrible, and very easy to get wrong (in which case, it's even possible to get the unexpected result). I honestly really hate the errors.As / errors.Is handling for that reason.
| @@ -18,7 +17,7 @@ func TestRemove(t *testing.T) { | |||
| _, err := cli.ContextStore().GetMetadata("current") | |||
| assert.NilError(t, err) | |||
| _, err = cli.ContextStore().GetMetadata("other") | |||
| assert.Check(t, is.ErrorType(err, errdefs.IsNotFound)) | |||
| assert.Check(t, errdefs.IsNotFound(err)) | |||
There was a problem hiding this comment.
To keep the current (more informative) output of is.ErrorType, we should probably use the 3rd argument of assert.Check to write a custom error message (wanted %T, actual %T or something along those lines), but it will definitely make the code a lot more lengthy/verbose 😞
There was a problem hiding this comment.
Ahh, yeah I just saw you mention the nicer output (I definitely wondered why we were using Errortype(err, errdefs.IsXxx)) in the thread with the gotest.tools folks.
There was a problem hiding this comment.
If we want to continue using the is.ErrorType, I think Daniel was open to un-deprecating it (IIUC, reason for deprecating was mostly cleaning up ("stdlib now has utilities for it, and the alternative can work, so let's deprecate"). We can open a PR to do so, and see if they're ok with it.
|
FWIW, I opened a PR to see if un-deprecating is acceptable; |
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)