Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for opening miscellaneous devices on UNIX-alikes. #10137

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erikrose
Copy link
Contributor

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.

@github-actions github-actions bot added the wasi Issues pertaining to WASI label Jan 28, 2025
Fixes bytecodealliance#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!

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.
@alexcrichton
Copy link
Member

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

@erikrose
Copy link
Contributor Author

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.

@alexcrichton
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*nix test for opening /dev/*
2 participants