phd: check exit status of commands, have tests expect success/failure #797
Conversation
|
oh the Rust checks don't run until a PR is actually opened, that explains why this passed without the clippy/1.82.0 patch. i was wondering how that could be.. |
| // TO REVIEWERS: it would be really nice to write this as a function | ||
| // returning a `struct ShellCommandExecutor` that impls | ||
| // `Future<Output=String>` where the underlying ShellOutput is automatically | ||
| // `.expect_ok()`'d. In such a case it would be possible for the struct to | ||
| // have an `.expect_err()` that replaces teh default `.expect_ok()` | ||
| // behavior, so that the likely case doesn't need any change in PHD tests. | ||
| // | ||
| // unfortunately I don't know how to plumb the futures for that, since we'd | ||
| // have to close over `&self`, so doing any Boxing to hold an | ||
| // `async move {}` immediately causes issues. |
There was a problem hiding this comment.
my thinking is that it'd be nice if vm.run_shell_command("foo").await automatically did what expect_ok does in this change, but if you know to expect an error you could vm.run_shell_command("foo").expect_err().await to modify the future and then run it. concretely it looked something like:
struct ShellCommandExecutor {
fut: Pin<Box<impl Future<Output=Result<ShellOutput>> + Send + Sync>>
}
impl ShellCommandExecutor {
fn expect_err(self) -> ShellCommandExecutorExpectErr { /* build the thing */ }
}
impl Future for ShellCommandExecutor {
fn poll(&self, cx: &mut Context) -> Self::Output {
match self.fut.poll(cx) {
Poll::Ready(v) => Poll::Ready(v.expect_ok()),
Poll::Pending => Poll::Pending
}
}
}but i couldn't figure out how to impl Future in a way that doesn't conflict with fut holding &self. so this patch does the next best thing i could think of.
There was a problem hiding this comment.
This feels like it's what IntoFuture is supposed to be for, right? "I want to have a thing which is not actually a Future implementation, but knows how to turn itself into a future, so you can still .await it and it will go and turn itself into a future".
There was a problem hiding this comment.
ok! now that i got the more Future-ful approach together i think this all is much cleaner (and incidentally resolves your other comments). it's kinda sickrad that this only comes with like 10 changes in tests now, since it's going along with the general assumption that commands run in guests should be successful.
| @@ -37,6 +37,7 @@ async fn guest_reboot_test(ctx: &Framework) { | |||
| vm.launch().await?; | |||
| vm.wait_to_boot().await?; | |||
|
|
|||
| // XXX: use graceful_reboot() now. | |||
There was a problem hiding this comment.
(got a followup patch for this)
| // TO REVIEWERS: it would be really nice to write this as a function | ||
| // returning a `struct ShellCommandExecutor` that impls | ||
| // `Future<Output=String>` where the underlying ShellOutput is automatically | ||
| // `.expect_ok()`'d. In such a case it would be possible for the struct to | ||
| // have an `.expect_err()` that replaces teh default `.expect_ok()` | ||
| // behavior, so that the likely case doesn't need any change in PHD tests. | ||
| // | ||
| // unfortunately I don't know how to plumb the futures for that, since we'd | ||
| // have to close over `&self`, so doing any Boxing to hold an | ||
| // `async move {}` immediately causes issues. |
There was a problem hiding this comment.
This feels like it's what IntoFuture is supposed to be for, right? "I want to have a thing which is not actually a Future implementation, but knows how to turn itself into a future, so you can still .await it and it will go and turn itself into a future".
| impl ShellOutput { | ||
| /// Consume this [`ShellOutput`], returning the command's output as text | ||
| /// if the command completed successfully, or an error if it did not. | ||
| pub fn expect_ok(self) -> Result<String> { |
There was a problem hiding this comment.
it's a bit weird to me that expect_ok() returns a Result instead of panicking --- i would generally expect (haha) a function named expect_foo to panic if the expectation is violated. Perhaps these would make more sense as just .ok()/.err()? I dunno. Maybe this is clearer to others...
There was a problem hiding this comment.
hum yeah that seems pretty reasonable. this naming probably made a little more sense in the first draft where i was hoping this would configure a future to expect the result is ok rather than this goofy-Result-to-Result transformation. i'll see if i can get something nice with IntoFuture and if not i'll s/expect_/g
this makes `run_shell_command` a bit more magic but keeps tests less boilerplate-y. tyvm Eliza for helping me figure out how to put the future together...
moderately in the vein of std::process::Command, though i figure trying to separate out stdout/stderr is very much not worth the effort.
typically if a command fails or status-es a subsequent assert will fail, but in #792 we fail through to success. in the boot order tests with a Windows guest, the test likely fails through
dd | xxd -and errors about being unable to unhex the output (though some tests fail for other reasons earlier)so, hopefully this doesn't seem like too much tedium to layer on top of shell commands to double-check test commands are doing what we expect. but i'm not deeply attached to the change one way or the other!