Skip to content

Commit ebb0cad

Browse files
committed
test(rust): put back the environment variable a test borrows
`with_var` ended by removing the variable rather than restoring what it held. The Rust legs run against a mock with `GrpcClient__Endpoint` already set, so a test that borrowed it took it away from every `client::` test in the same binary, and all four legs went red: called `Result::unwrap()` on an `Err` value: Config { source: Invalid { source: Uri { source: InvalidUri(Empty), uri: "" ... } } } It now saves the value and puts it back from a `Drop` guard, so a failing test restores too. Restoring is not enough on its own: the `client::` tests are not serialised against this module, so a hardcoded endpoint could be read by one of them mid-flight. The test borrows `GrpcClient__UserAgent` instead, which nothing else depends on, and asserts on `ClientConfigArgs::from_env`, so it needs no endpoint at all. What it dropped, args becoming a parsed configuration, is covered in the transport's own tests. cargo test -p armonik --all-features --lib client::env: 6 passed, 0 failed. cargo fmt --all --check: clean.
1 parent ae435e3 commit ebb0cad

1 file changed

Lines changed: 33 additions & 14 deletions

File tree

  • packages/rust/armonik/src/client

packages/rust/armonik/src/client/env.rs

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -171,16 +171,33 @@ pub enum ReadEnvError {
171171
mod tests {
172172
use super::*;
173173

174-
/// A variable name of its own per test, so that a stray value cannot leak between them even though
175-
/// they are serialised.
174+
/// Puts back what the variable held, rather than removing it: these tests run in a process whose
175+
/// environment may already carry a `GrpcClient__*` that other tests need. On drop, so that a
176+
/// failing test does not take it away from them either.
177+
struct Restore {
178+
name: String,
179+
previous: Option<std::ffi::OsString>,
180+
}
181+
182+
impl Drop for Restore {
183+
fn drop(&mut self) {
184+
match self.previous.take() {
185+
Some(previous) => std::env::set_var(&self.name, previous),
186+
None => std::env::remove_var(&self.name),
187+
}
188+
}
189+
}
190+
176191
fn with_var<T>(name: &str, value: Option<&str>, body: impl FnOnce() -> T) -> T {
192+
let _restore = Restore {
193+
name: name.to_owned(),
194+
previous: std::env::var_os(name),
195+
};
177196
match value {
178197
Some(value) => std::env::set_var(name, value),
179198
None => std::env::remove_var(name),
180199
}
181-
let outcome = body();
182-
std::env::remove_var(name);
183-
outcome
200+
body()
184201
}
185202

186203
#[test]
@@ -271,16 +288,18 @@ mod tests {
271288

272289
#[test]
273290
#[serial_test::serial]
274-
fn the_endpoint_reaches_the_configuration() {
275-
// The one test that the whole chain is wired: a variable set here comes out of
276-
// `from_config_args` as a parsed endpoint.
277-
let config = with_var(
278-
"GrpcClient__Endpoint",
279-
Some("http://localhost:5001"),
280-
ClientConfig::from_env,
291+
fn a_variable_reaches_the_field_that_carries_it() {
292+
// The drift-prone half of this module is the mapping from name to field, so that is what is
293+
// checked. `UserAgent` on purpose: the tests that build a real client share this process and
294+
// are not serialised against this one, so borrowing a variable any of them depends on, the
295+
// endpoint above all, would send them somewhere else while this runs.
296+
let args = with_var(
297+
"GrpcClient__UserAgent",
298+
Some("armonik-test/1"),
299+
ClientConfigArgs::from_env,
281300
)
282-
.expect("a lone endpoint is a valid configuration");
301+
.expect("reading the environment must not fail");
283302

284-
assert_eq!(config.endpoint.to_string(), "http://localhost:5001/");
303+
assert_eq!(args.user_agent, "armonik-test/1");
285304
}
286305
}

0 commit comments

Comments
 (0)