test: improve test infrastructure #554
Conversation
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.
| looks like (see [`run-test.sh`]): | ||
|
|
||
| ```sh | ||
| $ ls run/$TARGET_TRIPLE/misc/some-test.c |
There was a problem hiding this comment.
Having directory names that end in .c seems a little strange. Perhaps just strip the extension?
There was a problem hiding this comment.
Yeah, I guess that's true. I was trying to make it clear which test it was coming from... but you're right that it just looks confusing.
| // filter.py(TARGET_TRIPLE): !wasm32-wasip2 | ||
| // add-flags.py(CFLAGS): ... | ||
| // add-flags.py(LDFLAGS): ... | ||
| // add-flags.py(RUN): ... |
There was a problem hiding this comment.
Would this be more readable if we drop the .py here?
Also perhaps a special prefix such as "//!" would be good to distinguish directives from normal comments?
There was a problem hiding this comment.
Agreed on the //! bit. Still mulling over the .py suggestion: one thing that always bugs me with infrastructure stuff like this is that I don't know where to look if something goes wrong. I figured if I put the .py extension on there most of us would think, "oh, I see, this logic is from some file... let's bust out find..." But that may not be as clear as I think?
| set -e | ||
|
|
||
| TESTDIR=${1:-.} | ||
| FAILED=$(find $TESTDIR -type f -and -name *.log -and -not -size 0) |
There was a problem hiding this comment.
The comment above says .err but here you are looking for .log?
test/scripts/run-test.sh
Outdated
| echo "$ENGINE $WASM" > cmd.sh | ||
| chmod +x cmd.sh | ||
| ./cmd.sh &> output.log | ||
| [ $? -eq 0 ] || echo "Test failed" >> output.log |
There was a problem hiding this comment.
It looks like the string "Test failed" at the end of output.log is the signal that the test failed? Is that right?
There was a problem hiding this comment.
It's actually if there's any output at all; appending this string just makes sure of that.
test/scripts/run-test.sh
Outdated
| ./cmd.sh &> output.log | ||
| [ $? -eq 0 ] || echo "Test failed" >> output.log | ||
| popd > /dev/null | ||
|
|
To avoid a `Makefile` dependency issue, a previous commit changed the location of the download directory to live inside the build directory; this change propagates that to the CI configuration. Also, it is no longer necessary to clean up anything between runs: both the `build` and `run` directory are subdivided by target triple so repeated builds will not interfere.
This change represents a rather large re-design in how
wasi-libcbuilds and runs its tests. Initially, #346 retrieved thelibc-testrepository 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 asmokedirectory 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*p2targets or*-threadstargets, 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#includethe original version. This not only keeps all enabled tests in one place, it also allows us to add "directives," C comments that theMakefileuses 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 thescriptsdirectory.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.