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

Split tests out into a separate repository? #129

Closed
sunfishcode opened this issue Oct 12, 2015 · 21 comments
Closed

Split tests out into a separate repository? #129

sunfishcode opened this issue Oct 12, 2015 · 21 comments

Comments

@sunfishcode
Copy link
Member

We want to make it as easy as possible for people developing WebAssembly implementations to run the official testsuite, because that's one of our main mechanisms for ensuring portable behavior. @Teemperor suggested on irc that we split the tests out of the spec repo into their own repo, so that they can be included as submodules in other projects.

I think this sounds like a good idea. Any objections?

@titzer
Copy link
Contributor

titzer commented Oct 12, 2015

sgtm

On Mon, Oct 12, 2015 at 10:01 AM, Dan Gohman [email protected]
wrote:

We want to make it as easy as possible for people developing WebAssembly
implementations to run the official testsuite, because that's one of our
main mechanisms for ensuring portable behavior. @Teemperor
https://github.com/Teemperor suggested on irc that we split the tests
out of the spec repo into their own repo, so that they can be included as
submodules in other projects.

I think this sounds like a good idea. Any objections?


Reply to this email directly or view it on GitHub
#129.

@lukewagner
Copy link
Member

I'm not familiar with how submodules work. Would it still be possible to have a single PR with both the ml-proto and test changes? Would it still easy to get a coherent view of ml-proto and tests at a single point in time? Instead, would it be sufficient if we moved test/ up to the top-level so that the tests felt like one symmetric part of the spec (sibling to ml-proto)?

@binji
Copy link
Member

binji commented Oct 12, 2015

You could have a single PR that updates the spec repo and bumps the test repo SHA, but that would require landing the change to the test repo prior.

I think it's better to hold off on this, or do something simpler, like mirror the test directory to a separate repo.

@Teemperor
Copy link
Member

I think just moving /test to the top level directory of the repo is fine for now (as @lukewagner and @binji said).

@sunfishcode
Copy link
Member Author

I opened #130 to move the test directory to the top level. I also renamed in testsuite to be slightly more descriptive.

@kg
Copy link
Contributor

kg commented Oct 12, 2015

PRs with dependencies across submodules are a nightmare, so I want to make it clear that we shouldn't do that unless we can't avoid it. Contributors to my personal projects have had a lot of trouble there.

Moving to the top-level sounds good for now, but it's still going to be awkward having a lot of traffic in that folder mixed with spec changes... hrm.

@jcbeyler
Copy link

Yes moving it to the top-level is really a good idea but will, as kg says, have problems for spec as it changes the definition of the tests. There is no great way of doing those kind of changes:

  • Spec changes that breaks the tests
    • Need to update both at the same time

However, not having this forces all projects to have local copies or reference the test suite via the spec repository. And I think that pro outweighs the cons even though as spec still changes things, the developers of spec will see more complications in updating their code. Now two pushes when they break a test.

Finally, it makes adding a feature without tests easier to miss because the feature and the tests are now separated.

@sunfishcode
Copy link
Member Author

Implementation developers could always pull in the entire spec repo as a submodule -- it's not all that big, and with #130 the tests will be in a toplevel directory making it a little prettier. But I'd also agree with @binji above that a standalone repo mirroring the tests would be feasible too.

@titzer
Copy link
Contributor

titzer commented Oct 12, 2015

Sounds like a mirror solves both problems, so let's do that.

On Mon, Oct 12, 2015 at 12:05 PM, Dan Gohman [email protected]
wrote:

Implementation developers could always pull in the entire spec repo as a
submodule -- it's not all that big, and with #130
#130 the tests will be in a
toplevel directory making it a little prettier. But I'd also agree with
@binji https://github.com/binji above that a standalone repo mirroring
the tests would be feasible too.


Reply to this email directly or view it on GitHub
#129 (comment).

@sunfishcode
Copy link
Member Author

I created testsuite repository here with a snapshot of the current spec tests. I have not yet set up any automatic mirroring.

@jfbastien
Copy link
Member

Is that a hand-maintained mirror? Similar to what Chrome has with DEPS files? Or am I missing a subtlety of git submodules or something arcane?

@sunfishcode
Copy link
Member Author

At the moment, it's completely manual. I just copied the files over to get things started.

@rossberg
Copy link
Member

LGTM. I added another test recently, you probably have to include it as well after rebase.

@sunfishcode
Copy link
Member Author

This is now lower-priority now that we have a mirror repository. If people think it's still useful though, I'm happy to rebase and finish it.

@Teemperor
Copy link
Member

I made an quck&dirty automatic mirror here: https://github.com/Teemperor/wasm-testsuite

@sunfishcode
Copy link
Member Author

@Teemperor We now have an official mirror; see above :-).

@drom
Copy link

drom commented Oct 22, 2015

@sunfishcode separate testsuite repo is good idea. I am using it for my parser https://github.com/drom/wast-parser

@jcbeyler
Copy link

I will be using it too for my project, I am working on something else and will move to using it in a bit

@jcbeyler
Copy link

Done as well now: https://github.com/WebAssembly/wasm-to-llvm-prototype

I added a file to reduce the tests I support internally to my project. However, when are we pulling:
#141

Because of this, I now regress (on wasm-to-llvm-prototype) on that test since my project sees the duplications.

@jcbeyler
Copy link

jcbeyler commented Nov 5, 2015

Did a PR for this:
#162

@sunfishcode
Copy link
Member Author

With an official mirror now, this isn't currently a high priority for me.

eqrion pushed a commit to eqrion/wasm-spec that referenced this issue Nov 25, 2019
* [interpreter] Simplify zero-len and drop semantics

* Update overview

* [spec] Change drop semantics

* [spec] Forgot to adjust prose for *.init ops

* [spec] Adapt to early OOB checks

* [spec] Fix OOB for table rules

* [spec] Spec memory OOB

* [spec] Extend store typing to elem and data instances

* Apply suggestions from code review

Co-Authored-By: Ryan Hunt <[email protected]>

* Comments

* [spec] Fix uses of table.set
Connicpu pushed a commit to Connicpu/wasm-spec that referenced this issue May 11, 2020
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

No branches or pull requests

10 participants