*: fix improperly wrapped errors #72353
Conversation
|
This change is |
rafiss
force-pushed
the
fix-extra-errwrap
branch
from
cd547cd to
664f622
Compare
rafiss
force-pushed
the
fix-extra-errwrap
branch
2 times, most recently
from
dcb0ad6 to
634fd19
Compare
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().
| @@ -310,7 +310,7 @@ func run() error { | |||
| } | |||
| 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
| @@ -117,7 +117,7 @@ func (sr *SQLRunner) ExpectErrSucceedsSoon( | |||
| sr.succeedsWithin(t, func() error { | |||
| _, 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?
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.
rafiss
force-pushed
the
fix-extra-errwrap
branch
from
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
rafiss
force-pushed
the
fix-extra-errwrap
branch
from
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 <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: richardjcai <[email protected]> Co-authored-by: Richard Cai <[email protected]>
|
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 <[email protected]>
refs #42510
I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.
Release note: None