Fix test that calls setenv() without synchronization #64
Conversation
This test is flaky under the following interleaving: - exec_to_string() grabs MUTATE_ENV. - env_inherit_set() calls temp_env_var() which calls env::set_var(). - exec_to_string() executes some asserts, one of which fails because the other test messed with the environment in a way exec_to_string() did not expect. - exec_to_string() panics and poisons MUTATE_ENV. - env_inherit_set() grabs the lock and panics because it's poisoned. Taking the lock *before* setenv fixes this.
| @@ -317,10 +317,12 @@ struct TmpEnvVar<'a> { | |||
| } | |||
|
|
|||
| impl<'a> TmpEnvVar<'a> { | |||
| fn new(varname: &'static str) -> TmpEnvVar<'a> { | |||
| fn new(varname: &'static str, tmp_value: &'static str) -> TmpEnvVar<'a> { | |||
There was a problem hiding this comment.
I agree with the change, I'd prefer TmpEnvVar::new() not to actually call setenv(). When possible, constructors should be side-effect-free.
The simplest fix would be to modify the body of tmp_env_var() to something like:
{
let guard = TmpEnvVar::new(varname);
env::set_var(varname, tmp_value);
guard
}There was a problem hiding this comment.
Given that TmpEnvVar's sole purpose in life is to roll back a set_env call, maybe it might make sense to replace new with TmpEnvVar::set_var or something to that effect. I know this is test code, but the fact that TmpEnvVar::new(...) own will have a side effect when it is destroyed, it makes sense to relieve the possibility of surprise in this case by making it impossible to get into that situation.
(Like, I just wanna fix a flaky test; if you'd really rather I do it this way, that's fine by me.)
This test is flaky under the following interleaving:
the other test messed with the environment in a way exec_to_string()
did not expect.
Taking the lock before setenv fixes this.