Skip to content
This repository was archived by the owner on Dec 29, 2021. It is now read-only.

Reuse output assertions between stdout/stderr #38

Merged
merged 4 commits into from
Sep 23, 2017

Conversation

epage
Copy link
Collaborator

@epage epage commented Sep 14, 2017

Reusing output assertions makes it easier to add new ones in the future.

I feel the new naming scheme this provides makes intent of the API clearer as well (stdout().contains vs prints).

This is done by creating an OutputAssertionBuilder that Assertion delegates to for creating output assertions, passing in an enum of which stream to read from. This unfortunately meant dropping the cool type tricks that were introduced in #24. This also required merging the storage of stdout/stderr assertions. To accommodate this, output assertions are now appended which might be useful on its own.

BREAKING CHANGE: Setting a new output assertion will not overwrite the
previous one.
This simplifies the logic for generically adding new assertions at the
cost of an match statement for each output assertion executed.
BREAKING CHANGE:

- `.prints` -> `.stdout().contains`
- `.prints_exactly` -> `.stdout().is`
- `.prints_error` -> `.stderr().contains`
- `.prints_error_exactly` -> `.stderr().is`
@epage epage requested a review from killercup September 14, 2017 19:43
@epage
Copy link
Collaborator Author

epage commented Sep 14, 2017

From #36

I can see why you want this, and I think this is a good idea. I don't know if the API is obvious enough, though. People might be expecting the usual "builder" behavior of the second call replacing the effects of the first. In the very least, can you add some tests? :)

Understandable. #35 was an intermediate step and so my full intent of where I was going with this wasn't clear. Now that I have the rest of the changes (though possibly controversial?), it should help. Feel free to tell me you don't like bits and I'll pull out the commits you do like and get a PR up for those.

With the full, new API, any thoughts on communicating that these append instead of replace? My original thought was more along the lines of a general fluent API rather than specifically a fluent builder and didn't consider the possible confusion that this might provide.

@epage
Copy link
Collaborator Author

epage commented Sep 22, 2017

@killercup this now has documentation/test for the new multiple-output-assertions behavior.

While I admit src/output.rs didn't end up as clean as I expected, I feel this is ready to go in now and src/output.rs can be refactored as more predicates are added.

  1. So my main question is if you are comfortable with the new output API:
#[macro_use] extern crate assert_cli;
fn main() {
    assert_cmd!(echo "Hello world! The ansswer is 42.")
        .stdout().contains("Hello world")
        .stdout().contains("42")
        .stderr().is("")
        .unwrap();
}
  1. Could you do a new release once this is in?

Thanks!

Copy link
Collaborator

@killercup killercup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! And sorry it took me so long to review this.

I'm not entirely sure I like the two-step-dance with .the_output_pipe().matcher_method(), but I don't have better idea that is quite as readable.

So! Let's land this already!

bors r+

&o.stderr
pub fn map_err(self, e: Error, cmd: &[String]) -> super::errors::Error {
match self {
OutputKind::StdOut => super::errors::ErrorKind::StdoutMismatch(cmd.to_vec(), e).into(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use super::errors::ErrorKind::*; but doesn't really matter :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the idea. I was unsure what could be used to simplify it without mixing up the file's error chain with the crate's error chain.

bors bot added a commit that referenced this pull request Sep 23, 2017
38: Reuse output assertions between stdout/stderr r=killercup a=epage

Reusing output assertions makes it easier to add new ones in the future.

I feel the new naming scheme this provides makes intent of the API clearer as well (`stdout().contains` vs `prints`).

This is done by creating an `OutputAssertionBuilder` that `Assertion` delegates to for creating output assertions, passing in an enum of which stream to read from.   This unfortunately meant dropping the cool type tricks that were introduced in #24.  This also required merging the storage of stdout/stderr assertions.  To accommodate this, output assertions are now appended which might be useful on its own.
@killercup
Copy link
Collaborator

And for making a release: I've added you as owner to the crate on crates.io :) Feel free to make a PR with a version bump/changelog and release this as 0.5! :)

@bors
Copy link
Contributor

bors bot commented Sep 23, 2017

Build succeeded

@bors bors bot merged commit 64c766b into assert-rs:master Sep 23, 2017
@epage epage deleted the refactor/output branch September 23, 2017 15:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants