Review comments

crates/test-programs/tests/wasm_tests/runtime.rs

@@ -1,7 +1,7 @@
 use anyhow::{bail, Context};
 use cranelift_codegen::settings::{self, Configurable};
 use std::fs::File;
-use std::{collections::HashMap, path::Path};
+use std::path::Path;
 use wasmtime::{Config, Engine, HostRef, Instance, Module, Store};
 
 pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> anyhow::Result<()> {
@@ -18,7 +18,6 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any
     let engine = HostRef::new(Engine::new(&config));
     let store = HostRef::new(Store::new(&engine));
 
-    let mut module_registry = HashMap::new();
     let global_exports = store.borrow().global_exports().clone();
     let get_preopens = |workspace: Option<&Path>| -> anyhow::Result<Vec<_>> {
         if let Some(workspace) = workspace {
@@ -47,19 +46,13 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any
     // stdin is closed which causes tests to fail.
     let (reader, _writer) = os_pipe::pipe()?;
     builder = builder.stdin(reader_to_file(reader));
-
-    // The current stable Rust toolchain uses the old `wasi_unstable` ABI,
-    // aka `snapshot_0`.
-    module_registry.insert(
-        "wasi_snapshot_preview1".to_owned(),
-        Instance::from_handle(
-            &store,
-            wasmtime_wasi::instantiate_wasi_with_context(
-                global_exports.clone(),
-                builder.build().context("failed to build wasi context")?,
-            )
-            .context("failed to instantiate wasi")?,
-        ),
+    let snapshot1 = Instance::from_handle(
+        &store,
+        wasmtime_wasi::instantiate_wasi_with_context(
+            global_exports.clone(),
+            builder.build().context("failed to build wasi context")?,
+        )
+        .context("failed to instantiate wasi")?,
     );
 
     // ... and then do the same as above but for the old snapshot of wasi, since
@@ -73,16 +66,13 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any
     }
     let (reader, _writer) = os_pipe::pipe()?;
     builder = builder.stdin(reader_to_file(reader));
-    module_registry.insert(
-        "wasi_unstable".to_owned(),
-        Instance::from_handle(
-            &store,
-            wasmtime_wasi::old::snapshot_0::instantiate_wasi_with_context(
-                global_exports.clone(),
-                builder.build().context("failed to build wasi context")?,
-            )
-            .context("failed to instantiate wasi")?,
-        ),
+    let snapshot0 = Instance::from_handle(
+        &store,
+        wasmtime_wasi::old::snapshot_0::instantiate_wasi_with_context(
+            global_exports.clone(),
+            builder.build().context("failed to build wasi context")?,
+        )
+        .context("failed to instantiate wasi")?,
     );
 
     let module = HostRef::new(Module::new(&store, &data).context("failed to create wasm module")?);
@@ -91,20 +81,22 @@ pub fn instantiate(data: &[u8], bin_name: &str, workspace: Option<&Path>) -> any
         .imports()
         .iter()
         .map(|i| {
-            let module_name = i.module().as_str();
-            if let Some(instance) = module_registry.get(module_name) {
-                let field_name = i.name().as_str();
-                if let Some(export) = instance.find_export_by_name(field_name) {
-                    Ok(export.clone())
-                } else {
-                    bail!(
-                        "import {} was not found in module {}",
-                        field_name,
-                        module_name
-                    )
-                }
+            let instance = if i.module().as_str() == "wasi_unstable" {
+                &snapshot0
+            } else if i.module().as_str() == "wasi_snapshot_preview1" {
+                &snapshot1
+            } else {
+                bail!("import module {} was not found", i.module().as_str())
+            };
+            let field_name = i.name().as_str();
+            if let Some(export) = instance.find_export_by_name(field_name) {
+                Ok(export.clone())
             } else {
-                bail!("import module {} was not found", module_name)
+                bail!(
+                    "import {} was not found in module {}",
+                    field_name,
+                    i.module().as_str(),
+                )
             }
         })
         .collect::<Result<Vec<_>, _>>()?;

crates/test-programs/wasi-tests/src/bin/clock_time_get.rs

@@ -4,13 +4,12 @@ unsafe fn test_clock_time_get() {
     // Test that clock_time_get succeeds. Even in environments where it's not
     // desirable to expose high-precision timers, it should still succeed.
     // clock_res_get is where information about precision can be provided.
-    wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 1)
-        .expect("precision 1 should work");
+    wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 1).expect("precision 1 should work");
 
-    let first_time = wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 0)
-        .expect("precision 0 should work");
+    let first_time =
+        wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 0).expect("precision 0 should work");
 
-    let time = wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 0).unwrap();
+    let time = wasi::clock_time_get(wasi::CLOCKID_MONOTONIC, 0).expect("re-fetch time should work");
     assert_le!(first_time, time, "CLOCK_MONOTONIC should be monotonic");
 }
 

crates/test-programs/wasi-tests/src/lib.rs

@@ -6,6 +6,10 @@ use std::ffi::CString;
 use std::io;
 use wasi_old::wasi_unstable;
 
+/// Opens a fresh file descriptor for `path` where `path` should be a preopened
+/// directory. This is intended to be used with `wasi_unstable`, not with
+/// `wasi_snapshot_preview1`. This is getting phased out and will likely be
+/// deleted soon.
 pub fn open_scratch_directory(path: &str) -> Result<wasi_unstable::Fd, String> {
     // Open the scratch directory.
     let dir_fd: wasi_unstable::Fd = unsafe {
@@ -24,6 +28,11 @@ pub fn open_scratch_directory(path: &str) -> Result<wasi_unstable::Fd, String> {
     }
 }
 
+/// Same as `open_scratch_directory` above, except uses `wasi_snapshot_preview1`
+/// APIs instead of `wasi_unstable` ones.
+///
+/// This is intended to replace `open_scratch_directory` once all the tests are
+/// updated.
 pub fn open_scratch_directory_new(path: &str) -> Result<wasi::Fd, String> {
     unsafe {
         for i in 3.. {