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

Find way to make obvious what all extension methods exist in the docs #37

Closed
epage opened this issue Jul 31, 2018 · 7 comments
Closed
Labels
enhancement Improve the expected
Milestone

Comments

@epage
Copy link
Contributor

epage commented Jul 31, 2018

Right now you have to hunt through all of the traits and it makes it hard to see the big picture.

@epage epage added the enhancement Improve the expected label Jul 31, 2018
@epage epage changed the title Find way to make obvious what all extension methods exist Find way to make obvious what all extension methods exist in the docs Jul 31, 2018
@epage epage added this to the 1.0 milestone Aug 2, 2018
@epage
Copy link
Contributor Author

epage commented Oct 5, 2018

#53 is a really good log of an experience in trying to adopt assert_cmd./

  1. Normally our examples are very focused on a single feature. We should provide examples that show all the features
  2. We should have a section that highlights Command / Output and then highlight additional functionality with links to the relevant extension traits
  3. We should identify the different errors and try to consolidate them. I never noticed because they have only been in my test functions where I only unwrap them
  4. Is there a way we can make the transition between different states clearer (command, stdin, assertion, etc)? We ran into a similar problem with assert_cli, see Fluent Output Tests Look off with rustfmt assert_cli#70.

@azriel91 any other thoughts or feedback on the above?

@azriel91
Copy link

azriel91 commented Oct 5, 2018

  1. Yeap, it'll be useful to have bigger examples that cover all of the common cases (for some definition of common)

    If necessary, there could be one mega example at the end for a corner case.

  2. Yeap, I think a comment annotated example will help, something like:

    use std::path::MAIN_SEPARATOR;
    
    #[test]
    fn integration_test() {
        Command::main_binary()
            .unwrap()
    
            // --- environment, args, working dir --- //
            // See <https://doc.rust-lang.org/std/process/struct.Command.html>
            .env("APP_VAR", "value")
            .args(&["--timeout", "2000"])
            .current_dir(
                format!(
                    "{base_dir}{sep}tests",
                    base_dir = env!("CARGO_MANIFEST_DIR"),
                    sep = MAIN_SEPARATOR
                )
            )
    
            // --- stdin --- //
            .with_stdin()     // Returns `StdinCommandBuilder`
            .buffer("exit\n") // Returns `StdinCommand`
                              // Note: `.path("..")` returns `io::Result<StdinCommand>`
            .output()         // Returns `io::Result<std::process::Output>`
            .unwrap()
    
            // --- assertion --- //
            .assert()                // Returns an `Assert`
            .code(predicate::eq(42)) // Assert a specific exit code
            .output(predicate::str::starts_with("Hello")); // These can be chained!
    }

    That StdinCommandBuilder -> StdinBuilder is a gotcha. Also, I guess the extension traits being the first method call is something we should explain up front.

  3. This one might be a preference thing, so probably comes down to some communal vote. I like to avoid unwrap()s because it saves some noise when reading the test.

  4. Perhaps the pre-knowledge of the assert_cli model was a handicap, but knowing the "sections" now helps my understanding of assert_cmd.

    I know the StdinCommandBuilder -> StdinCommand confused me because normally Builders have a build method or similar, which makes it obvious when you switch to the built object, whereas both buffer and path are build methods for StdinCommandBuilder (and to add to the confusion they return different built objects), so it's a gotcha.

    Perhaps taking the Assert part out of the chain in examples would make it clearer. That is, it's still chainable, but breaking the chain could make it explicit like so:

        let output = Command::main_binary()
            .unwrap()
            .with_stdin()     // Returns `StdinCommandBuilder`
            .buffer("exit\n") // Returns `StdinCommand`
                              // Note: `.path("..")` returns `io::Result<StdinCommand>`
            .output()         // Returns `io::Result<std::process::Output>`
            .unwrap();
    
        output.assert()              // Returns an `Assert`
            .code(predicate::eq(42)) // Assert a specific exit code
            .output(predicate::str::starts_with("Hello")); // These can be chained!

    One other thing, the ability to do this felt weird:

    let output = Command::main_binary()
        .unwrap() // `Command`
        .output() // `io::Result<std::process::Output>`
        .unwrap() // `Output`
        .ok();    // `OutputResult` aka `Result<Output, OutputError>`

    That is, you might miss that OutputOkExt is implemented for &mut Command and &mut StdInCommand as well:

    let output = Command::main_binary()
        .unwrap() // `Command`
        .ok();    // `OutputResult` aka `Result<Output, OutputError>`

woosh, that's an essay

@epage
Copy link
Contributor Author

epage commented Oct 6, 2018

Also from #32

  • find ways to make the role of predicates clearer
  • try to find some way to make relevant predicates more visible.

@epage
Copy link
Contributor Author

epage commented Oct 10, 2018

So I've made some documentation tweaks. The linking could be improved; I was feeling lazy.

https://docs.rs/assert_cmd/0.10.1/assert_cmd/

@epage
Copy link
Contributor Author

epage commented Oct 10, 2018

btw I didn't merge the errors.

  • In some respects, this is like 3 crates in one
  • The different types of errors all serve very different roles. At least for OutputError that has an intentional structure for people to access the data.

I like to avoid unwrap()s because it saves some noise when reading the test.

The one thing I realized about this approach is it loses call stack information unless the underlying errors capture a backtrace which nothing in assert_cmd does atm.

@azriel91
Copy link

The one thing I realized about this approach is it loses call stack information unless the underlying errors capture a backtrace which nothing in assert_cmd does atm.

that's okay, I've been keeping an eye on failure's progress, and the std::error::Error trait is slowly moving (rust-lang/rust#53487).

Progress 😄!

@epage
Copy link
Contributor Author

epage commented Oct 26, 2018

I feel like the new docs resolve this.

@epage epage closed this as completed Oct 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

2 participants