Add WaitAndRecover method to WaitGroup #53
Conversation
waitgroup.go
Outdated
| // WaitSafe will block until all goroutines spawned with Go exit and will | ||
| // return a *panics.RecoveredPanic if one of the child goroutines panics. | ||
| func (h *WaitGroup) WaitSafe() *panics.RecoveredPanic { |
There was a problem hiding this comment.
I'm starting to think that WaitAndRecover() might be a better name. The term "Safe" is pretty overloaded, and it's not super clear what it means without reading the doc string. "Recover" is a much more specific term regarding panics, and is a little more intuitive from my perspective.
The extra verbosity seems acceptable given that most users should probably be using Wait() still.
What do you think?
There was a problem hiding this comment.
I think that is a good call. Updated. Thanks!
waitgroup_test.go
Outdated
| panic("super bad thing") | ||
| }) | ||
| p := wg.WaitSafe() | ||
| require.Contains(t, p.Error(), "super bad thing", p.Error()) |
There was a problem hiding this comment.
nit: we can compare the panic value directly rather than checking the stringified error message.
Also, arguments 4+ for require.Contains() are to optionally add more information to the failure message. The error will already be included in the message if it fails, so there's no need for the optional args here.
| require.Contains(t, p.Error(), "super bad thing", p.Error()) | |
| require.Equal(t, p.Value, "super bad thing") |
There was a problem hiding this comment.
Oops. Thank you! Fixed.
waitgroup_test.go
Outdated
| panic("another bad thing") | ||
| }) | ||
| p := wg.WaitSafe() | ||
| require.Contains(t, p.Error(), "bad thing", p.Error()) |
There was a problem hiding this comment.
This test seems slightly misleading. I think it might be more clear if the test more checked that the returned panic matches one of the panic values exactly. Something like:
require.True(t, p.Value == "one bad thing" || p.Value == "another bad thing")There was a problem hiding this comment.
Ah, good point. How about checking simply if p is non-nil?
I think at first I was thinking about adding a small sleep and checking that the first one to panic is the one that is returned, but that may possibly add flakiness, so I decided against it.
I'll update to a non-nil check for now, but let me know if you would like to be more explicit. Thanks!
Codecov Report
@@ Coverage Diff @@
## main #53 +/- ##
=======================================
Coverage 99.44% 99.45%
=======================================
Files 10 10
Lines 359 364 +5
=======================================
+ Hits 357 362 +5
Misses 2 2
馃摚 We鈥檙e building smart automated test selection to slash your CI/CD build times. Learn more |
craigpastro
changed the title
Resolves #29.
馃憢 My attempt at resolving #29. I wasn't sure if was better to reimplement
Waitin terms ofWaitSafe. I think leaving the code as it looks a bit cleaner, so I just left it.I also wondered if it is better to say in the docs that
WaitSafewill return the first panic it encounters and add a test for that. That is true, right?Thank you!