Tests trigger shotover shutdown and then unwrap the result of the join handle #139
Conversation
| ..ConfigOpts::default() | ||
| }; | ||
| let spawn = Runner::new(opts).unwrap().run_spawn(); | ||
| pub struct ShotoverManager { |
There was a problem hiding this comment.
Should this be a dev-dep and go in the test-helpers dep as this appears to be duplicated in the tests folder as well
There was a problem hiding this comment.
The problem is that they depend on types from the shotover-proxy crate and moving them to test-helpers would create a circular dependency.
A possible fix is to move the shotover-proxy specific logic into a lambda passed in by a benches/helpers.rs wrapper.
But I wanted to investigate that in a separate PR.
Although if your not comfortable with leaving so much code duplicated for now, I can investigate it here?
There was a problem hiding this comment.
Ok makes sense.
DRY is good, but as you hinted at this smells like a pre-mature optimisation without further investigation.
I don't mind the duplicated code, feel free to merge.
|
Feel free to close out #138 as well as this looks to be the preferred approach |
I figured out a solution to the problems I hinted at in #138
The solution to panicking in drops in test helpers is to check
std::thread::panickingto tell if we are currently panicking. If we are panicking then we should not run any code that can panic.We could possibly simplify the code in the future by allowing double panics if rust-lang/rust#82850 is ever resolved. But the current implementation is fine and its probably desirable to never double panic regardless of rust-lang/rust#82850 anyway.
closes #134
This PR improves tests such that:
error!DockerComposecan no longer cause double panics.This PR improves the shutdown process by adding the trigger_shutdown broadcast channel so that we can trigger a shutdown from any source.
This PR supercedes #138 so we can just close 138 if we are happy with this one.