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

test: run a subset of tests using libc-test #346

Merged
merged 2 commits into from
Dec 3, 2022

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Nov 28, 2022

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. 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 some src/functional tests are enabled but the src/math and src/api tests could also be useful.

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.
@abrown abrown marked this pull request as draft November 28, 2022 18:00
@abrown abrown force-pushed the libc-test branch 3 times, most recently from f6f8316 to 9fe391d Compare November 28, 2022 18:23
mkdir -p download

$(LIBC_TEST): | $(DOWNDIR)
git clone --depth 1 $(LIBC_TEST_URL) $@
Copy link
Member

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?

Copy link
Collaborator Author

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?

Copy link
Member

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..

Copy link
Collaborator Author

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.

@loganek
Copy link
Contributor

loganek commented Nov 29, 2022

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?).

@abrown abrown force-pushed the libc-test branch 4 times, most recently from 2d8af7d to 55f9e19 Compare November 29, 2022 17:07
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.
@abrown
Copy link
Collaborator Author

abrown commented Nov 29, 2022

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?).

@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 abrown marked this pull request as ready for review November 29, 2022 18:14
@loganek
Copy link
Contributor

loganek commented Nov 30, 2022

@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).
Regarding the abstraction layer - wasi-testsuite is aiming to focus mainly on the WASI interfaces and not standard library implementation (that's why I think having those tests in wasi-libc is the right thing to do), but we also have tests that use libc implementation if that simplifies testing. So I'm just calling out that there will be an overlap, but how to share tests (or whether we should do that at all) can probably be discussed later on.

@abrown
Copy link
Collaborator Author

abrown commented Nov 30, 2022

@loganek, makes sense; thanks for the heads up!

@abrown abrown requested review from sbc100 and sunfishcode November 30, 2022 23:08
@abrown
Copy link
Collaborator Author

abrown commented Nov 30, 2022

@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:

  • the test/Makefile is very Linux-specific and requires a weird "put this .a file over here" hack to make Clang compile the tests; I've heard comments to the effect of "it's fine for a starter PR" but I wanted to bring it up to see if there is something cleaner I could have done
  • I have only enabled a subset of tests (the ones that passed!); ideally we would enable more by resolving whatever problems we encounter. But I don't want to be the sole owner of "make all the tests work"--does the current Makefile and the libc-test organization seem clear enough for others to hack on to add more tests? Is anyone else interested in helping out enabling tests? I can create an issue to track this but wanted to gauge how much interest there is in the testing effort.

@sunfishcode
Copy link
Member

@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.

@loganek
Copy link
Contributor

loganek commented Dec 2, 2022

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.

@abrown abrown merged commit 8098d86 into WebAssembly:main Dec 3, 2022
@abrown abrown deleted the libc-test branch December 3, 2022 02:27
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
* 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.
abrown added a commit to abrown/wasi-libc that referenced this pull request Nov 21, 2024

Verified

This commit was signed with the committer’s verified signature.
abrown Andrew Brown
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.
abrown added a commit that referenced this pull request Dec 9, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.
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