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

Show file tree
Hide file tree
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
29 changes: 6 additions & 23 deletions src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,40 +26,23 @@ error_chain! {
got,
)
}
OutputMismatch(cmd: Vec<String>, expected: String, got: String) {
OutputMismatch(output_name: String, cmd: Vec<String>, expected: String, got: String) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could also embed the OutputType enum directly - but then it must be public and would appear in the docs.

description("Output was not as expected")
display(
"{}: `(output of `{}` expected to contain `{:?}`)` (output was: `{:?}`)",
"{}: `({} of `{}` expected to contain `{:?}`)` (output was: `{:?}`)",
ERROR_PREFIX,
output_name,
cmd.join(" "),
expected,
got,
)
}
ExactOutputMismatch(cmd: Vec<String>, diff: String) {
ExactOutputMismatch(output_name: String, cmd: Vec<String>, diff: String) {
description("Output was not as expected")
display(
"{}: `(output of `{}` was not as expected)`\n{}\n",
ERROR_PREFIX,
cmd.join(" "),
diff.trim()
)
}
ErrorOutputMismatch(cmd: Vec<String>, expected: String, got: String) {
description("Stderr output was not as expected")
display(
"{}: `(stderr output of `{}` expected to contain `{:?}`)` (stderr was: `{:?}`)",
ERROR_PREFIX,
cmd.join(" "),
expected,
got,
)
}
ExactErrorOutputMismatch(cmd: Vec<String>, diff: String) {
description("Stderr output was not as expected")
display(
"{}: `(stderr output of `{}` was not as expected)`\n{}\n",
"{}: `({} of `{}` was not as expected)`\n{}\n",
ERROR_PREFIX,
output_name,
cmd.join(" "),
diff.trim()
)
Expand Down
84 changes: 52 additions & 32 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@
extern crate difference;
#[macro_use] extern crate error_chain;

use std::process::Command;
use std::process::{Command, Output};
use std::fmt;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are there any style recommendations how to order use and mod statements?
I regularly have this question - I don't know of any answer. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I think about it, I do it like

  1. things from std
  2. things from external crates
  3. my things

and sort the individual blocks alphanumerically

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, that sounds good. But when pub and mod come into play I find it less clear - I usually end up with

use std
use external
pub use mystuff
use myotherstuff

pub mod mymod
mod myothermod

But I've also seen mod use mixed like here.


use difference::Changeset;

Expand All @@ -101,6 +102,30 @@ struct OutputAssertion {
fuzzy: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the way you split out this struct, it's very neat.

Seeing how the code below with the matches on fuzziness etc. is pretty much duplicated, I'm wondering if maybe it can become a method on this type, like OutputAssertion::compare_with(String)? (It might also be a good idea to make this struct generic over a trait OutputType , but more on that in a comment below.)

At one point I was thinking about introducing an enum Precision { Exact, Fuzzy } instead of using a bool, but didn't go through with it.

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 suggest we start a separate discussion about future refactoring, e.g. in the ticket #20 you created, or yet another general architecture discussion ticket?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I think I'll just open a new PR where I try that design after this one gets merged. Issues that are not about bugs or features but contain a ton of quoted code are kinda hard to keep track of :)

}

#[derive(Debug, Copy, Clone)]
enum OutputType {
StdOut,
StdErr,
}

impl OutputType {
fn select<'a>(&self, o: &'a Output) -> &'a [u8] {
match *self {
OutputType::StdOut => &o.stdout,
OutputType::StdErr => &o.stderr,
}
}
}

impl fmt::Display for OutputType {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
OutputType::StdOut => write!(f, "stdout"),
OutputType::StdErr => write!(f, "stderr"),
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing this – an enum with two impl blocks – we could also use two unit structs and introduce a trait OutputType which both implement, that requires a Display impl and has a select(&Output) -> &[u8] method.

This way (as alluded to above) we can add a field kind: T to what is then OutputAssertion<T> and change the last two fields in Assert to Option<OutputAssertion<Stdout>> and Option<OutputAssertion<Stderr>> respectively.

I'm not sure this is a truly nicer design; I got the idea from wanting to encapsulate the whole output comparison business. Maybe it's totally over engineered 😄

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 generally tend to prefer traits too. Hm, maybe we keep that idea in mind for the 'general architecture' discussion?


impl std::default::Default for Assert {
/// Construct an assert using `cargo run --` as command.
///
Expand Down Expand Up @@ -357,57 +382,52 @@ impl Assert {
));
}

let stdout = String::from_utf8_lossy(&output.stdout);
match self.expect_stdout {
self.assert_output(OutputType::StdOut, &output)?;
self.assert_output(OutputType::StdErr, &output)?;

Ok(())
}

/// Perform the appropriate output assertion.
fn assert_output(&self, output_type: OutputType, output: &Output) -> Result<()> {
let observed = String::from_utf8_lossy(output_type.select(output));
match *self.expect_output(output_type) {
Some(OutputAssertion {
expect: ref expected_output,
fuzzy: true,
}) if !stdout.contains(expected_output) => {
}) if !observed.contains(expected_output) => {
bail!(ErrorKind::OutputMismatch(
output_type.to_string(),
self.cmd.clone(),
expected_output.clone(),
stdout.into(),
observed.into(),
));
},
Some(OutputAssertion {
expect: ref expected_output,
fuzzy: false,
}) => {
let differences = Changeset::new(expected_output.trim(), stdout.trim(), "\n");
let differences = Changeset::new(expected_output.trim(), observed.trim(), "\n");
if differences.distance > 0 {
let nice_diff = diff::render(&differences)?;
bail!(ErrorKind::ExactOutputMismatch(self.cmd.clone(), nice_diff));
bail!(ErrorKind::ExactOutputMismatch(
output_type.to_string(),
self.cmd.clone(),
nice_diff
));
}
},
_ => {},
}
Ok(())
}

let stderr = String::from_utf8_lossy(&output.stderr);
match self.expect_stderr {
Some(OutputAssertion {
expect: ref expected_output,
fuzzy: true,
}) if !stderr.contains(expected_output) => {
bail!(ErrorKind::ErrorOutputMismatch(
self.cmd.clone(),
expected_output.clone(),
stderr.into(),
));
},
Some(OutputAssertion {
expect: ref expected_output,
fuzzy: false,
}) => {
let differences = Changeset::new(expected_output.trim(), stderr.trim(), "\n");
if differences.distance > 0 {
let nice_diff = diff::render(&differences)?;
bail!(ErrorKind::ExactErrorOutputMismatch(self.cmd.clone(),nice_diff));
}
},
_ => {},
/// Return a reference to the appropriate output assertion.
fn expect_output(&self, output_type: OutputType) -> &Option<OutputAssertion> {
match output_type {
OutputType::StdOut => &self.expect_stdout,
OutputType::StdErr => &self.expect_stderr,
}

Ok(())
}

/// Execute the command, check the assertions, and panic when they fail.
Expand Down