Integrate misc-tests into wasi-common#29
Integrate misc-tests into wasi-common#29sunfishcode merged 9 commits intoCraneStation:masterfrom kubkon:tests-integration
Conversation
sunfishcode
left a comment
There was a problem hiding this comment.
tests/runtime/wasi.rs appears to be duplicating a lot of code from wasmtime. Is there a way we could factor/organize things to minimize the duplication?
Would it be possible to instead import the wasmtime-wasi crate, and use a patch to replace its wasi-common repo with the local tree?
Another option would be to integrate misc-tests into the wasmtime repo, rather than here in the wasi-common repo. That would provide clearer layering, however it would mean that we wouldn't get many tests that could test wasi-common in isolation. However, it might not be a bad idea to start building up a testsuite for wasi-common that just works in terms of WASI API calls rather than through full wasm programs.
Thoughts?
I hate code duplication as well, so I'm all for maximising code re-use as much as possible.
That sounds like a great idea! I'll give it a go and see where it takes us. I actually didn't know you can do crazy things like this with Cargo, so thanks for pointing it out! :-)
That's a good point. In fact, I'm currently also more inclined to have those tests as part of the To sum up, I'll try the patch approach first, and if that fails, I guess we can think on how to integrate |
|
@sunfishcode OK, I've successfully managed to import |
sunfishcode
left a comment
There was a problem hiding this comment.
Nice
I like your changes to misc-tests to split it into separate programs. What would you think of moving this into a CraneStation repository, named wasi-misc-tests or something?
Right now, your misc-tests repo contains the precompiled wasm files that it builds, and then you pull them in by git submodule here. I find it a little confusing, because this also pulls in the source for these tests too, however we don't actually build from the source here; we only use the precompiled versions. What would you think about checking the precompiled wasms into the wasi-common repo instead, and avoiding the submodule?
tests/misc_tests.rs
Outdated
| #[test] | ||
| fn sched_yield() -> Result<(), String> { | ||
| run_test_without_workspace("tests/misc-tests/bin/sched_yield.wasm") | ||
| } |
There was a problem hiding this comment.
What would you think of auto-generating these test functions, similar to what wasm time does for the spec testsuite? That way, when we add new tests, we don't have to remember to add them here before they actually get run.
There was a problem hiding this comment.
SGTM! And this is exactly what I've done now so have a look :-)
Now, test binaries are bundled with the repo, and just like in CraneStation/wasmtime, the test cases are generated automatically using build.rs. So all it takes is to drop a new test binary in the testsuite dir to get the test case for it generated (with some caveats to do with handling preopens).
Great idea! I've now cleaned up my fork of
Yeah, that's a fair point. My thinking here was that it would be nice to preserve some context for the test binaries we use. However, this is actually not needed since, in the end, we will still have access to the sources in a separate repo. So now, I've removed the submodule, and instead, as you suggested, bundled the test binaries directly in |
|
Cool. If we later decide we'd like to organize things differently, we can change things of course :-). |
This PR builds on #28. It integrates tests from sunfishcode/misc-tests into
wasi-common. In order to achieve more seamless integration and better control of each tested aspect, I've taken the liberty of modularising sunfishcode/misc-tests into separate testcases. The effect of this work can be seen in my fork kubkon/misc-tests.Summary of changes:
The workspaces used (per each testcase which is essentially a separate executable Wasm module) is generated on the fly and saved in the host's
tempdir. In order to avoid any race conditions, etc., each testcase module gets its own workspace dir suffixed with the timestamp of execution. This can potentially generate quite a few directories, albeit in host'stempdir, so I'm wondering whether there could be a better way of handling this. Any thoughts on this are more than welcome! :-)EDIT: I was thinking of simply disabling all of the tests on Win, but still I'd expect theOK, I've separated out tests which require preopens from those that don't, and nowbuf_random_buftestcase to pass. Somehow, this is not the case, so I'll have to investigate. Anyhow, otherwise, feel free to review the changes as I don't expect the bulk to change in the process of me fixing the Win build.buf_random_buftestcase passes just fine 👍