roachtest: remove all panics from perturbation/* tests #130866
Conversation
andrewbaptist
requested review from
nameisbhaskar and
renatolabs
and removed request for
a team
|
This change is |
|
@renatolabs I removed all the panics from the perturbation tests. Thanks for the pointer that this was necessary! |
There was a problem hiding this comment.
Changes are fine to me but, for the record, the panics removed here would not cause the test suite to crash. The issue is not that panics cause tests to fail but instead that panics in "bare" go functions do (unless you always remember to call recover). This is because we can't recover from panics from a different goroutine. This is a more general point about Go programs1 rather than something to do with roachtest. See also #58164.
The panic removed in #130803 was only a problem because it happened in one such bare go call. Calling panic in the main test Run function or in a monitor.Go() function are both fine (t.Fatal and require are implemented as panics anyway).
Footnotes
Panics in the test cause the entire roachtest to stop. Instead we should call t.Fatal or similar methods. This commit passes test.Test to most methods and removes all panics from the perturbation tests. Epic: none Release note: None
andrewbaptist
force-pushed
the
2024-09-17-no-panic
branch
from
00e4d91 to
b7b61d0
Compare
|
TFTR! Its probably still best to remove the panics, then if something moves into/out of a bare goroutine it won't cause problems. bors r=renatolabs |
Panics in the test cause the entire roachtest to stop. Instead we should call t.Fatal or similar methods. This commit passes test.Test to most methods and removes all panics from the perturbation tests.
Epic: none
Release note: None