-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
assert: collect.FailNow() should not panic #1481
Conversation
@marshall-lee Your review of #1395 would be welcome. |
#1395 is now merged. @marshall-lee Please rebase. @czeslavo Please review. |
ab5d73e
to
8bedbde
Compare
I'm wondering whether we could document this behavior somewhere. It would be cool to have an example of using |
@cszczepaniak Could you also help with a review of this MR? |
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 think that the CollectT.failed
field is not necessary to fix the issue.
If we ensure c.errors
is a non nil slice when c.FailNow
is called, the patch can be much smaller.
34c6cc5
to
0ebb845
Compare
4b3659f
to
cbb508a
Compare
@czeslavo @cszczepaniak @dolmen Hey I tried to address all the previous review issues. Please review. |
cbb508a
to
0292bb5
Compare
@czeslavo @cszczepaniak @dolmen Hi! Any plans on merging this? |
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, but I'm just an ordinary contributor here, @dolmen decides 🙂
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.
Rebase needed.
0292bb5
to
2749dce
Compare
It's a good thing this issue has been addressed. Thank you guys |
when will this be released? |
@marshall-lee This seems to have broken the tests for 1.18. |
Summary
collect.FailNow()
should exit goroutine without a panic to be usable withrequire
package.Changes
collect.FailNow()
just doesruntime.Goexit()
instead ofpanic()
. For exampleFailNow()
fromtesting
package behaves similarly.Motivation
I just want
require
package to be usable withEventuallyWithT
e.g. I want this example to pass but it panics:See https://go.dev/play/p/Oqd95IT7qxQ
Related issues
Fixes #1396
Fixes #1457