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

Regex matching. #85

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ difference = "2.0"
error-chain = "0.11"
serde_json = "1.0"
environment = "0.1"
regex = "0.2.5"

[build-dependencies]
skeptic = "0.13"
Expand Down
53 changes: 46 additions & 7 deletions src/assert.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
extern crate regex;

use environment::Environment;
use error_chain::ChainedError;
use errors::*;
Expand Down Expand Up @@ -342,9 +344,9 @@ impl Assert {
None => command,
};

let mut spawned = command
.spawn()
.chain_err(|| ErrorKind::SpawnFailed(self.cmd.clone()))?;
let mut spawned = command.spawn().chain_err(
|| ErrorKind::SpawnFailed(self.cmd.clone()),
)?;

if let Some(ref contents) = self.stdin_contents {
spawned
Expand All @@ -360,7 +362,9 @@ impl Assert {
let out = String::from_utf8_lossy(&output.stdout).to_string();
let err = String::from_utf8_lossy(&output.stderr).to_string();
let err: Error = ErrorKind::StatusMismatch(expect_success, out, err).into();
bail!(err.chain_err(|| ErrorKind::AssertionFailed(self.cmd.clone())));
bail!(err.chain_err(
|| ErrorKind::AssertionFailed(self.cmd.clone()),
));
}
}

Expand All @@ -370,14 +374,17 @@ impl Assert {
let err: Error =
ErrorKind::ExitCodeMismatch(self.expect_exit_code, output.status.code(), out, err)
.into();
bail!(err.chain_err(|| ErrorKind::AssertionFailed(self.cmd.clone())));
bail!(err.chain_err(
|| ErrorKind::AssertionFailed(self.cmd.clone()),
));
}

self.expect_output
.iter()
.map(|a| {
a.verify(&output)
.chain_err(|| ErrorKind::AssertionFailed(self.cmd.clone()))
a.verify(&output).chain_err(|| {
ErrorKind::AssertionFailed(self.cmd.clone())
})
})
.collect::<Result<Vec<()>>>()?;

Expand Down Expand Up @@ -445,6 +452,38 @@ impl OutputAssertionBuilder {
self.assertion
}

/// Expect the command to match **however many times** this `output`.
///
/// # Examples
///
/// ```rust
/// extern crate assert_cli;
/// assert_cli::Assert::command(&["echo", "42"])
/// .stdout().matches("[0-9]{2}")
/// .unwrap();
/// ```
pub fn matches(mut self, output: String) -> Assert {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Should we accept a regex?
  2. Should we accept a byte slice and a byte regex?

If we decide on yes, it doesn't mean you have to do them (I don't want to bar for contributions to be perfection) but we need to at least create issues for them and keep them in mind with the design of the API / implementation.

Copy link
Author

Choose a reason for hiding this comment

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

I think yes, at least a regex, a byte regex not so much IMHO, however, I see no problem in setting us up for later by implementing an abstraction layer here.

Copy link
Author

Choose a reason for hiding this comment

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

Also, here the current usage code is more like .matches(String::from("[0-9]") as such, i will probably use str instead of string (just borrow).

Copy link
Collaborator

Choose a reason for hiding this comment

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

For str vs String, you can always make it accept both

let pred = OutputPredicate::new(self.kind, Output::matches(output));
self.assertion.expect_output.push(pred);
self.assertion
}

/// Expect the command to match `nmatches` times this `output`.
///
/// # Examples
///
/// ```rust
/// extern crate assert_cli;
/// assert_cli::Assert::command(&["echo", "42"])
/// .stdout().matches_ntimes("[0-9]{1}", 2)
/// .unwrap();
/// ```
pub fn matches_ntimes(mut self, output: String, nmatches: u32) -> Assert {
let pred = OutputPredicate::new(self.kind, Output::matches_ntimes(output, nmatches));
self.assertion.expect_output.push(pred);
self.assertion
}

/// Expect the command's output to not **contain** `output`.
///
/// # Examples
Expand Down
91 changes: 89 additions & 2 deletions src/output.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
extern crate regex;

use self::errors::*;
pub use self::errors::{Error, ErrorKind};
use diff;
Expand Down Expand Up @@ -63,7 +65,6 @@ impl IsPredicate {
bail!(ErrorKind::BytesMatches(got.to_owned()));
}
}

Ok(())
}

Expand All @@ -83,7 +84,6 @@ impl IsPredicate {
bail!(ErrorKind::StrMatches(got.to_owned()));
}
}

Ok(())
}
}
Expand Down Expand Up @@ -174,11 +174,47 @@ impl fmt::Debug for FnPredicate {
}
}

#[derive(Debug, Clone)]
struct RegexPredicate {
regex: regex::Regex,
times: u32,
}

