Update WASI tests to use wasi crate v0.9.0 #743
Conversation
| @@ -55,41 +55,14 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any | |||
| .context("failed to instantiate wasi")?, | |||
| ); | |||
|
|
|||
| // ... and then do the same as above but for the old snapshot of wasi, since | |||
| // a few tests still test that | |||
| let mut builder = wasi_common::old::snapshot_0::WasiCtxBuilder::new() | |||
There was a problem hiding this comment.
Will the changes to this file preclude the ability to have test programs that specifically target wasi_unstable or older snapshots?
My concern is that we should likely (in the future) have test programs that are specifically designed to test our back compat as WASI evolves.
Perhaps we can leave your change as-is and then add support for older WASIs when such test programs are implemented.
There was a problem hiding this comment.
Excellent point! I'm of the opinion we should have tests for different snapshot versions in the future. However, I'm not sure we should have it the way it is ATM. I'm not saying it's bad, just that perhaps we could come up with a more automated way? That said, I'm happy to revert this bit code of back for posterity and future guidance.
There was a problem hiding this comment.
Personally, I think we should keep your change and in the future add additional test crates that are designed against specific versions of WASI for ensuring old test programs continue to work. I think having many different target WASIs mixed into one crate can be pretty confusing.
There was a problem hiding this comment.
@sunfishcode @alexcrichton your thoughts on this?
There was a problem hiding this comment.
@peterhuene's suggestion sounds good to me.
|
FYI, the failing Emscripten job looks like a problem with rustc itself (see rust-lang/rust#66308), and I've now submitted #744 which disables it until the issue is resolved. |
This commit updates _all_ WASI test programs to use the latest version of the `wasi` crate (`v0.9.0`). While at it, it also unifies asserting error conditions across all test programs.
| assert_eq!( | ||
| file_fd, | ||
| wasi_unstable::Fd::max_value(), | ||
| "failed open should set the file descriptor to -1", |
There was a problem hiding this comment.
One unfortunate thing about this change is that it means we no longer test the value of file_fd coming out of the WASI syscall in the failure case. The wasi crate is right to hide this detail, but it is an observable aspect of WASI if one codes at the raw WASI level.
I'm undecided on what we should do here. On one hand, I agree that using the higher-level interfaces makes these tests much nicer. On the other, we will eventually want detailed low-level test coverage at the raw WASI level.
I wonder if it would make sense to do something in the wasi crate? We could add asserts for the returned values on the error paths, maybe controlled by a cargo feature so that they'd be off by default but we could enable them here. Thoughts?
There was a problem hiding this comment.
Yeah, I wasn't sure how to deal with it either. I really like the way the tests look now, as thanks to the latest version of the wasi crate, they are much cleaner now. Having said that, we are now not dealing directly with the syscalls but rather with their higher-level wrappers.
Personally, I like your idea to perform the asserts in the wasi crate and have them feature-gated so that they are off by default. This would probably mean we could move the range check assert
assert_gt!(
fd,
libc::STDERR_FILENO as wasi::Fd,
"file descriptor range check",
);to the wasi crate as well. So it's 👍 from me.
|
Sounds good. I filed an issue to track that, so we can merge this now. |
…tion-definitions Expose function definitions, populate FaerieCompiledFunction
This commit updates all WASI test programs to use the latest
version of the
wasicrate (v0.9.0). While at it, it alsounifies asserting error conditions across all test programs.