tests/util: Enhance CmdResult

[Joining7943]

I would love to enhance the existing functionality of CmdResult by a few points.

Use ExitStatus instead of code and success

We get the ExitStatus from the wait methods in UChild, so no extraction of code and success is required to initialize the CmdResult. On unix platforms we could provide a method in CmdResult to assert signals or whatever is available in std::os::unix::ExitStatusExt.

Rename stderr_is to stderr_trimmed_is and provide a stderr_is method which asserts the stderr untrimmed

This method gave a hard time once, until I found out that stderr is trimmed. The trimming should be more obvious.

Provide stdout_apply, stdout_str_apply, stderr_apply and stderr_str_apply methods which apply a function to the stdout (stderr) and return a CmdResult

Such method calls could for example look like

ucommand.run().stdout_str_apply(str::trim).stdout_only("hello world");
ucommand.run().stdout_apply(|s| &s[0..5]).stdout_only("hello");

This has the advantage, that we can stay in the CmdResult chain, instead of breaking out with let stdout = ucommand.run().stdout_str(), then doing something with stdout and then asserting this stdout manually.

[tertsdiepraam]

The first two sound good! The last one I'm not sure about for two reasons (but happy to be convinced otherwise):

1. Breaking out of the assertion chain is not necessarily a big issue and might be more clear than what you're suggesting.

2. What are the lifetimes of the input and output of the function? Is it owned values for do they have the lifetime of the result? How do we store those behind the scenes?

[Joining7943]

Regarding your concerns about the lifetimes etc. I found it easier to quickly implement a working version, which should be self-explaining, in a draft pr #4261. Concerning CmdResult, it's a minimal invasive implementation. There are other ways around providing the same result.

I skimmed through the tests and refactored two of them to demonstrate the stdout_str_apply, stderr_str_apply methods. The other methods work the same. It's maybe a matter of personal preference but I would choose

#[test]
fn test_uname_processor() {
    new_ucmd!()
        .arg("-p")
        .succeeds()
        .stdout_str_apply(str::trim_end)
        .stdout_only("unknown");
}

over

#[test]
fn test_uname_processor() {
    let result = new_ucmd!().arg("-p").succeeds();
    assert_eq!(result.stdout_str().trim_end(), "unknown");
}

Having such methods is a convenient way to edit stdout or stderr in any way before running the assertions in CmdResult.

[tertsdiepraam]

I see, it's owned values all the way through with a bit of Into magic. That looks good to me. Your examples are more convenient but in my opinion less clear. What I don't like is that the two calls are disjointed and that we have to mutate the state. It's not really a big issue though. What do you think of this API instead:

new_ucmd!()
    .arg("-i")
    .succeeds()
    .stdout_check(|s| {
        s.trim_end() == "somestring"
    });

[Joining7943]

that we have to mutate the state.

To be precise, a new state is created but the old state isn't mutated. This is maybe relevant because the old CmdResult is still around, and theoretically something like this would be possible (this example doesn't make too much sense):

let result = ucommand.run();
result.stdout_str_apply(str::trim).stdout_is("unknown");
result.stdout_str_apply(str::trim_end).stdout_is("    unknown");

I like your predicate method! I would like to have both methods for full power and flexibility, perhaps modifying your suggestion a bit to stdout_str_check, but also providing strout_check for bytes.

[tertsdiepraam]

a new state is created but the old state isn't mutated.

Yeah that's true and indeed a bit better, but the fact remains that the order of assertions now matters.

[Joining7943]

but the fact remains that the order of assertions now matters.

I'm not sure I understand. Can you please give an example in which the order of the assertions matters?

[tertsdiepraam]

Ah sorry for being vague, I just mean this:

ucommand
    .run()
    .stdout_only("hello world") // fails
    .stdout_str_apply(str::trim)
    .stdout_only("hello world"); // works

[Joining7943]

Ok, I see, but what's unclear or wrong about it?

[tertsdiepraam]

The assertions are generally independent of each other and with this function they aren't anymore. That's all :)

[tertsdiepraam]

Upon second thought, that's maybe not what I dislike. I find it counterintuitive that by applying a function on the CmdResult, the CmdResult is not really the result of the command anymore. Which is why I prefer the predicate API. I also think the predicate API is clearer when checking both stdout and stderr, because it visually groups the transformations:

