Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660903003:
[wasm] include JS conformance tests in Wasm mjsunit tests (Closed)
Description[wasm] include JS conformance tests in Wasm mjsunit tests
BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5507
Review-Url: https://codereview.chromium.org/2660903003
Cr-Original-Commit-Position: refs/heads/master@{#42821}
Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd3ccac
Review-Url: https://codereview.chromium.org/2660903003
Cr-Commit-Position: refs/heads/master@{#43201}
Committed: https://chromium.googlesource.com/v8/v8/+/c7eabee42207adc614324441fa2b0d8309bf8e47
Patch Set 1 #Patch Set 2 : Better failure accounting #Patch Set 3 : Added a TODO about the github repo. #
Total comments: 5
Patch Set 4 : Responding to feedback" #Patch Set 5 : Rebasing #
Total comments: 1
Patch Set 6 : Updating to latest spec tests #
Total comments: 4
Patch Set 7 : Be more selective in mjsunit.isolate #
Messages
Total messages: 34 (19 generated)
eholk@chromium.org changed reviewers: + bradnelson@chromium.org, mtrofin@chromium.org, titzer@chromium.org
This change pulls in the JS API Conformance Tests from https://github.com/bnjbvr/wasm-spec/tree/gh-pages/test/js-api and adds enough scaffolding to run them in our own system. Currently three tests are failing: 'WebAssembly.Module.customSections' method 'WebAssembly.Table.prototype.get' method 'WebAssembly.Table.prototype.set' method
https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi.js File test/mjsunit/wasm/jsapi.js (right): https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi... test/mjsunit/wasm/jsapi.js:2: // Use of this source code is governed by a BSD-style license that can be could you rename this to something like jsapi-harness.js? also, please delete the js-api.js file we've had here, as it becomes obsoleted by the official one - perhaps after one last look to make sure folks haven't introduced something new since mid last week, when I did that and pushed to the git repo our changes. https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi... test/mjsunit/wasm/jsapi.js:32: let assert_false = assertEquals.bind(null, false); Could you add a mechanic to whitelist failing tests? Something like a pair {test_id: <number>, bug_url: "https://crbug.com/<blah>}. We'd want that encoded outside the git file, since that's shared. ...but that also may mean we need a stable test ID - which may not be terrible (makes communication with partners easy: test1241 fails on your platform)
https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi.js File test/mjsunit/wasm/jsapi.js (right): https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi... test/mjsunit/wasm/jsapi.js:2: // Use of this source code is governed by a BSD-style license that can be On 2017/01/31 02:07:31, Mircea Trofin wrote: > could you rename this to something like jsapi-harness.js? > > also, please delete the js-api.js file we've had here, as it becomes obsoleted > by the official one - perhaps after one last look to make sure folks haven't > introduced something new since mid last week, when I did that and pushed to the > git repo our changes. No, please don't delete tests, even if they are redundant with spec tests. It's faster development to be able to run them independently. If they diverge, we should correct them.
https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi.js File test/mjsunit/wasm/jsapi.js (right): https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi... test/mjsunit/wasm/jsapi.js:2: // Use of this source code is governed by a BSD-style license that can be On 2017/01/31 05:00:12, titzer wrote: > On 2017/01/31 02:07:31, Mircea Trofin wrote: > > could you rename this to something like jsapi-harness.js? > > > > also, please delete the js-api.js file we've had here, as it becomes obsoleted > > by the official one - perhaps after one last look to make sure folks haven't > > introduced something new since mid last week, when I did that and pushed to > the > > git repo our changes. > > No, please don't delete tests, even if they are redundant with spec tests. It's > faster development to be able to run them independently. If they diverge, we > should correct them. I renamed jsapi.js to jsapi-harness.js, but per Ben's request, I left the ols js-api.js. https://codereview.chromium.org/2660903003/diff/40001/test/mjsunit/wasm/jsapi... test/mjsunit/wasm/jsapi.js:32: let assert_false = assertEquals.bind(null, false); On 2017/01/31 02:07:31, Mircea Trofin wrote: > Could you add a mechanic to whitelist failing tests? Something like a pair > {test_id: <number>, bug_url: "https://crbug.com/<blah>}. > > We'd want that encoded outside the git file, since that's shared. > > ...but that also may mean we need a stable test ID - which may not be terrible > (makes communication with partners easy: test1241 fails on your platform) I added known_failures, keyed on the test description. This isn't quite a good enough test id, since we have a bunch of tests called "unexpected success in assertInstantiateError". As far as a proper test ID, I think this is better addressed in the spec repo, so I opened https://github.com/WebAssembly/spec/issues/415.
lgtm
The CQ bit was checked by eholk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485889367464200,
"parent_rev": "e791ded4cdbe8c4d3f4e4a65e62f61e2b5a1c196", "commit_rev":
"eb9b5edffb8c5acb0abdff0729901f95dbd3ccac"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= ========== to ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2663063005/ by machenbach@chromium.org. The reason for reverting is: http://crbug.com/687279.
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2660903003/diff/80001/DEPS File DEPS (right): https://codereview.chromium.org/2660903003/diff/80001/DEPS#newcode46 DEPS:46: "https://github.com/bnjbvr/wasm-spec.git" + "@" + "190bd270e148a532082f6eb1b97dea5302800485", I don't know yet for sure what caused the purpleness and have no time to investigate in detail, but using github repos directly is not lgtm. Please request a chromium mirror for this so that it can be referenced by our infrastructure. I.e. file a chromium bug with the git-admin template with a mirror request.
Message was sent while issue was closed.
Description was changed from ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ========== to ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ==========
Description was changed from ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ========== to ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5507 Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ==========
PTAL Now that the tests have landed in the official spec repo, I changed DEPS to point to the chromium mirror instead. Hopefully this will fix the bots.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2660903003/diff/100001/DEPS File DEPS (right): https://codereview.chromium.org/2660903003/diff/100001/DEPS#newcode44 DEPS:44: Var("chromium_url") + "/external/github.com/WebAssembly/spec.git" + "@" + "ab1673f1e47b90471ab9352866b7682269c5b8ab", DEPS lg. Note that none of the files will be used for testing yet unless you instruct mjsunit/testcfg.py to do so. Or place files in mjsunit that refer to test/wasm-js. https://codereview.chromium.org/2660903003/diff/100001/test/mjsunit/wasm/jsap... File test/mjsunit/wasm/jsapi-harness.js (right): https://codereview.chromium.org/2660903003/diff/100001/test/mjsunit/wasm/jsap... test/mjsunit/wasm/jsapi-harness.js:58: load("test/wasm-js/test/harness/wasm-constants.js"); So for now, only those three files are used from the external repo? Could you instruct mjsunit.isolate to refer only to those three then? Otherwise the whole repo is constantly up and downloaded. Or are there more indirect dependencies?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by eholk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2660903003/diff/100001/DEPS File DEPS (right): https://codereview.chromium.org/2660903003/diff/100001/DEPS#newcode44 DEPS:44: Var("chromium_url") + "/external/github.com/WebAssembly/spec.git" + "@" + "ab1673f1e47b90471ab9352866b7682269c5b8ab", On 2017/02/11 12:27:09, Michael Achenbach wrote: > DEPS lg. Note that none of the files will be used for testing yet unless you > instruct mjsunit/testcfg.py to do so. Or place files in mjsunit that refer to > test/wasm-js. test/mjsunit/wasm/jsapi-harnes.js includes the files out of wasm-js, so the tests currently run as part of the mjsunit tests. https://codereview.chromium.org/2660903003/diff/100001/test/mjsunit/wasm/jsap... File test/mjsunit/wasm/jsapi-harness.js (right): https://codereview.chromium.org/2660903003/diff/100001/test/mjsunit/wasm/jsap... test/mjsunit/wasm/jsapi-harness.js:58: load("test/wasm-js/test/harness/wasm-constants.js"); On 2017/02/11 12:27:09, Michael Achenbach wrote: > So for now, only those three files are used from the external repo? Could you > instruct mjsunit.isolate to refer only to those three then? Otherwise the whole > repo is constantly up and downloaded. Or are there more indirect dependencies? Nope, it's just those three files. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by eholk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2660903003/#ps120001 (title: "Be more selective in mjsunit.isolate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487096627591850,
"parent_rev": "494fd7156b36213bc7718bc7177424596013f4ba", "commit_rev":
"c7eabee42207adc614324441fa2b0d8309bf8e47"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5507 Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... ========== to ========== [wasm] include JS conformance tests in Wasm mjsunit tests BUG= https://bugs.chromium.org/p/v8/issues/detail?id=5507 Review-Url: https://codereview.chromium.org/2660903003 Cr-Original-Commit-Position: refs/heads/master@{#42821} Committed: https://chromium.googlesource.com/v8/v8/+/eb9b5edffb8c5acb0abdff0729901f95dbd... Review-Url: https://codereview.chromium.org/2660903003 Cr-Commit-Position: refs/heads/master@{#43201} Committed: https://chromium.googlesource.com/v8/v8/+/c7eabee42207adc614324441fa2b0d8309b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/c7eabee42207adc614324441fa2b0d8309b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
