-
Notifications
You must be signed in to change notification settings - Fork 206
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
test: run a subset of tests using libc-test
#346
Conversation
This change introduces a `test` directory that retrieves the `libc-test` suite, compiles a subset of the tests using `wasi-libc`, and runs them with Wasmtime.
f6f8316
to
9fe391d
Compare
mkdir -p download | ||
|
||
$(LIBC_TEST): | $(DOWNDIR) | ||
git clone --depth 1 $(LIBC_TEST_URL) $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a git submodule instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some concerns about licensing; some parts (unused here) are licensed under GPL. In discussions with @sunfishcode, we felt that downloading the code on demand and never releasing any of the libc-test parts avoided license contamination with this repo. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how doing a git clone
programmatically is any different to using a git submodule, but IANAL..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see what you mean; I guess I had thought that cloning it at a different time made it more optional (i.e., only those who want to run tests). I set up a meeting to talk to someone here about it because... IAANAL.
Just a general comment about that; we've recently started a wasi-testsuite project that aims to provide unified tests and runtime environment that's easily integrated with any WASM VM. Whereas the goal of wasi-testsuite is to validate WASI implementation of various VMs and the goal of the tests you just added is to verify the correctness of libc implementation, I think there will be an overlap. I wonder if we can somehow share some of the tests? One option I can think of is moving all the wasi-libc tests to wasi-testsuite and run them as part of the CI in this repository, but the disadvantage of that is that not all the tests actually validate WASI layer (although I don't think this is such a big of the concern?). |
2d8af7d
to
55f9e19
Compare
This change includes some fixups to the filesystem to place Clang's runtime library for `wasm32-wasi` in the right location. Note that this CI action is limited to a single OS--Linux.
@loganek, I actually am agnostic about where these tests should live and even if libc-test should be the only suite to use (@sbc100 also suggested the Open POSIX Test Suite which is used by Emscripten). I do think, though, that given the trickiness of getting these tests to run (I had to iterate a bit on 93606b7), I would prefer to get something merged soon to start checking Wasmtime's pthread implementation. I'm in favor of wasi-testsuite in general but I don't think it is at the same abstraction level as libc-test in this case: e.g., for threads, we could add tests to wasi-testsuite to check that wasi-threads works as expected (and that would be valuable!) but we still need to check that the mapping from pthreads to wasi-threads was done correctly (which is what eventually libc-test could do). |
@abrown I didn't mean to block the PR; let's have something working merged so we can run more tests (I also started working on the WAMR implementation and currently compile pthread tests by myself, so having that as part of wasi-libc would be very valuable). |
@loganek, makes sense; thanks for the heads up! |
@loganek, @sunfishcode, @sbc100: I see two future-facing "problems" with this PR and I wanted to note them here. I still feel we should merge something like this to make progress toward some type of testing for wasi-libc but I wanted to share these concerns as we review it:
|
@abrown My sense here is that this PR is ok with those concerns. The changes here are relatively contained so people who aren't running the tests aren't affected, so I think we can look at it as, this PR adds some value for some people, even if it doesn't do everything for everyone. |
Agree with @sunfishcode , and I'm ok with the current implementation. We can iterate on that if we see opportunity to make it better and unblock other usecases. |
* test: run a subset of tests from `libc-test` This change introduces a `test` directory that retrieves the `libc-test` suite, compiles a subset of the tests using `wasi-libc`, and runs them with Wasmtime. * ci: run tests during CI This change includes some fixups to the filesystem to place Clang's runtime library for `wasm32-wasi` in the right location. Note that this CI action is limited to a single OS--Linux.
This change represents a rather large re-design in how `wasi-libc` builds and runs its tests. Initially, WebAssembly#346 retrieved the `libc-test` repository and built a subset of those tests to give us some amount of test coverage. Later, because there was no way to add custom C tests, WebAssembly#522 added a `smoke` directory which allowed this. But (a) each of these test suites was built and run separately and (b) it was unclear how to add more tests flexibly--some tests should only run on `*p2` targets or `*-threads` targets, e.g. This change reworks all of this so that all tests are built the same way, in the same place. For downloaded tests like those from `libc-test`, I chose to add "stub tests" that `#include` the original version. This not only keeps all enabled tests in one place, it also allows us to add "directives," C comments that the `Makefile` uses to filter out tests for certain targets or add special compile, link or run flags. These rudimentary scripts, along with other Bash logic I moved out of the Makefile now live in the `scripts` directory. Finally, all of this is explained more clearly in an updated `README.md`. The hope with documenting this a bit better is that it would be easier for drive-by contributors to be able to either dump in new C tests for regressions they may find or enable more libc-tests. As of my current count, we only enable 40/75 of libc-test's functional tests, 0/228 math tests, 0/69 regression tests, and 0/79 API tests. Though many of these may not apply to WASI programs, it would be nice to explore how many more of these tests can be enabled to increase wasi-libc's test coverage. This change should explain how to do that and, with directives, make it possible to condition how the tests compile and run.
This change represents a rather large re-design in how `wasi-libc` builds and runs its tests. Initially, #346 retrieved the `libc-test` repository and built a subset of those tests to give us some amount of test coverage. Later, because there was no way to add custom C tests, #522 added a `smoke` directory which allowed this. But (a) each of these test suites was built and run separately and (b) it was unclear how to add more tests flexibly--some tests should only run on `*p2` targets or `*-threads` targets, e.g. This change reworks all of this so that all tests are built the same way, in the same place. For downloaded tests like those from `libc-test`, I chose to add "stub tests" that `#include` the original version. This not only keeps all enabled tests in one place, it also allows us to add "directives," C comments that the `Makefile` uses to filter out tests for certain targets or add special compile, link or run flags. These rudimentary scripts, along with other Bash logic I moved out of the Makefile now live in the `scripts` directory. Finally, all of this is explained more clearly in an updated `README.md`. The hope with documenting this a bit better is that it would be easier for drive-by contributors to be able to either dump in new C tests for regressions they may find or enable more libc-tests. As of my current count, we only enable 40/75 of libc-test's functional tests, 0/228 math tests, 0/69 regression tests, and 0/79 API tests. Though many of these may not apply to WASI programs, it would be nice to explore how many more of these tests can be enabled to increase wasi-libc's test coverage. This change should explain how to do that and, with directives, make it possible to condition how the tests compile and run.
This change introduces a
test
directory that retrieves thelibc-test
suite, compiles a subset of the tests usingwasi-libc
, and runs them with Wasmtime. It also enables the running of these tests in CI.For now, only a small subset of
libc-test
is used. This PR only introduces the ability to run tests and in the future more tests (and more kinds of tests) can be enabled. Currently only somesrc/functional
tests are enabled but thesrc/math
andsrc/api
tests could also be useful.