Review/feedback #16

colin-kiegel · 2017-03-21T21:54:35Z

Ok, here is my feedback. I bundled it into different topics, so it is easier for you to opt-out of individual suggestions. :-)

The later commits are those which might be more controversial. Only the last two change the API surface.

I'll leave further comments here and there. :-)

TODO

const instead of static Review/feedback #16 (diff)
expect_success: Some(true), Review/feedback #16 (comment)
- introduce an un-setter .fails_or_succeeds() to express the I-don't-care-case
- double check that this prominently well-documented

- make the header less cluttered - add generic docs badge - remove coveralls badge (its not up-to-date) - print() -> prints()

- note: std has `.` at the end of one-line doc-strings.

- try to mimic `assert_eq!` - close #14: try to avoid multiline panics

.. just a suggestion, since stdout and stderr are somewhat redundant.

colin-kiegel · 2017-03-21T21:58:09Z

src/errors.rs

@@ -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) {


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

colin-kiegel · 2017-03-21T21:59:25Z

src/lib.rs

@@ -65,7 +76,8 @@
 extern crate difference;
 #[macro_use] extern crate error_chain;

-use std::process::Command;
+use std::process::{Command, Output};
+use std::fmt;


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

When I think about it, I do it like

things from std

things from external crates

my things

and sort the individual blocks alphanumerically

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.

colin-kiegel · 2017-03-21T22:03:17Z

src/lib.rs

 #[derive(Debug)]
 pub struct Assert {
    cmd: Vec<String>,
-    expect_success: bool,
+    expect_success: Option<bool>,


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). :-)

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.

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. ;-)

Opened #23 but have no immediate plans to implement it

colin-kiegel · 2017-03-21T22:42:06Z

README.md

-If you want to check for the program's output, you can use `print` or
-`print_exactly`:
+If you want to match the program's output _exactly_, you can use
+`prints_exactly`:


Just a random comment.

When first poking around with this crate I found the semantics of prints a bit surprising. I expected a strict equality assertion. Then I learned that there is prints_exactly.

It's very likely a matter of taste and I might even change my mind (I often do), but I think I would prefer the strict assertion over the fuzzy assertion. I tried hard to think of a different naming, where the strict variant would have the shorter name, but I couldn't find something nice.

I tried hard to think of a different naming, where the strict variant would have the shorter name, but I couldn't find something nice.

Me neither! That's one of the main reason I made the exact method's name longer.

The previous version only had exact matching, and it was difficult to get right, because you had to include every new line (Maybe with carriage return on Windows? Who knows!) and control character.

This reminds of of another thing I want to support: Multiple fuzzy checks. I'll open an issue for that in a minute.

killercup

Wow, Colin, thank you so much, this is awesome! I chose to mark this as Comment instead of Approve as I left, well, a gazillion inline comments 😆

killercup · 2017-03-21T22:25:55Z

README.md


 ```diff
 -1337
 +92
 ```

+More detailed information is available in the [documentation]. :-)


This link doesnt work because the link ref definition starts with an uppercase D

I don't know if it's in the markdown specs, but github and my atom IDEs markdown preview treat it case-insensitive:
https://github.com/colin-kiegel/assert_cli/blob/1f29f4f25978bedf0bf476f5a27bedb0ea6a83e5/README.md

Yep, you win: http://spec.commonmark.org/0.27/#example-167

killercup · 2017-03-21T22:27:21Z

src/errors.rs

@@ -1,3 +1,5 @@
+static ERROR_PREFIX: &'static str = "CLI assertion failed";


Nice. Why static and not const?

Oops, I tend to forget that there are consts too. ^^

I'll link it in a todo list, at the top comment. :-)

killercup · 2017-03-21T22:30:27Z

src/lib.rs

@@ -65,7 +76,8 @@
 extern crate difference;
 #[macro_use] extern crate error_chain;

-use std::process::Command;
+use std::process::{Command, Output};
+use std::fmt;


When I think about it, I do it like

things from std

things from external crates

my things

and sort the individual blocks alphanumerically

killercup · 2017-03-21T22:30:47Z

src/lib.rs

-//! (Make sure to include the crate as `#[macro_use] extern crate assert_cli;`!)
+//! Don't forget to import the crate with `#[macro_use]`. ;-)
+//!
+//! ## Don't Panic!


killercup · 2017-03-21T22:39:58Z

src/lib.rs

+#[derive(Debug)]
+struct OutputAssertion {
+    expect: String,
+    fuzzy: bool,


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.

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?

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 :)

killercup · 2017-03-21T22:46:11Z

src/lib.rs

+            OutputType::StdErr => write!(f, "stderr"),
+        }
+    }
+}


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 😄

I generally tend to prefer traits too. Hm, maybe we keep that idea in mind for the 'general architecture' discussion?

killercup · 2017-03-21T22:49:49Z

