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

Review/feedback #16

Merged
merged 10 commits into from
Mar 22, 2017
Merged

Review/feedback #16

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5681d0b
cargo.toml keywords
colin-kiegel Mar 21, 2017
ed19a66
README.md
colin-kiegel Mar 21, 2017
cc2d7be
documentation tweaks
colin-kiegel Mar 21, 2017
cdf0d1e
error formatting
colin-kiegel Mar 21, 2017
0ee479c
refactor output assertion
colin-kiegel Mar 21, 2017
fcd20f1
refactor output assertion II
colin-kiegel Mar 21, 2017
1f29f4f
allow matching without asserting the exit status
colin-kiegel Mar 21, 2017
f45db85
refactor `assert_cmd!` macro
colin-kiegel Mar 22, 2017
8a01125
expect success by default + cleanup
colin-kiegel Mar 22, 2017
e307b80
some doc changes
colin-kiegel Mar 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ mod diff;
#[derive(Debug)]
pub struct Assert {
cmd: Vec<String>,
expect_success: bool,
expect_success: Option<bool>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Option<bool> is more expressive than bool. Because None is the I-don't-care-case (e.g. should print "foo", but may fail or succeed).

If you don't like this change I'd suggest to change the docs instead. The call .succeeds() is really all over the place and could then be omitted in almost all cases except the doc-comment for .succeeds() itself, where it should state that this is pure decoration (unless you receive a partial assertion builder from somewhere). :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, you're right, this should be much more explicit. I'm also not sure if "expect success" is a good default, but I think I want to keep it for now. What do you think? Will the most common use case be to check something failed in a certain or that it succeeded?

I'm a big fan of fluent APIs and being explicit, that's why I added .succeeds() everywhere in the docs (and added .and()). It might be a good idea to leave it out in a few places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that succeeds is a reasonable default and symmetry is not always the best design. But

  • you would then need an un-setter .fails_or_succeeds() to express the I-don't-care-case
  • it should be well-documented

People may have very different expectations about this. So either way there might be confusion about the behaviour if it's not well-documented.

That could also be a candidate for a poll in users.rust-lang.org. ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opened #23 but have no immediate plans to implement it

expect_exit_code: Option<i32>,
expect_stdout: Option<OutputAssertion>,
expect_stderr: Option<OutputAssertion>,
Expand Down Expand Up @@ -134,7 +134,7 @@ impl std::default::Default for Assert {
Assert {
cmd: vec!["cargo", "run", "--"]
.into_iter().map(String::from).collect(),
expect_success: true,
expect_success: None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to keep the "expect success", this is the only line that needs to change. (To Some(true))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes and that would fix a documentation bug, I introduced. I wrote everywhere /// Defaults to asserting _successful_ execution. ^^

Adding it to the todo list.

expect_exit_code: None,
expect_stdout: None,
expect_stderr: None,
Expand Down Expand Up @@ -216,8 +216,6 @@ impl Assert {

/// Expect the command to be executed successfully.
///
/// Note: This is already set by default, so you only need this for explicitness.
///
/// # Examples
///
/// ```rust
Expand All @@ -228,7 +226,8 @@ impl Assert {
/// .unwrap();
/// ```
pub fn succeeds(mut self) -> Self {
self.expect_success = true;
self.expect_exit_code = None;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch!

self.expect_success = Some(true);
self
}

Expand All @@ -247,7 +246,7 @@ impl Assert {
/// .unwrap();
/// ```
pub fn fails(mut self) -> Self {
self.expect_success = false;
self.expect_success = Some(false);
self
}

Expand All @@ -263,7 +262,7 @@ impl Assert {
/// .unwrap();
/// ```
pub fn fails_with(mut self, expect_exit_code: i32) -> Self {
self.expect_success = false;
self.expect_success = Some(false);
self.expect_exit_code = Some(expect_exit_code);
self
}
Expand Down Expand Up @@ -366,11 +365,13 @@ impl Assert {
let output = command.output()?;


if self.expect_success != output.status.success() {
bail!(ErrorKind::StatusMismatch(
self.cmd.clone(),
self.expect_success.clone(),
));
if let Some(expect_success) = self.expect_success {
if expect_success != output.status.success() {
bail!(ErrorKind::StatusMismatch(
self.cmd.clone(),
expect_success,
));
}
}

if self.expect_exit_code.is_some() &&
Expand Down