Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Factor reference tests for the proposal to avoid confusion #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lars-t-hansen
Copy link
Collaborator

As discussed in the context of the proposal for the sign extension ops, we should keep tests introduced by a proposal in separate files until the proposal is stable, so that it is easier for implementations to handle the new tests without having to deal with any other changes to the test file where the tests eventually belong.

This patch factors the tests for the exportable/importable mutable globals into separate files, core/globals_proposed.wast and core/linking_proposed. wast. Eventually these should be merged into globals.wast and linking.wast, or renamed in some way to remove the "_proposed" suffix.

The present proposal has a unique challenge in that it actually invalidates existing semantics in a corner case, so some existing tests have to be removed (from globals.wast). I have opted here to comment them out, as documentation, so that they can be removed eventually when the proposal has been approved.

@lars-t-hansen lars-t-hansen requested a review from binji April 9, 2018 11:17
@rossberg
Copy link
Member

rossberg commented Apr 9, 2018

I'm not actually convinced this is a good idea. Depending on the proposal, separating out new tests can be rather unnatural and tedious and create unnecessary work later to properly clean up.

If I understand correctly, the motivation for this is being able to merge test suites. But what for? Why not simply run the test suite from any proposal repo you want to track as is? IIRC, that's what V8 does. It's not like it concerns many repos, or that running the suite takes a lot of time, so the redundancy in doing so doesn't seem like a big deal.

@lars-t-hansen
Copy link
Collaborator Author

I'm not actually convinced this is a good idea. Depending on the proposal, separating out new tests can be rather unnatural and tedious and create unnecessary work later to properly clean up.

In cases other than for globals the test cases should be additive, no existing tests should need to be modified.

If I understand correctly, the motivation for this is being able to merge test suites.

The desire is to import test cases for proposals into existing CI infrastructure without having to take whatever unrelated changes those files have due to being forked earlier / updated later than the files already imported into the CI infra.

But what for? Why not simply run the test suite from any proposal repo you want to track as is?

Because of existing CI infrastructure that does not run ocaml and for which we do not want to deal with running the test interpreter. We have an import of the test suite in roughly binary form (the intermediate form produced by the interpreter for running on a JS engine). We'd like to keep it this way.

@rossberg
Copy link
Member

rossberg commented Apr 9, 2018

In cases other than for globals the test cases should be additive, no existing tests should need to be modified.

Not sure how you come to this conclusion. Any proposal that is not just the addition of new instructions will likely cross-cut various existing tests. For example, the tests for multi-values or for reference types do.

Because of existing CI infrastructure that does not run ocaml

I don't understand how that's related. You have to convert the additional tests somehow. At that point, why would it be any more difficult to simply convert all tests?

@binji
Copy link
Member

binji commented May 10, 2018

Coming back to this -- was this previously discussed at a CG meeting? If not, maybe we should bring it up at the next one.

@lars-t-hansen
Copy link
Collaborator Author

I don't think this was discussed. My fault. I have a note on my TODO list: "Followup with AR", without a subject... must have been this one. @rossberg makes a good point about proposals generally impacting existing tests when they remove restrictions.

I persist in thinking that we would very much like to take test cases for individual proposals into our test suite, we never want to "convert all tests". The tests contain test cases for features we don't implement; the tests have bugs or choose other interpretations than we did where the interpretation is open; we have bugs that we can't fix yet and aren't allowed to land test cases that will regress test results. Really, we maintain a fork of the test repo and the proposal here is just to make it easier to pick test cases for in-development features without burning a lot of manpower on "taking everything".

(How this will fit in with the move to WPT and its proposed two-way sync I don't know.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants