Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What is needed for 1.0? #74

Closed
epage opened this issue Mar 22, 2019 · 20 comments
Closed

What is needed for 1.0? #74

epage opened this issue Mar 22, 2019 · 20 comments
Labels
question Uncertainty is involved

Comments

@epage
Copy link
Contributor

epage commented Mar 22, 2019

Collecting feedback or assert_fs. Please post even if you think it might not be needed for 1.0 so we can try to get the big picture of uses, challenges, etc. We're also looking at including experience reports in the 1.0 announcement blog post.

Preferably, please update to the latest version first but don't let upgrading stop you from providing feedback

To help spur feedback, consider these questions

  • Why do you use this crate?
  • Why don't you use it more?
  • How does this crate fit into your testing strategy?
  • Is there anything that feels out of place / not rust-like?
  • Are there common patterns you implement on top of this crate (e.g. fixture setup or assertions we don't support)?

Also, if your programs deals with the file system, please provide feedback on assert_fs. Even if you don't use it, I want to know why.


Summary

Areas to improve

Successes

fitzgen

assert_cmd is just such a pleasure to use every single time, I fall in love all over again
bravo bravo WG-cli

@passcod

Running commands and dealing with output can be complex in many many ways, so assert_cmd smoothing that is excellent, very much welcome, and improves ergonomics significantly.

@volks73

I have used [assert_cmd] in other projects and I am extremely pleased with it

@coreyja

[assert_cmd] pretty much IS my testing strategy so far, though my app under test is pretty small.

This library has made it really easy to add some test coverage to my project, even when I am just learning how to write Rust!

@epage epage added the question Uncertainty is involved label Mar 22, 2019
@epage
Copy link
Contributor Author

epage commented Mar 22, 2019

#63 already includes a good heap of feedback

@epage epage pinned this issue Mar 22, 2019
@epage
Copy link
Contributor Author

epage commented Mar 23, 2019

@coreyja, @azriel91, @AnderEnder, @dbrgn, @fernandobatels, @mssun, @gnzlbg, @quininer, @zetok: you all have participated in some issues. Could you provide input on this?

@epage

This comment has been minimized.

@passcod

This comment has been minimized.

@epage

This comment has been minimized.

@epage

This comment has been minimized.

@coreyja
Copy link

coreyja commented Mar 23, 2019

I think I am running into #51 right now in my testing. Here is the code I am working with

let mut cmd = Command::cargo_bin("devicon-lookup").unwrap();
cmd
  .with_stdin()
  .buffer("test.rs".blue().to_string())
  .assert()
  .stdout(format!(" {}\n", "test.rs".blue()).as_str());

Color commands from https://github.com/mackwic/colored

I am also fairly new to Rust, so I am still working on getting my head how lifetimes work, and I also can't comment on how Rust-y the code is!

To answer some of your questions:

How does this crate fit into your testing strategy?

It pretty much IS my testing strategy so far, though my app under test is pretty small. See https://github.com/coreyja/devicon-lookup/blob/master/tests/integration.rs

This library has made it really easy to add some test coverage to my project, even when I am just learning how to write Rust!

@DrSensor
Copy link

How does this crate fit into your testing strategy?

Recently I use assert_cmd alongside with rexpect in my REPL program and I love it until I change the implementation to use linefeed.

Are there common patterns you implement on top of this crate (e.g. fixture setup or assertions we don't support)?

Might be a bit out of topic but I would love it if assert_cli has some functionality for assertion in scenario like REPL, Prompt, or anything that involve stdin.
While assert_cmd more focus on CLI that only use stdout/stderr (including sanitize the output a.k.a removing terminal code for color, blinking text, empty line, and others)

@epage

This comment has been minimized.

@epage
Copy link
Contributor Author

epage commented Mar 25, 2019

@coreyja Thanks for the feedback and the comments!

Could you provide the error you are running into? Looking at the code, I actually wonder if you are running into #73.

@epage
Copy link
Contributor Author

epage commented Mar 25, 2019

@DrSensor

Might be a bit out of topic but I would love it if assert_cli has some functionality for assertion in scenario like REPL, Prompt, or anything that involve stdin.

I've not dealt with a REPL yet, so I'm a bit lacking on knowing what kinds of challenges there are and what could be done to help. In what ways are you thinking that the experience could be improved over just using rexpect?

While assert_cmd more focus on CLI that only use stdout/stderr (including sanitize the output a.k.a removing terminal code for color, blinking text, empty line, and others)

Hmm, currently we don't have output sanitizing and I know there are people who want to explore testing things like color, so I don't think we should cover it universally. However, I think it'd be a great idea for us to have a predicate modifier that could strip them, like our modifier for normalizing newlines. I've created assert-rs/predicates-rs#76 for this.

@vincentdephily
Copy link

As food for thoughts, here's how I'm using assert_cmd.

The guiding idea is that I build a struct describing programs to run/delay/kill, a set of input files, and a set of post-run checks that operate on created files (including the program's stdout/stderr). I can run() on that struct once it is set up, and it takes care of setting up the TempDir, scheduling program start, and running the checks.

/// Main struct described above. It's got builder-pattern methods,
/// and a `run` method that will do all the work and `assert` upon failure.
pub struct Exec {
    cmds: Vec<Cmd>,
    timeout: Duration,
    files: Vec<FileIn>,
    checks: Vec<FilePredicate>,
}

pub struct Cmd {
    /// Program to run.
    bin: Command,
    /// Command name for `after()`, std/err filname prefix, and logs.
    name: String,
    /// Expected exit status. Some(0)=success, Some(n)=failure, None=timeout.
    exit: Option<i32>,
    /// Fail if the cmd exits too early.
    mintime: Duration,
    /// Fail if the cmd run for too long.
    maxtime: Duration,
    /// Current state.
    state: CmdState,
    /// List of signals to send to the process after startup.
    signals: Vec<(CmdCond, c_int)>,
}

enum CmdState {
    Wait(CmdCond),
    Started(Child, Instant),
    Done,
}

pub enum CmdCond {
    /// Immediately true
    None,
    /// Duration elapsed
    Delay(Duration),
    /// Other Cmd exited
    Cmd(String),
    /// File-based predicate
    Predicate(FilePredicate),
}

pub struct FilePredicate {
    /// Desciption for assert-logging purpose.
    desc: String,
    /// Which file to operate on.
    file: String,
    /// Closure that tests the content of the file.
    pred: Box<dyn Fn(&str) -> bool>,
}

pub enum FileIn {
    FromFs(&'static str, &'static str),
    Bin(&'static str, Vec<u8>),
}

With that in place, I have a pretty powerful way to write a unittest, using my crate's main binary and a few ancillary processes:

    // As basic as it gets.
    exec().cmd(Cmd::any("echo", "echo", "-n a")).check("echo a", "echo.out", |s| s == "a").run();
    // File from hardcoded data.
    exec().inbytes("a", "a")
          .cmd(Cmd::any("cat", "cat", "a"))
          .check("cat a", "cat.out", |s| s == "a")
          .run();
    // File from file in test/ directory.
    exec().infile("j", "input.basic.json")
          .cmd(Cmd::any("cat", "cat", "j"))
          .check("cat j", "cat.out", |s| s.starts_with("{"))
          .run();
    // run sequentially
    let start = Instant::now();
    exec().cmd(Cmd::any("s1", "sleep", "0.3"))
          .cmd(Cmd::any("s2", "sleep", "0.3").after("s1"))
          .cmd(Cmd::any("s3", "sleep", "0.3").after("s2"))
          .cmd(Cmd::any("cat", "cat", "s1.out s2.out s3.out").after("s3"))
          .run();
    assert!(Instant::now() - start > Duration::from_millis(900));
    // delayed start
    exec().cmd(Cmd::any("0", "cat", "1.out 2.out").after(20))
          .cmd(Cmd::any("1", "echo", "a"))
          .cmd(Cmd::any("2", "echo", "b"))
          .check("cat", "0.out", |s| s == "a\nb\n")
          .run();
    // timeout
    exec().cmd(Cmd::any("s", "sleep", "1").maxtime(100).exit(None)).run();
    exec().cmd(Cmd::any("s", "sleep", "0.05").mintime(50).maxtime(70)).run();
    // Multiple signals, ordered by Instant.
    exec().cmd(Cmd::any("s", "sleep", "1").signal(SIGINT, 70) // Added first but triggers last
                                          .signal(SIGCONT, 10) // Triggers first but doesn't terminate
                                          .signal(SIGTERM, 30) // Actual terminator
                                          .exit(SIGTERM))
          .run();
    // Signal after another cmd exit.
    exec().cmd(Cmd::any("s1", "sleep", "1").signal(SIGINT, "s2").maxtime(50).exit(SIGINT))
          .cmd(Cmd::any("s2", "sleep", "0.01"))
          .run();
    // Signal after file content matches
    exec().cmd(Cmd::any("s1", "sleep", "1").signal(SIGINT, ("s2.out", "4"))
                                           .maxtime(100)
                                           .exit(SIGINT))
          .cmd(Cmd::bash("s2", "for i in $(seq 10);do echo $i;sleep 0.01;done"))
          .timeout(1000)
          .run();
    // Main binary
    exec().cmd(Cmd::main("m", "-V")).check("progname", "0.out", |s| s.starts_with("mybin"));
    // Set/unset env
    exec().cmd(Cmd::any("env", "env", "").env("foo", "bar")
                                         .env("PATH", &format!("/ut:{}", env::var("PATH").unwrap()))
                                         .env_remove("HOME"))
          .check("added_foo", "env.out", |s| s.lines().any(|l| l == "foo=bar"))
          .check("changed_path", "env.out", |s| s.lines().any(|l| l.starts_with("PATH=/ut:")))
          .check("removed_home", "env.out", |s| !s.lines().any(|l| l.starts_with("HOME=")))
          .run();

If there's enough interest, I could try to get this cleared for upstreaming. Currently there are a few extensions and API decisions that are specific to my project, and the code is a bit ad-hoc in places.

@epage
Copy link
Contributor Author

epage commented Nov 29, 2019

Thanks for sharing this!

There is a lot here, so let's see what a break down looks like:

So this crate started as assert_cli which is closer to what you describe for an API. When working on that, we decided to:

  • Focus on lower level primitives rather than an end-to-end CLI tester
  • Support alternative APIs like duct

This is what led to the current extension-trait API. The challenge with this API design is carrying state with it because the Command API is &mut builder which is causing problems with stdin, (see #73).

So options for stdin, timeout, and signals:

  • Find a way to make the existing API work
  • Create our own Command that supports these features and uses the extension traits
  • Switch back to a Command wrapper

Resolving this is going to be the next thing I focus on.

Now for command orchestration. That seems like it is going beyond the planned scope for assert_cmd. I suspect that might be more appropriate for assert_cli which we plan to turn into a higher level test crate, see assert-rs/assert_cli#41. Maybe your work could serve as the foundation for that?

I would be curious to know what is the use case that leads you to need command orchestration so I can better evaluate whether what I just said is right or not :).

FilePredicate

Is there a reason you use this rather than predicates? predicates seems like it offers more functionality.

@epage
Copy link
Contributor Author

epage commented Dec 5, 2019

@vincentdephily where can I find your code so I can steal parts of it :)

Just posted 0.12 with a Command wrapper to make things easier / fix stdin's API.

@vincentdephily
Copy link

@epage Currently the code isn't public. I've redacted the file to keep only the relevant parts, and will get that cleared for release.

I could probably have used predicates and other existing APIs, but either those didn't exist at the time or I didn't know about them. This is fairly pragmatic code, just gets the job done, only aimed at public release as a long-term maybe ;)

A wrapper API is easier to design, that's what I went after with my Cmd/Exec duo. Note that whatever we provide, the end-user will need to extend.

I won't have time for a PR, but should be able to help with the design discussions and dogfooding.

@epage
Copy link
Contributor Author

epage commented Dec 9, 2019

I can understand the release issues and the time. For now, I'm interested in seeing how you handled timeouts. If you have anything to add to #10, I'd appreciate it.

I've started down the path of providing a wrapper type. This cleaned up stdin (though I hate my function names).

@vincentdephily
Copy link

I've done a quick filtering out of the private code and got the thumbs up to share https://gist.github.com/vincentdephily/592546c41ff9713adff234e5535aa6d4 which you can treat as public domain.

I've added a few UPSTREAMING: comments, as you probably don't want to take it all as-is. Hope that helps.

@epage
Copy link
Contributor Author

epage commented Mar 27, 2020

Created a new issue for vincentdephily's fancy stuff. I feel we're ready to close this one out.

@epage epage closed this as completed Mar 27, 2020
@epage epage unpinned this issue Mar 27, 2020
@brycx
Copy link

brycx commented Jan 31, 2022

I know this is some time ago, but I just stumbled across this and felt like it was worth mentioning.

While I don't have a problem with this, and don't know how you've written to other people that provided feedback over email, at least I wasn't told that my feedback would be made public. I think this would be the correct thing to do in these situations.

@epage
Copy link
Contributor Author

epage commented Jan 31, 2022

Sorry, I hadn't considered that at the time but that is the right thing to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Uncertainty is involved
Projects
None yet
Development

No branches or pull requests

6 participants