Support operation based configuration#199
Conversation
|
Reporting-wise, even though the validators were subsumed by the operation description, not much has changed, aside from some minor cosmetic changes for DetailsTaken from: https://github.com/WebAssembly/wasi-testsuite/actions/runs/20829402028/job/59839495262?pr=199Test remove_directory_trailing_slashes failed
[Expectation mismatch] Wait(exit_code=0) failed: expected 0, got 134
==STDERR==
thread 'main' (1) panicked at src/bin/remove_directory_trailing_slashes.rs:19:10:
remove_directory with a trailing slash on a directory should succeed: Errno { code: 28, name: "INVAL", message: "Invalid argument." }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: failed to run main module `tests/rust/testsuite/wasm32-wasip1/remove_directory_trailing_slashes.wasm`
Caused by:
0: failed to invoke command default
1: error while executing at wasm backtrace:
0: 0xf558 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!abort
1: 0x9938 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::sys::pal::wasip1::helpers::abort_internal::he4ccae4da0b891f2
2: 0x764b - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::process::abort::h08641a5502f2d3bc
3: 0x76c7 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!__rustc[d9b87f19e823c0ef]::__rust_abort
4: 0x742f - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!__rustc[d9b87f19e823c0ef]::__rust_start_panic
5: 0x7454 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!__rustc[d9b87f19e823c0ef]::rust_panic
6: 0xa7d1 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::panicking::panic_with_hook::hfa2bd51216c99200
7: 0x9c50 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::panicking::panic_handler::{{closure}}::h6c4c622b7534cc7e
8: 0x77b6 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::sys::backtrace::__rust_end_short_backtrace::hd804cb3f65eca479
9: 0x77aa - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!__rustc[d9b87f19e823c0ef]::rust_begin_unwind
10: 0x11bf3 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!core::panicking::panic_fmt::h9ad9818a2179998c
11: 0x1466a - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!core::result::unwrap_failed::hdf10c517ba830923
12: 0x33cf - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!core::result::Result<T,E>::expect::h734d0265a4c1c763
13: 0x18bf - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!remove_directory_trailing_slashes::test_remove_directory_trailing_slashes::he40762d2527fd8b6
14: 0x2478 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!remove_directory_trailing_slashes::main::h5ebae87b7d3154e4
15: 0x2570 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!core::ops::function::FnOnce::call_once::hc49b7f0102ad9334
16: 0x2549 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::sys::backtrace::__rust_begin_short_backtrace::hc30856ca4720cf1c
17: 0x2533 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::rt::lang_start::{{closure}}::h76c79d29d038749d
18: 0x8c61 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::rt::lang_start_internal::hbd75f3360f3556c2
19: 0x2515 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!std::rt::lang_start::h9340e6bfaad846bf
20: 0x31bb - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!__main_void
21: 0x5c6 - remove_directory_trailing_slashes-08a7e14a215daa4c.wasm!_start
2: wasm trap: wasm `unreachable` instruction executed |
|
Unsure about the |
wingo
left a comment
There was a problem hiding this comment.
Looking great! Two open questions for me: the adapter interface, and a test script validation pass. I think ideally we settle the adapter question at least in this PR; other nits can be addressed here or later as you like.
| if str(_test_runner_path) not in sys.path: | ||
| sys.path.insert(0, str(_test_runner_path)) | ||
|
|
||
| _test_case_module = importlib.import_module('wasi_test_runner.test_case') |
There was a problem hiding this comment.
Hmm, I think importing test runner modules from adapters is something to avoid. To me there are two sensible setups:
- Adapters can be written independently from the test runner, e.g. in a third-party repo, and therefore shouldn't import from the test runner. In this case the parameters and results of an adapter's exported functions are just plain old data.
- Adapters are part of the test runner, should live in the test runner dir, can import test runner modules, and we avoid all the importlib silliness.
Here we have a mix of the two. WDYT is the right way to go? FWIW my instinct would be to consider splitting out the changes to adapters to a second patch, but ymmv.
There was a problem hiding this comment.
cb721ba to
77653a6
Compare
|
@wingo I believe I've addressed all of your comments, this should be ready for another review. As mentioned in #199 (comment), I'll follow-up with documentation updates in a different patch and I'll rebase on top of https://github.com/WebAssembly/wasi-testsuite/pull/201/changes once it lands to get rid of the failing Markdown job. |
This commit refactors the test suite to support operation-based configuration as discussed in WebAssembly#194 (comment) To ensure backwards compatibility with the previous ad-hoc configuration, this change translates from the older configuration to the newer configuration "on the fly", so other test runners can continue using the previous configuration for the time being. The main motiviation of this change is to set the stage for more complex testing, particularly for connection-based tests in which we'd like to have fine-grained control over the steps and permissions involved when dealing with client/server interactions. Note that the intention is for new tests, especially those involving client/server interactions will adopt the new configuration.
This commit removes test runner imports from the adapters, allowing them to be developed independently. Additionally this commit introduces the concept of `proposals` in the configuration, which are passed to the configuration in the form of `List[str]`, through the `proposals` the adapters can derive the specific flags needed to run the test to completion. As a last change, this commit also migrates the http-response test to use the new test configuration using the `http` proposal. Without this change it is not straight forward to derive "base proposals" for tests with the legacy configuration that wish to use specific proposals. We could certainly introduce other heuristics, at the cost of more complexity and seemingly not many advantages.
This commit adds a `dry_run` method to the config, so that it can be called before executing tests. The dry run validates the semantic struture of the operation definitions. Aditionally this commit adds more robust error handling to the main test loop.
77653a6 to
9ad2e3b
Compare
|
Rebased. |
wingo
left a comment
There was a problem hiding this comment.
Looks great, thanks for bearing with my nitpicks!
This commit refactors the test suite to support operation-based
configuration as discussed in
#194 (comment)
To ensure backwards compatibility with the previous ad-hoc
configuration, this change translates from the older configuration to
the newer configuration "on the fly", so other test runners can
continue using the previous configuration for the time being.
The main motiviation of this change is to set the stage for more
complex testing, particularly for connection-based tests in
which we'd like to have fine-grained control over the steps and
permissions involved when dealing with client/server interactions.
Note that the intention is for new tests, especially those involving
client/server interactions will adopt the new configuration.