assert_cmd! unexpected behaviour, which might eat your laundry

[colin-kiegel]

This morning I made the following observation:

• assert_cmd!(rm foo.bar) is equivalent to assert_cmd!(rm foo . bar).

See simplified Playground example. This is due to current limitations of the macro system. Macros can not access information about whitespaces. Period.

But quotations #15 won't work as expected either assert_cmd!(rm "foo.bar") will run rm "\"foo.bar\"".

rm: cannot remove '"foo.bar"': No such file or directory

This turns out to be way more problematic than initially thought...
Interpreting arguments as expressions is also problematic.

• In this world assert_cmd!(rm "foo.bar") would work
• but assert_cmd!(rm foo) could have serious side effects, e.g. if let foo = "/ -r"

This situation doesn't look good at all.

[colin-kiegel]

I was in a rush this morning. I updated the ticket and included a link to a simplified playground example.

My goal this morning was to add a hint, what you have to watch out for. When I noticed this behaviour I ran out of time and hastily filed this issue.

@killercup we could try to restrict what this macro accepts. I already have an idea, but the usefulness of the macro will stay limited, but at least we could get something safe.

[colin-kiegel]

Ok, here is a proof of concept, how we could implement safety checks in assert_cmd!.

assert_cmd!(echo 42);      // not rejected -> works :-)
assert_cmd!(rm "foo.bar"); // not rejected -> but does *not* work as expected
                           // all we'd need here is string-decode it, to make it work.
    
//assert_cmd!(cargo test --test foo);  // rejected -> does not work
//assert_cmd!(cat non-exisiting-file); // rejected -> does not work
//assert_cmd!(rm foo.bar);             // rejected -> does not work

[colin-kiegel]

Ok, so this is not so bad after all. If we find a solution for the quotation bug #15, we would gain full expressiveness.

I think I will include something like this safety-check in the PR #16. @killercup Is this ok for you?

[killercup]

Hmm. This still seems fragile. What do you think about removing the macro and adding <T: ToCommandWithArgs> Assert::command(T) where ToCommandWithArgs is a trait implemented for &[u8] as well as &str? This way, you could run Assert::command(r#"cargo run --bin whatever -- --input="Lorem ipsum" -f"#).

In the &str case we'd parse it in a clever way to split the string by whitespace while preserving quoted blocks. I'm thinking of some split_args that fulfills

#[test]
fn test_arg_split() {
    assert_eq(split_args("echo 42"), vec!["echo", "42"]);

    assert_eq(split_args(r#"echo "42""#), vec!["echo", "\"42\""]);

    assert_eq(
        split_args(r#"cargo run --bin whatever -- --input="Lorem ipsum" -f"#),
        vec!["cargo", "run", "--bin", "whatever",  "--", "--input=\"Lorem ipsum\"", "-f"]
    );
}

[colin-kiegel]

That sounds interesting, too.

Btw. I just pushed this commit fixing both #15 and #22. It works ok now. :-)

The semantics are that you should use quotes

assert_cmd!(cargo "test" "--test" "foo"); // works now

but you can omit them in ident-like or number-like cases

assert_cmd!(cargo test "--test" foo); // works now

however this will complain

assert_cmd!(cargo test --test foo); // does not parse
                    // ^~~ unexpected token `-`

This is as far as we will get with the macro. I think it's ok, to keep it for now. We can still replace it after the PR has landed.