src/lib.rs

 #[derive(Debug)]
 pub struct Assert {
    cmd: Vec<String>,
-    expect_success: bool,
+    expect_success: Option<bool>,


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.

killercup · 2017-03-21T22:50:33Z

src/lib.rs

@@ -228,7 +226,8 @@ impl Assert {
    ///     .unwrap();
    /// ```
    pub fn succeeds(mut self) -> Self {
-        self.expect_success = true;
+        self.expect_exit_code = None;


Good catch!

killercup · 2017-03-21T22:51:06Z

src/lib.rs

@@ -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,


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

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

Adding it to the todo list.

killercup · 2017-03-21T22:57:04Z

README.md

-If you want to check for the program's output, you can use `print` or
-`print_exactly`:
+If you want to match the program's output _exactly_, you can use
+`prints_exactly`:


I tried hard to think of a different naming, where the strict variant would have the shorter name, but I couldn't find something nice.

Me neither! That's one of the main reason I made the exact method's name longer.

The previous version only had exact matching, and it was difficult to get right, because you had to include every new line (Maybe with carriage return on Windows? Who knows!) and control character.

This reminds of of another thing I want to support: Multiple fuzzy checks. I'll open an issue for that in a minute.

- fix #22 - fix #15

killercup · 2017-03-22T22:26:04Z

@colin-kiegel, I'll merge this with two additional commits, to use const, and to, for now, default to success.

Ohh, as I was writing this, I saw your macro refactor commit 😅 I'll not include that commit for now, before deciding what I really want to do here (see #22 (comment)). This probably means Github will not mark this as merged.

colin-kiegel · 2017-03-22T22:27:51Z

@killercup I had all the todos ready and quite a bunch of documentation changes. But I stopped, because of the macro issue.

colin-kiegel · 2017-03-22T22:28:58Z

have you worked in parallel?

PS: seems that only clippy fails the travis build https://travis-ci.org/killercup/assert_cli/builds/214033920

colin-kiegel · 2017-03-22T22:30:07Z

PPS: If you give me just a couple of minutes, then I will add the final commit closing the todos (except for the unsetter idea). :-)

killercup · 2017-03-22T22:33:43Z

(Re CI: Clippy seems to have a cache/rustup problem)

Sorry, didn't know you were working on this at this moment! Can you make a new PR for the macro, but the other commits here? I have no problem force-pushing master.

colin-kiegel · 2017-03-22T22:36:20Z

ok :-)

killercup · 2017-03-22T22:37:15Z

Actually, wait, keep the macro changes in if it's any kind of hassle to pull them out. It's strictly better than before.

colin-kiegel · 2017-03-22T22:38:25Z

ok, that's what I was thinking. :-)

I'll split my next commits in two parts - the todos above and some doc changes I'd like to suggest, ok?

killercup · 2017-03-22T22:38:52Z

Sounds good. I'll probably merge all of them right aways 😄

PS: I set master to a commit right before my merge and added another commit on top for travis. I hope it's not too confusing.

colin-kiegel · 2017-03-22T22:48:27Z

btw. is the typo in non-exisiting-file intentional? I guess so. If not, I can correct it. :-)

killercup · 2017-03-22T22:49:59Z

Haha, that was not on purpose, no :D And like all good typos it got copypasted all over the place! Feel free to correct it

- explain asserting the exit codes - better reflect the fact that `success` is expected by default - tip/hint about the macro limitation

colin-kiegel · 2017-03-22T22:59:20Z

ok done :-)

killercup · 2017-03-22T23:03:18Z

Very nice! Thanks for making these changes so quickly! Now, let's get this merged already!

… Oh Clippy, what now?

error: you should put `assert_cli` between ticks in the documentation
  --> src/macros.rs:74:76
   |
74 | /// information about whitespaces, to address https://github.com/killercup/assert_cli/issues/22.
   |                                                                            ^^^^^^^^^^
   |
   = note: #[deny(doc_markdown)] implied by #[deny(warnings)]

I'll make it a nice link and merge manually.

killercup · 2017-03-22T23:05:19Z

🎉

colin-kiegel · 2017-03-22T23:05:22Z

Thank you, too. I had a lot of fun. :-)

colin-kiegel added 7 commits March 21, 2017 21:17

cargo.toml keywords

5681d0b

README.md

ed19a66

- make the header less cluttered - add generic docs badge - remove coveralls badge (its not up-to-date) - print() -> prints()

documentation tweaks

cc2d7be

- note: std has `.` at the end of one-line doc-strings.

error formatting

cdf0d1e

- try to mimic `assert_eq!` - close #14: try to avoid multiline panics

refactor output assertion

0ee479c

.. just a suggestion, since stdout and stderr are somewhat redundant.

refactor output assertion II

fcd20f1

allow matching without asserting the exit status

1f29f4f

colin-kiegel commented Mar 21, 2017

View reviewed changes

killercup reviewed Mar 21, 2017

View reviewed changes

killercup mentioned this pull request Mar 21, 2017

Multiple output matchers #20

Closed

colin-kiegel mentioned this pull request Mar 22, 2017

assert_cmd! unexpected behaviour, which might eat your laundry #22

Closed

killercup mentioned this pull request Mar 22, 2017

The I-don't-care-about-the-status feature #23

Closed

refactor assert_cmd! macro

f45db85

- fix #22 - fix #15

killercup force-pushed the master branch from d34c9a8 to 05a88f2 Compare March 22, 2017 22:42

colin-kiegel added 2 commits March 22, 2017 23:55

expect success by default + cleanup

8a01125

some doc changes

e307b80

- explain asserting the exit codes - better reflect the fact that `success` is expected by default - tip/hint about the macro limitation

killercup merged commit e307b80 into assert-rs:master Mar 22, 2017

colin-kiegel deleted the review/feedback branch March 22, 2017 23:05

killercup mentioned this pull request Mar 22, 2017

Refactor output assertions #24

Merged

3 tasks

		@@ -1,3 +1,5 @@
		static ERROR_PREFIX: &'static str = "CLI assertion failed";

Review/feedback #16

Review/feedback #16

Conversation

colin-kiegel commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

killercup commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

killercup commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

killercup commented Mar 22, 2017 • edited Loading

colin-kiegel commented Mar 22, 2017

killercup commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

killercup commented Mar 22, 2017

killercup commented Mar 22, 2017

colin-kiegel commented Mar 22, 2017

colin-kiegel commented Mar 21, 2017 •

edited

Loading

killercup commented Mar 22, 2017 •

edited

Loading