Add with_env helper #46

Freyskeyd · 2017-10-09T14:10:53Z

Check the discussion in #27

Signed-off-by: Freyskeyd [email protected]

killercup

Very cool! Thank you so much for this!

I left some inline comments. Most importantly, please move the tests so they become unit tests.

Also, I saw you merged master into this. That's fine; please keep on adding commits to this branch without rebasing, but it'd be awesome if you could squash it once all reviews are done :)

killercup · 2017-10-09T14:18:20Z

src/assert.rs

@@ -24,6 +26,7 @@ impl default::Default for Assert {
        Assert {
            cmd: vec!["cargo", "run", "--"]
                .into_iter().map(String::from).collect(),
+            env: Environment::empty(),


I think it should default to inherit.

killercup · 2017-10-09T14:20:01Z

src/environment.rs

+    pub vars: Vec<(String, String)>,
+    /// Define if the structure must inherit
+    pub inherit: bool,
+}


The fields should not be pub. The compile method's output is all we should ever test in integration tests.

killercup · 2017-10-09T14:20:44Z

src/environment.rs

+
+    /// Compile Environment object
+    pub fn compile(&self) -> Vec<(String, String)> {
+        ::std::env::vars().chain(self.vars.clone()).collect()


Shouldn't this have an if self.inherit?

killercup · 2017-10-09T14:21:04Z

src/assert.rs

-        let command = command.args(&args);
+        let command = command
+            .env_clear()
+            .envs(::std::env::vars().chain(self.env.clone().vars))


I'm pretty sure you want to call compile here.

killercup · 2017-10-09T14:22:40Z

tests/environment.rs

@@ -0,0 +1,72 @@
+extern crate assert_cli;


Can you move all the tests into a #[cfg(test)] mod test in environment.rs and use the real with_env?

The fact is, with_env doesn't allow to test the environment object after ...

killercup · 2017-10-09T14:23:23Z

src/environment.rs

+    fn to_environment_tuple(&self) -> (String, String);
+}
+
+impl<'s, T: ToString, Z:ToString> EnvironmentItem for &'s (T, Z) {


space after Z:

killercup · 2017-10-09T14:26:40Z

src/assert.rs

+    ///     .stdout().contains("TEST_ENV=OK")
+    ///     .execute()
+    ///     .unwrap();
+    /// ```


Feel free to add some more examples here as they are the real integration tests :)

epage

I'm really liking this API!

epage · 2017-10-09T14:22:59Z

src/assert.rs

-            .stderr(Stdio::piped());
-        let command = command.args(&args);
+            .stderr(Stdio::piped())
+            .env_clear()


There a reason we unconditionally clear and then re-add the vars we cleared via compile?

In fact we need the env_clear() because .envs doesn't clear it or replace the whole variables.

I think I'm missing something. I had assumed we'd only call env_clear() when inherit == false. Its unclear to me why we need env_clear when inherit == true.

By clearing the environment before adding our own environment, we don't have to check if inherit is true nor false in execute command. We just want to clear and add the appropriate environment. That's the environment role to know when to add the existing env or not.

If inherit is set to true, the compile method will return a merge of actual environment (env::vars()) and our customized variables. I think it's nicer to have a compile method returning what our environment will look like. But feel free to propose something else I'm ok with it :)

tl;dr move along, these aren't the droids you're looking for.

I think it's nicer to have a compile method returning what our environment will look like.

That is nice. It helps with testing, etc.

I just assumed that Assert would directly access the members of Environment and there wouldn't be a need for a public .compile().

Recreating an inherited environment is more work but I'm assuming it doesn't matter in this case, despite it being against "you pay for what you use" (adds cost to people not using these new features).

@killercup how do you see this test?

Set an env var to something that is not UTF-8. How would I get a byte slice like that? head -c 42 /dev/urandom | rcat --quote (with this rcat) gives me:

b"\xf4\xa9\xd5\x8d\xc0\xe8\x00\xff\x1dqz\x86k\x83\xcb~\xc1\x18!\xfa\x01J\x1c\\:\x18)%\xd4\xa6^7\xb4\x93\x80.\x9a\x9c\xe1\xcf\xe2,"

