Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distinction between Assert::set_stdin() and with_stdin is confusing #29

Closed
zetok opened this issue Jul 24, 2018 · 4 comments
Closed

Distinction between Assert::set_stdin() and with_stdin is confusing #29

zetok opened this issue Jul 24, 2018 · 4 comments

Comments

@zetok
Copy link

zetok commented Jul 24, 2018

No idea if it doesn't actually work, or if I'm doing something wrong and API is simply allowing for it, but either way it should be fixed.

assert_cmd = "0.6"

my test case:

#[test]
fn broek_dep() {
    Command::new("cat")
        .assert()
        .set_stdin(vec![0])
        .stdout("\0")
        .success();
}

Which results in:

thread 'broek_dep' panicked at 'Unexpected stdout
command=`"cat"`
stdin=``````
code=0
stdout=``````
stderr=``````
',

Also on a side note, output of assert_cmd when there's an error is rather confusing and not helpful when you're trying to deal with input/output that consists of raw, often unprintable, bytes. Debug output without additional ``` would work a lot better IMO.

@epage
Copy link
Contributor

epage commented Jul 25, 2018

No idea if it doesn't actually work, or if I'm doing something wrong and API is simply allowing for it, but either way it should be fixed.

#[test]
fn broek_dep() {
    Command::new("cat")
        .assert()
        .set_stdin(vec![0])
        .stdout("\0")
        .success();
}

Are you converting over from assert_cli? That wouldn't run the program until the expression was completed. assert_cmd runs at the time of assert or earlier. I've started an issue for documenting migration, see #30

So that set_stdin call you are making doesn't do what you think. Instead of writing it out (see instead with_stdin), the content from with_stdin is passed to set_stdin.

I can see how this isn't letting people fall into the pit of success. Any ideas for better names or other means to help people use the right function and avoid the one more so meant for internal purposes (but is pub for anyone writing custom logic)

@epage epage changed the title Assert::set_stdin() doesn't seem to work Distinction between Assert::set_stdin() and with_stdin is confusing Jul 25, 2018
@zetok
Copy link
Author

zetok commented Jul 25, 2018

Are you converting over from assert_cli?

No, but I've found out about assert_cli first from a blog post, and then I've looked at docs of both crates to see what's different.

Any ideas for better names or other means to help people use the right function and avoid the one more so meant for internal purposes (but is pub for anyone writing custom logic)

I think that having something like a bold Warning in the documentation of the function explaining that this might not be what they want and recommending using with_stdin instead would do the trick.

@epage
Copy link
Contributor

epage commented Jul 25, 2018

Forgot to reply to:

Also on a side note, output of assert_cmd when there's an error is rather confusing and not helpful when you're trying to deal with input/output that consists of raw, often unprintable, bytes. Debug output without additional ``` would work a lot better IMO.

Please open a distinct issue for this

@epage
Copy link
Contributor

epage commented Jul 25, 2018

I think that having something like a bold Warning in the documentation of the function explaining that this might not be what they want and recommending using with_stdin instead would do the trick.

Documentation just patches over problems and only helps so far as it is read (e.g. the current documentation already pretty clearly states how the parameter to set_stdin works).

I'm considering things like

  • Changing the name (adding "context")
  • Making this functions pub(crate) until there is a need and handle it then
  • Try to make clearer the idea that the problem has already run by the time Assert exists.

epage added a commit to epage/assert_cmd that referenced this issue Jul 27, 2018
While the documentation for `set_stdin` and `with_stdin` is clear, we
can't expect everyone to read the documentation closely.  This attempts
to make the API harder to misuse so users can be more successful.

BREAKING CHANGE: `Assert::set_cmd` and `Assert::set_stdin` have been
replaced with `Assert::append_context`.

Fixes assert-rs#29
epage added a commit to epage/assert_cmd that referenced this issue Jul 27, 2018
While the documentation for `set_stdin` and `with_stdin` is clear, we
can't expect everyone to read the documentation closely.  This attempts
to make the API harder to misuse so users can be more successful.

BREAKING CHANGE: `Assert::set_cmd` and `Assert::set_stdin` have been
replaced with `Assert::append_context`.

Fixes assert-rs#29
@epage epage closed this as completed in 624a798 Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants