Setting environment variables

[killercup]

Supporting setting/clearing of the environment variables.

AssertCli::command("echo $foo").with_env(es) where es is an IntoIter<Item=(&str, &str)>, e.g. &[("foo", "bar")]

cf. #21

[Freyskeyd]

I'm on it.

I need to be able to override environment variables for some tests.

You said setting/clearing of the environment variables, it means that with_env override completely all environment variables? Does it merge with the currently defined variables?

[killercup]

it means that with_env override completely all environment variables?

Yeah I think that's what past me was thinking 😅

std::env::vars returns this struct which is a Interator<Item=(String, String)>. So, inheriting some env vars and adding others is possible with some iterator chaining (cf. this playpen), but it's not very nice.

Do you have a suggestion how to make this a nice API?

---

From what I can tell, this feature probably doesn't block our next release (hopefully 1.0).

[epage]

When I was considering implementing this, I was thinking the API would look like

• env to add one variable to existing environment
• envs to add a series of variables to the existing environment
• with_env to set a new environment
  (basing the API off of std::prcoess::Command)

This allows the program under test to inherit an environment or get one from scratch, as well as add additional variables to it.

I imagine the Assert would contain

• clear_environ: bool
• envs: Vec<(OsStr, OsStr>

So the operations would have the following behavior

• env appends
• envs extends
• with_env sets clear_environ and clears envs

[Freyskeyd]

@epage I agree.

I was thinking of:

let test = assert_cli::Assert::command(&["some", "command"])
    .env("key", "value") // Just add one
    .envs(&[("key", "value")]) // Extends with multiple vars
    .with_env(&vars) // Clean and write new bunch of vars
    .remove("key") // Remove only one var
    .stdout().contains("42")
    .unwrap();

assert!(test.is_ok());

What do you think?
Do we need to provide a way to remove a bunch of vars? like .remove(&["key1", "key2"]) ?

[killercup]

(Oh, right, should be OsStr. Even though vars() returns (String, String) for some reason…)

Do you think it'd be a good idea to move this to its own small DSL so we only need to have with_env and can also cache and share env var settings between AssertCli calls? Something like

use assert_cli::{Assert, Env};

let vars: Env = Env::inherit().except(&["FOO", "BAR"]).add("FOO", "42");

Assert::command(&["whatever"])
.with_env(&vars)
.fails().unwrap();

Assert::command(&["whatever"])
.with_env(&vars.add("BAR", "1337"))
.contains("OK").unwrap();

[epage]

Do we need to provide a way to remove a bunch of vars? like .remove(&["key1", "key2"]) ?

Overall, my opinion is to model the API after Process. In that context its called env_remove and it only accepts one parameter

[Freyskeyd]

Why not. But I like the way we can add single (or more) variable without instantiate a new variable:

Assert::command(&["whatever"])
.with_env(&[("key", "value"), ("key2", "value")]) // inherit and set/override key and key2
.fails().unwrap();

let vars: Env = Env::inherit().except(&["FOO", "BAR"]).add("FOO", "42");

Assert::command(&["whatever"])
.with_env(&vars) // use the previously defined vars
.fails().unwrap();

One thing is possible. with_env can accept a ToEnv trait.

When passing a &[("key", "value")], we should be able to define an Env with inherit and override keys.

[killercup]

with_env can accept a ToEnv trait.

Good idea for a shortcut. Not sure about the defaulting to inherit, though. I think it'll be the most commonly used option, but the inherit is very implicit…

[epage]

Do you think it'd be a good idea to move this to its own small DSL so we only need to have with_env and can also cache and share env var settings between AssertCli calls? Something like

• That was the main alternative in my mind to my stdout API I wrote. The main reason I didn't do it is brevity.
• It is more like how clap does its fluent API.
• Reuse in tests is an interesting problem. It highlights even more the problem with over applying DRY because they tend towards more incidental code patterns (diverge with requirements) rather than inherent code patterns (shared requirements)

For me, personally, I don't yet have use cases enough to use environments to really say whether the API is best with reuse or not.

I would say that the API should not borrow so that someone can declare their environment inline

Assert::command(&["whatever"])
.with_env(Env::empty().add("FOO", "42"))
.fails().unwrap();

[epage]

Good idea for a shortcut. Not sure about the defaulting to inherit, though. I think it'll be the most commonly used option, but the inherit is very implicit…

btw with your separate DSL, I really liked the idea of calling it inherit

[epage]

Love the ToEnv idea (and agree about it not inheriting by default). The name with_env to me implies that its replacing, like with_file_name and friends.

[Freyskeyd]

@epage you're right about with_env and inheriting.

To be clear:

That way we are clearing and defining only one environment variable: key=value

Assert::command(&["whatever"])
    .with_env(&[("key", "value")]) 
    .fails().unwrap();

If we want to just add (or override) an environment variable

Assert::command(&["whatever"])
    .with_env(&Env::inherit().add("key", "value")) 
    .fails().unwrap();

We can also define an Env before and reuse it:

let mut env: Env = Env::cleared().add("key", "value");

Assert::command(&["whatever"])
    .with_env(&env) 
    .fails().unwrap();

env.remove("key");

Assert::command(&["whatever"])
    .with_env(&env) 
    .fails().unwrap();

I would say that the API should not borrow so that someone can declare their environment inline

Can I ask you why?

[epage]

I would say that the API should not borrow so that someone can declare their environment inline

Can I ask you why?

I remembered having problems when doing something like

Assert::command(&["whatever"])
    .with_env(&Env::inherit().add("key", "value")) 
    .fails().unwrap();

and I assumed it was due to not being able to borrow that temporary long enough but maybe it works and my problem was something else.

This does remind me that if we intend to borrow the environment, then we'll be needing to add lifetimes to Assert which might be a bit of a pain especially when the performance might not matter in this context.

I could see making with_env automatically clone if it takes in a reference to make it more ergonomic for the client for when they want to transfer ownership or not.

[killercup]

Instead of a ToEnv trait: Can with_env take a T: Into<Env>? And does/can &Env impl Into<Env> (which is just .clone())? Ed Page <[email protected]> schrieb am Mi. 4. Okt. 2017 um 17:36:

…

I would say that the API should not borrow so that someone can declare their environment inline Can I ask you why? I remembered having problems when doing something like Assert::command(&["whatever"]) .with_env(&Env::inherit().add("key", "value")) .fails().unwrap(); and I assumed it was due to not being able to borrow that temporary long enough but maybe it works and my problem was something else. This does remind me that if we intend to borrow the environment, then we'll be needing to add lifetimes to Assert which might be a bit of a pain especially when the performance might not matter in this context. I could see making with_env automatically clone if it takes in a reference to make it more ergonomic for the client for when they want to transfer ownership or not. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#27 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOX0cCXjF-HNdz1AB60tS_EjKumAXhks5so6XxgaJpZM4Mmvvp> .

[killercup]

(impl<'a> ToEnv for &'a Env should work in any case) Pascal <[email protected]> schrieb am Mi. 4. Okt. 2017 um 18:01:

…

Instead of a ToEnv trait: Can with_env take a T: Into<Env>? And does/can &Env impl Into<Env> (which is just .clone())? Ed Page ***@***.***> schrieb am Mi. 4. Okt. 2017 um 17:36: > I would say that the API should not borrow so that someone can declare > their environment inline > > Can I ask you why? > > I remembered having problems when doing something like > > Assert::command(&["whatever"]) > .with_env(&Env::inherit().add("key", "value")) > .fails().unwrap(); > > and I assumed it was due to not being able to borrow that temporary long > enough but maybe it works and my problem was something else. > > This does remind me that if we intend to borrow the environment, then > we'll be needing to add lifetimes to Assert which might be a bit of a > pain especially when the performance might not matter in this context. > > I could see making with_env automatically clone if it takes in a > reference to make it more ergonomic for the client for when they want to > transfer ownership or not. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#27 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABOX0cCXjF-HNdz1AB60tS_EjKumAXhks5so6XxgaJpZM4Mmvvp> > . >

[Freyskeyd]

@killercup yep that's what I think.

I will try to implement a first draft to discuss about it.

[Freyskeyd]

@killercup @epage what do you think?

https://play.rust-lang.org/?gist=9fe95d4554be169de1fdf78a6ee7a97f&version=stable

[epage]

Overall, looks good. I did tweak it some to see how it looks

• with non-placeholder names
• with real function implementations
  so I could better consider it

https://play.rust-lang.org/?gist=a22dcd5f647a390e5287330dedb2d4bb&version=stable

No clue why my impls for a few cases aren't working. Overall, I was switching from AsRef to Into<String> just so we can save allocations in case the caller can transfer them (which is unlikely and the performance difference is probably negligible shrug)

I'm a bit mixed on what cases are important or not to provide conversions for. I believe the general recommendation is to take slices or iterators but our goal isn't as much a general purpose API but an ergonomic API for writing tests which can have different care abouts.

[killercup]

Good work, you two! I think this is one of these awful cases where it all comes down to Rust hating slices not supporting const generics.

I cleaned up the code a bit and added some stuff: https://play.rust-lang.org/?gist=8aa2dfa10ca1f6ce2861d0430f32e584

I would really like to support the following:

with_env(&[("Foo", "bar")]);

let x: String = some_magic_black_box_fn();
with_env(&[("Foo", &x)]);
with_env(&[("Foo", x)]);

with_env(Env::inherit().insert("Foo", "bar"));

I'm a bit mixed on what cases are important or not to provide conversions for.
[…] our goal isn't as much a general purpose API but an ergonomic API for writing tests […]

Exactly. I'm fine with only supporting calling this with vec! as this should never ever be performance critical code (famous last words).

[killercup]

Oh! @epage, you renamed add to insert with this comment:

Since this is meant to act like a map, calling it insert

I'm fine with that, but would like to point out that we are using a Vec<(K, V)> instead of an actual map.

Do we want to allow overwriting env vars? Or just appending?

[Freyskeyd]

@killercup I take your code and made all test passes. I will start working on the implementation.
We can use:

    let x = Environment::inherit();
    with_env(x.clone());
    with_env(&x);
    with_env(x);

    let y = Environment::empty();
    with_env(y.insert("key", "value"));

    let x = Environment::inherit();
    with_env(&x.insert("key", "value").insert("key", "vv"));

    let y = Environment::empty();
    with_env(y.clone().insert("key", "value"));
    with_env(y);

    let v = vec![("bar".to_string(), "baz".to_string())];
    with_env(&vec![("bar", "baz")]);
    with_env(&v);
    with_env(&[("bar", "BAZ")]); 
    with_env(&[("bar", "BAZ")][..])
    with_env([("bar", "BAZ")].as_ref());
    with_env(&[("bar".to_string(), "BAZ".to_string())]);
    with_env(&[("bar".to_string(), "BAZ".to_string())][..]);
    with_env(&[("hey", "ho")]);
}

[epage]

I'm fine with that, but would like to point out that we are using a Vec<(K, V)> instead of an actual map.

Do we want to allow overwriting env vars? Or just appending?

I saw the usage of the Vec as an implementation detail. I felt like the behavior was more analogous to maps. This is with the assumption that envs (IntoIterator<Item = (K, V)>) will overwrite an environment variable if a key is repeated. I figured we didn't need to go through the extra work of enforcing any of this behavior in our own code.

[epage]

Fixed by #46