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

New test format #146

Closed
wants to merge 6 commits into from
Closed

New test format #146

wants to merge 6 commits into from

Conversation

Teemperor
Copy link
Member

As discussed with @sunfishcode a small example how the test would be more confirming to the design document.

Notes:

  • The $init function is the placeholder name for a function that is called after the module is loaded (same idea as behind ES6 main or .init_array sections. See Define a start function. design#398 ). All test code (e.g. invoke ...) that was previously floating around without a related module can be moved there.
  • (assert_invalid ...) is unchanged.

@jcbeyler
Copy link

I like this though for me this would not work in a long term. You'd like in theory to have a debug version that follows this and a non-debug that does not require the testing like this.

So it seems to me that we would need a $init_debug or something. Plus we still need to define correctly how the module defines this $init method, no?

@kg
Copy link
Contributor

kg commented Oct 19, 2015

I don't think making invoke etc valid inside of function bodies is a good idea. That introduces the concept of host-specific opcodes and I don't think that's something we want.

Right now all our tests are valid module definitions with a bit of non-spec-compliant goo outside for testing, and it's trivial to strip the outside stuff. This approach destroys that.

@rossberg
Copy link
Member

The invoke's also make no sense because the exports have been removed in the patch.

I suppose the actual idea would be to use regular calls. But either way, I don't think this is a particularly useful direction:

  • For real-world modules, you strongly want to separate tests from the module implementation.
  • Most interesting tests consist of assertions, which are not expressible inside a module.

@rossberg-old rossberg-old mentioned this pull request Oct 26, 2015
@Teemperor
Copy link
Member Author

Thanks for all the feedback! The initial PR really had a few errors,
so here is the (hopefully better) second version.

The idea of the PR is to remove three things that are AFAIK not
compatible with the design document:

  • The global namespace for exports
  • AST nodes that are outside of any module like the child nodes of assert_return, invoke and so on.
  • The statements assert_return, assert_trap, assert_return_nan and invoke.
    assert_invalid is not targeted in this PR.

The reasons for the removal are:

  • It's easier to compile the tests into valid binary modules later because
    features like invoke are probably not representable in the binary format.
  • Implementers of wasm compilers/interpreters don't need to implement
    support for above features just for the test suite. They can verify
    their implementation with normal standard-compliant wasm programs.
  • The tests are probably shorter if we don't have to export all functions.

The way this PR addresses those issues is that it

  • introduces an assert module. This module and it functions replace
    all assert_* statements beside assert_invalid.
  • moves all code inside a module. The PR currently proposes a main
    function for that but any other function will also do. This is
    supposed to replace the global namespace, code outside of modules and the invoke statement.
    The interpreter has to call the main function after all modules
    in a file are parsed, so the main function behaves similar to the
    one we all know from C-style languages.

It should be mentioned that the proposed changes are only intended
on making the test programs inside the test directory more like a 'real'
test suite that contains only valid programs or negative tests. See the GCC test suites (e.g.
this test)
or the Plum Hall test suites as examples.

I don't want to propose a testing framework (think xUnit) or any
other new features beside the assert module. I also don't want to
propose main as the official entry point for a non-web wasm program.

I don't have a problem with implementing the proposed changes,
so this PR will hopefully not create any additional workload for anyone.

Feedback is welcome!

NOTE: The first version proposed a _start clone as the entry point
for a test. I changed it to main as I don't think that _start is the
appropriate place to make self-checks.

@kg
Copy link
Contributor

kg commented Oct 26, 2015

Making the test assertions a part of the compiled module is mostly a distraction. They're not a part of the spec, they're host features. Exhaustively authoring tests as part of the module means that we need to introduce a lot of code into the modules to support the tests, and now the tests are a part of the module so we need exhaustive automated testing for the test framework (since it's wasm and can break, and if it breaks we're no longer actually validating whether the wasm modules work.) We also have to add new features to enable this wholesale transition - how will you write assert_trap assertions as part of a wasm module?

I don't think generating the current test syntax from code is that bad, either. It's awkward, but feasible:
https://github.com/WebAssembly/ilwasm/blob/master/third_party/tests/Puzzle.cs#L111

@ghost
Copy link

ghost commented Oct 26, 2015

Support for a debug trap operator seems fine to me. These can be very helpful in complex debug situations, and upper layers might insert them when debugging code, for example to watch an access to a specific memory location etc. The operator could be very simple and just include a text string with no arguments to limit it's effect on code generation, or optionally take some arguments.

@Teemperor
Copy link
Member Author

Woops, totally forgot to update this.

Closing this PR. If someone else has the problem handling the .wast files, then you could look at the wasm-module testsuite-converter which transforms the wast files into a set of positive/negative checks that are compatible with the standard (seems as if other implementations are doing something similar).

@JSStats there is a unreachable now that is good enough to simulate asserts

@Teemperor Teemperor closed this Nov 21, 2015
ngzhian pushed a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Mar 2, 2023
This updates the explainer text according to the new spec we agreed in
the 09-15-2020 CG meeting and discussions afterwards.

The following are modifications and clarifications we made after the
09-15-2020 CG meeting, and the relevant issue posts, if any:
https://github.com/WebAssembly/meetings/blob/master/main/2020/CG-09-15.md

- `catch_br` wasm renamed to `delegate` (WebAssembly#133)
- `rethrow` gains an immediate argument (WebAssembly#126)
- Removed dependences on the reference types proposal and the multivalue
  proposal. The multivalue proposal was previously listed as dependent
  because 1. `try` is basically a `block`, so it can have multivalue
  input/output 2. `br_on_exn` can extract multiple values from a
  `block`. We don't have `br_on_exn` anymore, and I'm not sure 1 is a
  strong enough reason to make it a dependence.
- Mention `rethrow` cannot rethrow exceptions caught by `unwind` (WebAssembly#142
  and WebAssembly#137)
- Mention some runtimes, especially web VMs, can attach stack traces to
  the exception object, implying stack traces are not required for all
  VMs
- Update label/validation rules for `delegate` and `rethrow` (WebAssembly#146)
- Finalize opcodes for `delegate` (0x18) and `catch_all` (0x19) (WebAssembly#145
  and WebAssembly#147)

I believe this resolves many previous issue threads, so I'll close them.
Please reopen them if you think there are things left for discussions in
those issues.

Resolves WebAssembly#113, resolves WebAssembly#126, resolves WebAssembly#127, resolves WebAssembly#128, resolves
WebAssembly#130, resolves WebAssembly#142, resolves WebAssembly#145, resolves WebAssembly#146, resolves WebAssembly#147.
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.

4 participants