ucommand
    .run()
    .stdout_str_check(|s| {
        s.trim() == "something"
    })
    .stderr_str_check(|s| {
        s.trim() == "something else"
    });

// vs
ucommand
    .run()
    .stdout_str_apply(str::trim)
    .stdout_is("something")
    .stderr_str_apply(str::trim)
    .stderr_is("something else");

[Joining7943]

I think both methods have their field of application :) They can be combined if wished so. In the end, it's up to the user how to realize it. Do you want to implement the predicate API? If not, I could do it as part of #4261.

[tertsdiepraam]

I think both methods have their field of application :) They can be combined if wished so.

The applications are basically the same though. In my opinion they're too similar to have both. If you can give a realistic example where the apply API is clearly better I might be convinced, but I can't think of any.

Do you want to implement the predicate API? If not, I could do it as part of #4261.

Feel free to give it a shot!

[Joining7943]

The advantage of the apply api is, that the existing assertion methods in CmdResult can still be used. It's preprocessing of the output before calling the actual assertions in CmdResult. Setting up an assertion like stdout_is_fixture or stdout_is_templated_fixture within the predicate closure isn't very convenient and unnecessary code duplication. The apply api also removes the need for having stdout_trimmed_is_fixture and stdout_trimmed_end_is_fixture (or whatever can be done with a str) ... , so prevents a flood of assertion methods and makes the existing assertion methods more widely applicable.

I would classify the apply method as the editing side of the str (&[u8]) type, which provides an interface to the string editing methods in it. In addition it's possible to manipulate the str (&[u8]) in any way one likes.

The predicate api on the other hand provides a convenient interface to the string verification methods (like is_empty, is_ascii_uppercase, matches or equivalence == (!=) etc.) in the str (&[u8]) type. It's also possible to verify the str (&[u8]) in any way one likes.

Having both apis makes CmdResult somewhat complete.

[tertsdiepraam]

Oh yeah I totally didn't think of those other methods. Very good point! Thanks!

[Joining7943]

Sure. I implemented everything we discussed here in the associated pr, if you wanna have a look.

The PR's ready so far but since we're already talking about CmdResult, I would like to address two other issues, I encountered:

• stdout_matches and stdout_does_not_match trim the stdout without notion. Instead of providing stdout_trimmed_matches etc. I would suggest to remove the trim() call from both methods and rewrite the affected tests to use stdout_str_apply(str::trim) (when appropriate) before calling this method. Then all methods in CmdResult would be free of unexpected trimming and it's clear if the test matches against a trimmed stdout or not.
• In stderr_is_bytes and stdout_is_bytes the error message from a failed assert could be improved, by also printing the bytes converted to a string, if possible (in addition to the byte array diff). If not possible we could include the error message why this is not a valid string and, for example, report utf-8 issues in stdout, stderr that way. It's sometimes pretty hard to figure out what's wrong by only looking at the diff of two byte arrays.

[tertsdiepraam]

Maybe it's better two address those in another PR? Sounds like it would require changes in a lot of tests.

[tertsdiepraam]

On a related note, I think we can get rid of the bytes methods by making something like the Pattern trait in the standard library.

I.e.

trait AssertOutput {
    fn assert_equal(self, output: &[u8]);
}

impl AssertOutput for &str { todo!() }
impl AssertOutput for &[u8] { todo!() }

impl CmdResult {
    fn stdout_is(expected: impl AssertOutput) { todo!() }
}

We could then also implement AssertOutput for Fn(&[u8]) -> bool and Fn(&str) -> bool and then we don't need the separate check method either.

[Joining7943]

Maybe it's better two address those in another PR? Sounds like it would require changes in a lot of tests.

yep no problem, although only the changes of stdout_matches require the tests (I found around 50 usages of these methods) to be changed. The latter suggestion is pretty small and could fit into this pr, if you're ok with. Only the stdout_is_bytes and stderr_is_bytes methods need to be modified.

On a related note, I think we can get rid of the bytes methods by making something like the Pattern trait in the standard library ...

I like the idea! Let's try it, but maybe also in another pr?

[tertsdiepraam]

Yeah the second suggestion is small indeed. And yeah my idea was indeed also for another PR.