Stream: git-wasmtime

Topic: wasmtime / PR #10137 Add test for opening miscellaneous d...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2025 at 16:45):

erikrose opened PR #10137 from erikrose:dev-read-test to bytecodealliance:main:

Fixes #514, but CI is grumpy and I have questions.

Both the sync and async variants run by cargo test -p wasi-common preview1_device_read work and are valid.

However, foreach_preview1!(assert_test_exists) fails its assertions when I run ci/run-tests.sh unless I repeat my tests in wasi (commented out here) as well as wasi-common. Is that the intent? I'm not sure what value such repetition has for this test. Perhaps I'm adding tests in the wrong place altogether. I welcome any illumination! (@alexcrichton?)

error[E0432]: unresolved import `self::preview1_device_read`
  --> crates/wasi/tests/all/main.rs:86:13
   |
86 |         use self::$name as _;
   |             ^^^^^^^^^^^^^^^^ no `preview1_device_read` in `async_`
   |
  ::: crates/wasi/tests/all/async_.rs:30:1
   |
30 | foreach_preview1!(assert_test_exists);
   | -------------------------------------
   | |
   | help: a similar name exists in the module: `PREVIEW1_DEVICE_READ`
   | in this macro invocation
   |
   = note: this error originates in the macro `assert_test_exists` which comes from the expansion of the macro `foreach_preview1` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0432]: unresolved import `self::preview1_device_read`
  --> crates/wasi/tests/all/main.rs:86:13
   |
86 |         use self::$name as _;
   |             ^^^^^^^^^^^^^^^^ no `preview1_device_read` in `preview1`
   |
  ::: crates/wasi/tests/all/preview1.rs:28:1
   |
28 | foreach_preview1!(assert_test_exists);
   | -------------------------------------
   | |
   | help: a similar name exists in the module: `PREVIEW1_DEVICE_READ`
   | in this macro invocation
   |
   = note: this error originates in the macro `assert_test_exists` which comes from the expansion of the macro `foreach_preview1` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0432]: unresolved import `self::preview1_device_read`
  --> crates/wasi/tests/all/main.rs:86:13
   |
86 |         use self::$name as _;
   |             ^^^^^^^^^^^^^^^^ no `preview1_device_read` in `sync`
   |
  ::: crates/wasi/tests/all/sync.rs:32:1
   |
32 | foreach_preview1!(assert_test_exists);
   | -------------------------------------
   | |
   | help: a similar name exists in the module: `PREVIEW1_DEVICE_READ`
   | in this macro invocation
   |
   = note: this error originates in the macro `assert_test_exists` which comes from the expansion of the macro `foreach_preview1` (in Nightly builds, run with -Z macro-backtrace for more info)

Note that I've hard-coded a "/dev" preopen into the test harnesses temporarily. Once I'm sure I'm adding to the right set of tests, I'll refactor to pass it only for this new one.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2025 at 18:27):

erikrose updated PR #10137.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2025 at 20:07):

alexcrichton commented on PR #10137:

Thanks! Yeah what you're seeing here is a bit of history + testing infrastructure, sorry for the confusion...

The wasi-common crate that #514 refers to has been deprecated for some time now in favor of wasmtime-wasi, although currently all preview1_*.rs tests are run in both to ensure neither regresses. The infrastructure basically asserts (via a slightly roundabout method) that there's a test function for each test case which is hand-written. You've added the tests to wasi-common here but the wasmtime-wasi test suite asserts it's running all preview1 tests here but uncommenting the blocks there should be good to go.

One thing you'll need to handle I think though is Windows-vs-Unix as these tests probably won't pass on Windows. Additionally it looks like you're changing how all tests are run by preopening /dev instead of the workspace? If possible that'd only be done for this one test as opposed to all of them, which might need a slightly fancier helper function to run the test than what exists right now

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2025 at 20:53):

erikrose commented on PR #10137:

Thanks so much! That clears it right up. Redundancy that serves a purpose is no redundancy at all. :-)

One thing you'll need to handle I think though is Windows-vs-Unix as these tests probably won't pass on Windows.

AFAIK, Windows doesn't have file-like devices, so I don't believe the tests make sense there. Indeed, ports of utils like dd have had to implement fakes to work around the lack. That's why I stuck#[cfg_attr(not(unix), ignore)] around the tests initially. Though please scream if I'm missing something.

Additionally it looks like you're changing how all tests are run by preopening /dev instead of the workspace?

Just some temporary violence. I had said this below the giant error message, but hiding it there made it easy to miss:

Note that I've hard-coded a "/dev" preopen into the test harnesses temporarily. Once I'm sure I'm adding to the right set of tests, I'll refactor to pass it only for this new one.

Thanks again! With that new information, I'll see if I can get things going.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 28 2025 at 21:30):

alexcrichton commented on PR #10137:

Oops sorry I completely missed the #[cfg_attr(not(unix), ignore)], sounds like you've got it all in hand :+1:

Last updated: Feb 28 2025 at 02:27 UTC