impl RegexPredicate {
fn verify(&self, got: &[u8]) -> Result<()> {
let conversion = String::from_utf8_lossy(got);
let got = conversion.as_ref();
if self.times == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are your thoughts on using a sentinel value (0) compared to using Option?

Copy link
Author

Choose a reason for hiding this comment

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

This is a good suggestion, It's just some bad habit's take a while to leave :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I ask not to point out bad habits because sometimes the line is blurry. In another project, I have one behavior when a Vec is empty and another when it has items. I'm not using an Option for it because it doesn't feel like it'd jive quite right with the API.

let result = self.regex.is_match(got);
if !result {
bail!(ErrorKind::OutputDoesntMatchRegexExactTimes(
String::from(self.regex.as_str()),
got.into(),
1,
1
));
}
} else {
let regex_matches = self.regex.captures_iter(got).count();
if regex_matches != (self.times as usize) {
bail!(ErrorKind::OutputDoesntMatchRegexExactTimes(
String::from(self.regex.as_str()),
got.into(),
self.times,
regex_matches,
));
}
}
Ok(())
}
}

#[derive(Debug, Clone)]
enum ContentPredicate {
Is(IsPredicate),
Contains(ContainsPredicate),
Fn(FnPredicate),
Regex(RegexPredicate),
}

impl ContentPredicate {
Expand All @@ -187,6 +223,7 @@ impl ContentPredicate {
ContentPredicate::Is(ref pred) => pred.verify(got),
ContentPredicate::Contains(ref pred) => pred.verify(got),
ContentPredicate::Fn(ref pred) => pred.verify(got),
ContentPredicate::Regex(ref pred) => pred.verify(got),
}
}
}
Expand Down Expand Up @@ -238,6 +275,46 @@ impl Output {
Self::new(ContentPredicate::Is(pred))
}

/// Expect the command to **match** this `output`.
///
/// # Examples
///
/// ```rust
/// extern crate assert_cli;
///
/// assert_cli::Assert::command(&["echo"])
/// .with_args(&["42"])
/// .stdout().matches("[0-9]{2}")
/// .unwrap();
/// ```
pub fn matches(output: String) -> Self {
let pred = RegexPredicate {
regex: regex::Regex::new(&output).unwrap(),
times: 0,
};
Self::new(ContentPredicate::Regex(pred))
}

/// Expect the command to **match** this `output` exacly `nmatches` times.
///
/// # Examples
///
/// ```rust
/// extern crate assert_cli;
///
/// assert_cli::Assert::command(&["echo"])
/// .with_args(&["42"])
/// .stdout().matches_ntimes("[0-9]{1}", 2)
/// .unwrap();
/// ```
pub fn matches_ntimes(output: String, nmatches: u32) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(longer term brain storming, feel free to ignore me)

Ideally, in the long run I'd like to do this through a more builder-like approach .matches("pattern").times(10). contains could also implement this. It'd provide a nice way to keep the upfront API "small". We're already talking about doing this for other features.

The interesting challenge is deciding how to implement that.

The nasty "unsafe" option is for Output to have a .times() function. I say "unsafe" because the call is meaningless in some cases (like .is).

Another option is to move away from having people interact with Output and instead have people construct the predicates directly and we'll do an Into<ContentPredicate>. This will end up more like killercup's proposed API.

Copy link
Author

Choose a reason for hiding this comment

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

This is something that can be very useful but out of the scope of this PR, for now, match_ntimes(V, N) is a good enough solution, we might as well open an improvement for later to refactor and abstract into times(N).

let pred = RegexPredicate {
regex: regex::Regex::new(&output).unwrap(),
times: nmatches,
};
Self::new(ContentPredicate::Regex(pred))
}

/// Expect the command's output to not **contain** `output`.
///
/// # Examples
Expand Down Expand Up @@ -386,6 +463,16 @@ mod errors {
description("Output predicate failed")
display("{}\noutput=```{}```", msg, got)
}
/* Adding a single error more makes this break, using the bottom one temporarily
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what way does this break?

Granted, at some point we should probably move beyond error-chain

Copy link
Author

Choose a reason for hiding this comment

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

Out of stack space, this is a known bug in error-chain. I'm gonna look up some docs for this problem.

OutputDoesntMatchRegex(regex: String, got: String) {
description("Expected to regex to match")
display("expected {}\n to match output=```{}```", regex, got)
}
*/
OutputDoesntMatchRegexExactTimes(regex: String, got: String, expected_times: u32, got_times: usize) {
description("Expected to regex to match exact number of times")
display("expected {}\n to match output=```{}``` {} times instead of {} times", regex, got, expected_times, got_times)
}
OutputMismatch(kind: super::OutputKind) {
description("Output was not as expected")
display(
Expand Down
25 changes: 25 additions & 0 deletions tests/cargo.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
extern crate assert_cli;
extern crate regex;

#[test]
fn main_binary() {
Expand All @@ -21,3 +22,27 @@ fn cargo_binary() {
.is("")
.unwrap();
}

#[test]
fn matches_with_regex() {
let re = regex::Regex::new("[0-9]{2}").unwrap();
assert_cli::Assert::main_binary()
.with_env(assert_cli::Environment::inherit().insert("stdout", "42"))
.stdout()
.matches(re)
.stderr()
.is("")
.unwrap();
}

#[test]
fn matches_with_regex_ntimes() {
let re = regex::Regex::new("[0-9]{1}").unwrap();
assert_cli::Assert::main_binary()
.with_env(assert_cli::Environment::inherit().insert("stdout", "42"))
.stdout()
.matches_ntimes(re, 2)
.stderr()
.is("")
.unwrap();
}