search

WebAssembly / wasi-testsuite

WASI Testsuite
Apache License 2.0
53 stars 21 forks source link

Port more tests from Wasmtime's testsuite #68

Closed sunfishcode closed 1 year ago

sunfishcode commented 1 year ago

Following up on ac32f57400cdcdd0425d3085c24fc7fc40011d1c, 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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

yamt commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.