which is not valid UTF-8.

Yes, but vars() use UTF-8 variables, do we really want to support not valid UTF-8? I don't think..

That's exactly what I'm saying: I'm fine with only allowing our users to add UTF-8 vars but we should not destroy valid, inherited env vars. So, calling vars at all is probably not a good call, and vars_os is the better option. Btw, you can find the impl of how std::process::Command deals with env vars on Unix here (it uses a HashMap<OsString, (usize, CString)> to store env vars).

How much work would it be to allow OsStr instead of String in our APIs?

Can you still add a test with the non-UTF-8 env var value?

epage · 2017-10-09T14:23:32Z

src/assert.rs

-        let command = command.args(&args);
+            .stderr(Stdio::piped())
+            .env_clear()
+            .envs(self.env.clone().compile()))


This function consumes Asssert so we should be able to avoid the .clone()

epage · 2017-10-09T14:25:06Z

src/assert.rs

@@ -26,6 +29,7 @@ impl default::Default for Assert {
        Assert {
            cmd: vec!["cargo", "run", "--"]
                .into_iter().map(String::from).collect(),
+            env: Environment::empty(),


Should the default be

empty

inherit (I presume this is std::process::Command's default and the current behavior)

None (use std::process::Command's default which is the current behavior)

epage · 2017-10-09T14:26:20Z

src/environment.rs

+    /// Customized environment variables
+    pub vars: Vec<(String, String)>,
+    /// Define if the structure must inherit
+    pub inherit: bool,


The members should probably not be pub

See https://rust-lang-nursery.github.io/api-guidelines/future-proofing.html#structs-have-private-fields-c-struct-private

epage · 2017-10-09T14:27:32Z

src/environment.rs

+}
+
+impl Environment {
+    /// Create a new Environment object with inherit


I feel like a little more context would be helpful
Create a new Environment that inherits this process' environment.

epage · 2017-10-09T14:28:22Z

src/environment.rs

+        }
+    }
+
+    /// Create a new Environment object without inheriting


Same about more context. Also, it can be confusing to describe something by what it is not.

So maybe Create a new Environment independent of the current process's Environment?

killercup · 2017-10-09T15:20:44Z

src/assert.rs

@@ -3,14 +3,17 @@ use std::process::{Command, Stdio};
 use std::path::PathBuf;
 use std::vec::Vec;
 use std::io::Write;
+use std::convert::Into;


That's in the prelude

killercup · 2017-10-09T15:23:45Z

src/environment.rs

+
+    fn command() -> Assert {
+        Assert::command(&["printenv"])
+    }


That's very clever 👍

Can you add an empty line below the fn?

killercup · 2017-10-09T15:24:32Z

src/environment.rs

+#[cfg(test)]
+mod test {
+    use super::*;
+    use super::super::Assert;


I know it's a very good Assert, but use ::Assert; should suffice ;)

epage · 2017-10-09T15:57:54Z

src/environment.rs

+        if self.inherit {
+            ::std::env::vars().chain(self.vars.clone()).collect()
+        } else {
+            self.vars.clone()


Should Environment be moved into this?

This would give the caller control on whether the .clone()s are needed or not. In our case, we call compile when consuming Assert, so we wouldn't need to call any of these .clone()s.

Freyskeyd · 2017-10-09T17:55:11Z

.travis.yml

@@ -5,13 +5,6 @@ rust:
 - beta
 - stable
 - nightly
-matrix:


@killercup @epage I removed the nightly-2017-03-15, are you ok with this?

The .envs methods from Command is stable since 1.10. But not for the 2017-03-15 build. I was wondering about why we are still testing this particular build.

The reason (which I like and want to copy to my other projects) is to pin the version of rust/clippy.

In my other projects, we are always running the latest clippy. Sometimes it is out of sync with rustc, so the build ignores clippy, losing coverage. Even when it succeeds, there might be new clippy warnings which will get turned into errors, sporadically breaking our build.

What I'd recommend is to lock us to a new version of clippy. This might be best as a separate PR so any clippy fixes don't get mixed in with your changes.

I'm a sucker for lints, clippy needs to stay! Hell, I might even enforce rustfmt!

What I'd recommend is to lock us to a new version of clippy. This might be best as a separate PR so any clippy fixes don't get mixed in with your changes.

I'll just do that in minute, @Freyskeyd you can merge in master after this.

Update: #47

@killercup I love having everything checked in the CI!

I'm unsure how stable rustfmt's output is from release to release. I've not yet submitted a rustfmt.toml to my other projects. I have played with a build optimization for rustfmt for locking to a specific version that you might find handy.

See https://github.com/cobalt-org/cobalt.rs/blob/master/.travis.yml#L29

You're using the old rustfmt, not rustfmt-nightly :) I opened #48 for getting our formatting in check.

Freyskeyd · 2017-10-10T10:59:21Z

@killercup can you check why the CI doesn't pass?

killercup · 2017-10-10T11:27:54Z

Probably because I tried being too clever… can you append || true to the travis.yml lines with the cargo install calls? 😅

epage · 2017-10-10T14:44:52Z

The command "[[ "$(cargo +nightly-2017-10-09 fmt -- --version)" != "$RUSTFMT_VERS"* ]] && \
  cargo +nightly-2017-10-09 install rustfmt-nightly --vers "$RUSTFMT_VERS" --force
  " failed and exited with 1 during .

https://travis-ci.org/killercup/assert_cli/builds/285971508

Probably because I tried being too clever… can you append || true to the travis.yml lines with the cargo install calls? 😅

I assume the || true is needed when the version check fails (its already installed), causing us not to reinstall the tool?

killercup · 2017-10-10T14:49:44Z

@Freyskeyd, wow, thanks for the quick OsString update!

@epage, yeah, it's a ternary: [[ if ]] && then || else. Which evals to a non-zero exit code without the else branch 😅

epage · 2017-10-10T14:57:13Z

@epage, yeah, it's a ternary: [[ if ]] && then || else. Which evals to a non-zero exit code without the else branch 😅

Would that cause the return value of then to be ignored?

Freyskeyd · 2017-10-10T15:00:14Z

@killercup I tried with OsStr but it's a bit tricky. OsString is more easy and has the same type as vars_os().

About travis, what do you want to do?

killercup · 2017-10-10T15:33:02Z

Would that cause the return value of then to be ignored?

Maybe? Probably. We could also try to use a real if. Tbh, I'm fine with dealing with this later…

OsString is more easy and has the same type as vars_os().

Yes, sorry, I meant OsString!

About travis, what do you want to do?

Eh, I want you to fix the valid error and the formatting? 😛

Freyskeyd · 2017-10-12T09:53:46Z

@killercup @epage I updated the code, can you check it please? :)

Freyskeyd · 2017-10-12T11:53:24Z

I move it to another crate because I need it outside assert_cli.

https://github.com/Freyskeyd/environment
https://crates.io/crates/environment

killercup

Awesome work. Having this as an external crate is pretty cool. Feel free to advertise that assert_cli uses it in its Readme ;)

Just a few more tiny nits and I will happily merge this :)

killercup · 2017-10-12T14:38:10Z

src/assert.rs

-            .stderr(Stdio::piped());
-        let command = command.args(&args);
+            .stderr(Stdio::piped())
+            .env_clear()


Can you still add a test with the non-UTF-8 env var value?

killercup · 2017-10-12T14:41:09Z

src/lib.rs

@@ -135,3 +137,4 @@ mod diff;
 mod assert;
 pub use assert::Assert;
 pub use assert::OutputAssertionBuilder;
+pub use environment::Environment;


Do you want to add some specific docs here mentioning this is a report from the external crate? I assume it inherits the docs from the item in the crate right now.

killercup · 2017-10-12T14:43:02Z

src/assert.rs

+    fn in_place_mod2() {
+        let x = Environment::inherit();
+
+        assert!(


Why the assert!(… .is_ok()) dance when you can just write .unwrap()?

killercup · 2017-10-12T14:54:54Z

src/assert.rs

+    ///     .stdout().is("FOO=BAR")
+    ///     .execute()
+    ///     .unwrap();
+    /// ```


Great examples! Can you maybe add a third one that sets a var in the test's thread context and then shows that it's also available in the command output?

killercup

Thanks for the updates!

Use Environment crate to override and insert new environment variables. Signed-off-by: Freyskeyd <[email protected]>

killercup · 2017-10-12T18:16:13Z

bors r+

46: Add with_env helper r=killercup a=Freyskeyd Check the discussion in #27 Signed-off-by: Freyskeyd <[email protected]>

bors · 2017-10-12T18:23:32Z

Build succeeded

continuous-integration/travis-ci/push

killercup suggested changes Oct 9, 2017

View reviewed changes

epage suggested changes Oct 9, 2017

View reviewed changes

Freyskeyd force-pushed the master branch from 21ada32 to 4be66fd Compare October 9, 2017 15:08

killercup reviewed Oct 9, 2017

View reviewed changes

Freyskeyd force-pushed the master branch 2 times, most recently from f3b6e26 to 82288e6 Compare October 9, 2017 15:40

epage suggested changes Oct 9, 2017

View reviewed changes

Freyskeyd force-pushed the master branch from 82288e6 to d9150ce Compare October 9, 2017 17:54

Freyskeyd commented Oct 9, 2017

View reviewed changes

killercup mentioned this pull request Oct 9, 2017

Run and add rustfmt #49

Merged

Freyskeyd force-pushed the master branch from d9150ce to 9b3ffd3 Compare October 10, 2017 09:00

Freyskeyd force-pushed the master branch 2 times, most recently from 588998b to 8cb7289 Compare October 10, 2017 12:17

Freyskeyd force-pushed the master branch 5 times, most recently from 3a528a9 to 835a843 Compare October 12, 2017 09:53

Freyskeyd force-pushed the master branch from 835a843 to be217ca Compare October 12, 2017 09:54

epage approved these changes Oct 12, 2017

View reviewed changes

epage mentioned this pull request Oct 12, 2017

Future of assert_cli #41

Open

killercup suggested changes Oct 12, 2017

View reviewed changes

Freyskeyd force-pushed the master branch 2 times, most recently from 34fb40b to 16aaa61 Compare October 12, 2017 17:39

killercup approved these changes Oct 12, 2017

View reviewed changes

add with_env method to Assert

060a7c0

Use Environment crate to override and insert new environment variables. Signed-off-by: Freyskeyd <[email protected]>

Freyskeyd force-pushed the master branch from 16aaa61 to 060a7c0 Compare October 12, 2017 18:14

bors bot added a commit that referenced this pull request Oct 12, 2017

Merge #46

aa318ad

46: Add with_env helper r=killercup a=Freyskeyd Check the discussion in #27 Signed-off-by: Freyskeyd <[email protected]>

bors bot merged commit 060a7c0 into assert-rs:master Oct 12, 2017

epage mentioned this pull request Oct 12, 2017

Setting environment variables #27

Closed

epage mentioned this pull request Oct 21, 2017

with_env clearing the environment is jarring #58

Open

@@ @@ -5,13 +5,6 @@ rust: @@
               - beta
               - stable
               - nightly
-              matrix:

Add with_env helper #46

Add with_env helper #46

Conversation

Freyskeyd commented Oct 9, 2017

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freyskeyd commented Oct 10, 2017

killercup commented Oct 10, 2017

epage commented Oct 10, 2017

killercup commented Oct 10, 2017

epage commented Oct 10, 2017

Freyskeyd commented Oct 10, 2017

killercup commented Oct 10, 2017

Freyskeyd commented Oct 12, 2017

Freyskeyd commented Oct 12, 2017

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

killercup commented Oct 12, 2017

bors bot commented Oct 12, 2017

Build succeeded

killercup Oct 9, 2017 •

edited

Loading