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

Refactor output assertions #24

Merged
merged 2 commits into from
Sep 8, 2017
Merged

Refactor output assertions #24

merged 2 commits into from
Sep 8, 2017

Conversation

killercup
Copy link
Collaborator

@killercup killercup commented Mar 22, 2017

This refactors the output assertions to use a generic type, unit structs, and a new traits. Basically, type system shenanigans!

Previous discussions:

To be answered

@killercup
Copy link
Collaborator Author

r? @colin-kiegel

I've also added you as a collaborator :)

src/output.rs Outdated
if !got.contains(&self.expect) {
bail!(ErrorKind::OutputMismatch(
self.kind.to_string(),
vec!["Foo".to_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.

Placeholder for now. I think we can use error_chain to add the usual "Assert CLI command {cmd} failed:" prefix at the call site.

src/lib.rs Outdated
},
_ => {},
if let Some(ouput_assertion) = self.expect_stdout {
ouput_assertion.execute(&output)?;
Copy link
Collaborator Author

@killercup killercup Mar 22, 2017

Choose a reason for hiding this comment

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

(Do error-chain magic here, cf. #24 (comment))

@colin-kiegel
Copy link
Collaborator

LGTM

  • I like that lib.rs shrinks. :-)
  • I would not switch to enum Precision. I think the possibilities to generalize these assertions would be contains, equals, starts_with, ends_with, does_not_contain, matches_regex, does_not_match_regex etc. pp. and enum Precision wouldn't help there.
  • I can't help you with the error_chain question. I haven't used it much, because it feels to magic for my taste in the sense of I don't completely understand what's going on, and the doc doesn't explain it either. I like quick_error more, because it's simpler and I understand better what I am working with, even if it can't do as much. I also prefer annotated fields over tuple structs, but that's another discussion. :-)

@epage
Copy link
Collaborator

epage commented Sep 7, 2017

Is there an ETA on this?

I'm in a bit of a pickle. I have a new test for my project blocked on needing a new feature in assert_cli (current_dir). Unfortunately, even if I contribute it and it gets released quickly, I can't upgrade because of #31 which is currently blocked on this PR.

@killercup
Copy link
Collaborator Author

@epage sorry, this project hasn't been very actively lately.

…and I have totally forgotten to get this PR landed or even what its state was or if I had some more ideas I wanted to try.

Can you give this branch a try? If it works for you I can merge it right now. And if you were to send a PR to implement #31 you could have a new release quite soon :)

@epage
Copy link
Collaborator

epage commented Sep 8, 2017

I looked over the code. I think the changes are reasonable and look tested.

Of course there is room for improvement

  • Tweaking the output message as @colin-kiegel pointed out
  • Adding more predicates
  • Allowing multiple output assertions
  • Possibly making more fluent. For example command(...).stdout.contains("text").stderr.is("something").stdout.is_not("bleh").execute().unwrap(). This makes it so you don't need to duplicate functions for stdin/stdout. You do still need to duplicate for the not forms unless you are ok with .stdout.not.contains("text").

I do not think any of these ideas should hold up this PR.

@killercup
Copy link
Collaborator Author

Alright, thanks! I'll merge this and we can iterate on this in future PRs!

@killercup killercup merged commit dad1065 into master Sep 8, 2017
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.
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.

3 participants