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

Port more tests from Wasmtime's testsuite #68

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

sunfishcode
Copy link
Member

Following up on ac32f57, port several more tests from Wasmtime's testsuite to wasi-testsuite.

It is likely that at least some of these tests are testing behavior which differs between Wasm engine implementations. The intent here is not to instate these as authoritative, but to help surface these differences so that we can discuss whether it's best to change the test, or add documentation to the spec.

Other Wasm engines are welcome to submit their tests to wasi-testsuite as well.

Following up on ac32f57, port several
more tests from Wasmtime's testsuite to wasi-testsuite.

It is likely that at least some of these tests are testing behavior
which differs between Wasm engine implementations. The intent here is
not to instate these as authoritative, but to help surface these
differences so that we can discuss whether it's best to change the test,
or add documentation to the spec.

Other Wasm engines are welcome to submit their tests to wasi-testsuite
as well.
@loganek
Copy link
Collaborator

loganek commented Mar 21, 2023

@sunfishcode thanks for the PR. Do you already have a list of checks in the test that is not currently documented in the spec? I think that'd help with the discussion.

@sunfishcode
Copy link
Member Author

I don't have such a list, but realistically, it's probably almost everything isn't documented yet. I don't think a complete specification of all behavior is realistic in the short term, so I'm proposing we focus on resolving incompatibilities and documenting the resolutions.

@codefromthecrypt
Copy link

right now, tests don't run even on wasmtime on PR. can we please get this fixed?

Even if it is ok for some runtimes to fail, I think at least one should be able to pass, and without knowing that it is a hard to justify merging new tests.

@codefromthecrypt
Copy link

Since the goal is discussion and acknowledged value in beyond wasmtime, I would suggest this:

  1. these behaviors are not just about wasmtime, but they are also about rust and wasi-libc, other compiler owners may differ on the enforcements
  2. if these are for discussion, it is better to split into different PRs vs a batch. Canonicalizing rules is an impactful thing, and each rule matters
  3. ping diverse folks somehow (by CODEOWNERS or just ad-hoc). Don't leave it to watchers

There have been other changes that went in quickly today, they should have been looked at diversely before merge. Let's get a better practice in. It doesn't have to be as robust as for example Go's process, but it should have some diversity in it considering these rules define a large part of technology.

"the event.userdata should contain CLOCK_ID",
);
} else if event.type_ == wasi::EVENTTYPE_FD_READ {
assert_errno!(event.error, wasi::ERRNO_SUCCESS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, i ended up with this: yamt/toywasm@570e670

@yamt
Copy link
Contributor

yamt commented Apr 18, 2023

is it intentional that this batch doesn't include poll_oneoff_files?
i'm asking because i have a portability concern about the test.
cf. https://github.com/yamt/toywasm/blob/2ef7bfca8404de2cdf3e8e3faff3eb6011c4c5d2/test/wasmtime-wasi-tests-blacklist.txt#L15-L22

@sunfishcode
Copy link
Member Author

is it intentional that this batch doesn't include poll_oneoff_files? i'm asking because i have a portability concern about the test. cf. https://github.com/yamt/toywasm/blob/2ef7bfca8404de2cdf3e8e3faff3eb6011c4c5d2/test/wasmtime-wasi-tests-blacklist.txt#L15-L22

My idea here was just to port a lot of tests over to get things started, so I left out a few tests that I anticipated would be more involved.

@loganek
Copy link
Collaborator

loganek commented Aug 21, 2023

After reviewing those tests now I think it makes sense to have them merged. If there are any points for discussions, please open separate issue or PR.

@loganek loganek merged commit 385cb01 into WebAssembly:main Aug 21, 2023
yamt added a commit to yamt/wasi-testsuite that referenced this pull request Sep 22, 2023
* I couldn't find this in wasmtime repo. I suppose this was
  a local modification done as a part of
  WebAssembly#68

* As some implementations have implemented it, it isn't appropriate
  to assert NOTSUP by default.
loganek pushed a commit that referenced this pull request Oct 5, 2023
* I couldn't find this in wasmtime repo. I suppose this was
  a local modification done as a part of
  #68

* As some implementations have implemented it, it isn't appropriate
  to assert NOTSUP by default.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants