*: fix improperly wrapped errors#72353
Conversation
|
This change is |
cd547cd to
664f622
Compare
dcb0ad6 to
634fd19
Compare
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rafiss)
pkg/testutils/sqlutils/sql_runner.go, line 120 at r1 (raw file):
_, err := sr.DB.ExecContext(context.Background(), query, args...) if !testutils.IsError(err, errRE) { return errors.AssertionFailedf("expected error '%s', got: %v", errRE, err)
[nit] I don't think this should be an AssertionFailed (we don't really care to see this stack). I would just extract errStr := err.Error() and use that with Newf().
stevendanna
left a comment
There was a problem hiding this comment.
Left two questions, but overall it looks good.
The fmt.Errorf -> errors.Wrap transformation isn't necessarily equivalent, but I checked all the ones here and they seem OK. I flagged one we might want to double-check.
| return err | ||
| default: | ||
| return fmt.Errorf("unexpected context error: %v", err) | ||
| return errors.Wrap(err, "unexpected context error") |
There was a problem hiding this comment.
It looks like before this would always return an error whereas this might now return nil. However, I'm not sure it matters because I believe Err() only ever returns the two errors already caught in the switch above.
pkg/testutils/sqlutils/sql_runner.go
Outdated
| _, err := sr.DB.ExecContext(context.Background(), query, args...) | ||
| if !testutils.IsError(err, errRE) { | ||
| return errors.Newf("expected error '%s', got: %v", errRE, err) | ||
| return errors.AssertionFailedf("expected error '%s', got: %v", errRE, err) |
There was a problem hiding this comment.
For my own understanding, why are we preferring AssertionFailedf in these cases?
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/roachprod-stress/main.go, line 313 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
It looks like before this would always return an error whereas this might now return nil. However, I'm not sure it matters because I believe Err() only ever returns the two errors already caught in the switch above.
more importantly:
// If Done is closed, Err returns a non-nil error explaining why:
pkg/testutils/sqlutils/sql_runner.go, line 120 at r1 (raw file):
Previously, stevendanna (Steven Danna) wrote…
For my own understanding, why are we preferring AssertionFailedf in these cases?
my thinking was that it made sense since the test is asserting that the error is the expected type. but i agree with the point that the stack in this case is useless, so i'll change it back
actually, it's worth doing it explicitly anyway since err.Error() or %v doesn't show things like error details/hints, which might be useful to show here too.
634fd19 to
b42548c
Compare
|
merging since failure is from a flaky test bors r=otan,RaduBerinde,stevendanna |
|
Build failed: |
I'm working on a linter that detects errors that are not wrapped correctly, and it discovered these. Release note: None
b42548c to
b1eba61
Compare
|
bors r=otan,RaduBerinde,stevendanna |
72304: bazel: add `pkg/cmd/mirror` r=rail a=rickystewart This tool will be extended with functionality to mirror Go module ZIP's to cloud storage. For now, the mirroring functionality is missing, so we just generate the `DEPS.bzl` content in much the same way as `gazelle update-repos` does. (This doesn't change the content of `DEPS.bzl` in any way besides alphabetizing the contents.) Release note: None 72353: *: fix improperly wrapped errors r=otan,RaduBerinde,stevendanna a=rafiss refs #42510 I'm working on a linter that detects errors that are not wrapped correctly, and it discovered these. Release note: None 72417: sql: add unit tests for creating default privileges r=ajwerner a=RichardJCai Adding some unit test coverage so we don't hit bugs like this again. #72322 Release note: None Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: richardjcai <caioftherichard@gmail.com> Co-authored-by: Richard Cai <RichardJCai@users.noreply.github.com>
|
Build failed (retrying...): |
|
Build succeeded: |
71877: lint: add new errwrap linter r=ajwerner,knz a=rafiss fixes #42510 This linter checks if we don't correctly wrap errors. The `/* nolint:errwrap */` comment can be used to disable the linter inline. See individual commits for mistakes this linter caught. It had already caught a few in #72353, #72352, #72351, #72350, and #72349. Release note: None Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
refs #42510
I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.
Release note: None