Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658933004:
[SAB] Test262 Agent harness (Closed)
|
Created:
3 years, 10 months ago by binji Modified:
3 years, 10 months ago Reviewers:
Dan Ehrenberg CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[SAB] Test262 Agent harness
Review-Url: https://codereview.chromium.org/2658933004
Cr-Commit-Position: refs/heads/master@{#43357}
Committed: https://chromium.googlesource.com/v8/v8/+/8235558bd0db43818dac5be9794340e5b7ed9e57
Patch Set 1 #
Total comments: 8
Patch Set 2 : merge master + fixes #Patch Set 3 : Add harness-agent.js to test262.isolate #
Total comments: 2
Patch Set 4 : merge master + address comments #Patch Set 5 : skip allocation-limit and length-is-too-large-throws for SharedArrayBuffer too #
Messages
Total messages: 28 (18 generated)
binji@chromium.org changed reviewers: + littledan@chromium.org
Mind taking a look (or redirecting to someone who can)? Thanks!
I'm happy to see that it wasn't too onerous to get the harness working. https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.js File test/test262/harness-agent.js (right): https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.... test/test262/harness-agent.js:8: var i32a = new Int32Array(new SharedArrayBuffer(256)); Would it make sense to avoid creating this array until anyone calls start()? Maybe that would minimize the performance regression in test262 (which is already one of the longer-running test suites), especially if this harness file is run for every single test, even those that don't use SharedArrayBuffer. https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.... test/test262/harness-agent.js:40: ${script} Eh, to be pedantic, this doesn't really do exactly what the description says; the "script" is not run in top-level script context. But maybe it's fine to wait to fix this when someone checks in a test262 test that fails because of it; plenty of other tests exist to test top-level script-ness, and I don't see why you'd want to test that in conjunction with agents. If you wanted to get a little closer without much work, you could do an indirect eval here. https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:119: os.path.join(self.root, "harness-agent.js")] It would be nice if you could avoid including this file for tests that don't need the SharedArrayBuffer test infrastructure, especially if it allocates a SharedArrayBuffer unconditionally, but even if it doesn't, as test262 is already relatively slow to run and loading another script could make it slower. This is what is done below for detachArrayBuffer.js. It sounds like the logic could be, "if it's in built-ins/Atomics, built-ins/SharedArrayBuffer, or has the SharedArrayBuffer feature tag, you need harness-agent.js". https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:155: (["--harmony-sharedarraybuffer"]) + It would be nice to avoid passing this flag for tests that don't use SharedArrayBuffer. It's better to be running the test262 tests in something close to the production configuration, generally, except for in-progress features. Could you instead put the references to "--harmony-sharedarraybuffer" in test262.status for the tests that should have the flag passed? To make this work, you'd actually either have to pull in the test262 tests locally or wait until they are committed upstream and do a test262 roll. It'd be ideal if the infrastructure is checked in together with running the tests, so we actually get coverage of both. For more information on local test262 tests and per-test flags, see https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5... . Note that, in the status file, you can list a whole subdirectory of tests as needing a flag by doing something like "built-ins/Atomics/*": ["--harmony-sharedarraybuffer"]
Now that the test262 roll has been done, it might be practical to rebase and land this change.
On 2017/02/14 14:52:45, Dan Ehrenberg wrote: > Now that the test262 roll has been done, it might be practical to rebase and > land this change. Agreed, I have been sidetracked w/ other stuff but I'll get back to this soon.
Looks like there are a few failures. Strange because I don't remember these failing the last time I tried. But maybe I just didn't look closely enough. https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.js File test/test262/harness-agent.js (right): https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.... test/test262/harness-agent.js:8: var i32a = new Int32Array(new SharedArrayBuffer(256)); On 2017/01/28 03:41:06, Dan Ehrenberg wrote: > Would it make sense to avoid creating this array until anyone calls start()? > Maybe that would minimize the performance regression in test262 (which is > already one of the longer-running test suites), especially if this harness file > is run for every single test, even those that don't use SharedArrayBuffer. Done. https://codereview.chromium.org/2658933004/diff/1/test/test262/harness-agent.... test/test262/harness-agent.js:40: ${script} On 2017/01/28 03:41:06, Dan Ehrenberg wrote: > Eh, to be pedantic, this doesn't really do exactly what the description says; > the "script" is not run in top-level script context. But maybe it's fine to wait > to fix this when someone checks in a test262 test that fails because of it; > plenty of other tests exist to test top-level script-ness, and I don't see why > you'd want to test that in conjunction with agents. > > If you wanted to get a little closer without much work, you could do an indirect > eval here. Done. https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:119: os.path.join(self.root, "harness-agent.js")] On 2017/01/28 03:41:06, Dan Ehrenberg wrote: > It would be nice if you could avoid including this file for tests that don't > need the SharedArrayBuffer test infrastructure, especially if it allocates a > SharedArrayBuffer unconditionally, but even if it doesn't, as test262 is already > relatively slow to run and loading another script could make it slower. This is > what is done below for detachArrayBuffer.js. It sounds like the logic could be, > "if it's in built-ins/Atomics, built-ins/SharedArrayBuffer, or has the > SharedArrayBuffer feature tag, you need harness-agent.js". Done. https://codereview.chromium.org/2658933004/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:155: (["--harmony-sharedarraybuffer"]) + On 2017/01/28 03:41:06, Dan Ehrenberg wrote: > It would be nice to avoid passing this flag for tests that don't use > SharedArrayBuffer. It's better to be running the test262 tests in something > close to the production configuration, generally, except for in-progress > features. Could you instead put the references to "--harmony-sharedarraybuffer" > in test262.status for the tests that should have the flag passed? > > To make this work, you'd actually either have to pull in the test262 tests > locally or wait until they are committed upstream and do a test262 roll. It'd be > ideal if the infrastructure is checked in together with running the tests, so we > actually get coverage of both. For more information on local test262 tests and > per-test flags, see > https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5... > . Note that, in the status file, you can list a whole subdirectory of tests as > needing a flag by doing something like "built-ins/Atomics/*": > ["--harmony-sharedarraybuffer"] Done.
The CQ bit was checked by binji@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
The CQ bit was checked by binji@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
https://codereview.chromium.org/2658933004/diff/20002/test/test262/test262.st... File test/test262/test262.status (right): https://codereview.chromium.org/2658933004/diff/20002/test/test262/test262.st... test/test262/test262.status:574: 'built-ins/TypedArrays/*': ['--harmony-sharedarraybuffer'], Maybe this is an excessively pedantic request, but I'm generally trying to keep the test262.status file passing flags only where it's required, and keeping tests for shipping features running without passing any additional flags. Could you append the --harmony-sharedarraybuffer flag only for the tests which are listed above, rather than so many other existing DataView, ArrayBuffer and TypedArray tests?
https://codereview.chromium.org/2658933004/diff/20002/test/test262/test262.st... File test/test262/test262.status (right): https://codereview.chromium.org/2658933004/diff/20002/test/test262/test262.st... test/test262/test262.status:574: 'built-ins/TypedArrays/*': ['--harmony-sharedarraybuffer'], On 2017/02/20 21:11:56, Dan Ehrenberg wrote: > Maybe this is an excessively pedantic request, but I'm generally trying to keep > the test262.status file passing flags only where it's required, and keeping > tests for shipping features running without passing any additional flags. Could > you append the --harmony-sharedarraybuffer flag only for the tests which are > listed above, rather than so many other existing DataView, ArrayBuffer and > TypedArray tests? Done.
The CQ bit was checked by binji@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...
The CQ bit was checked by binji@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...
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 binji@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": 60001, "attempt_start_ts": 1487707044018520,
"parent_rev": "af76645baac4ba920b97dd55cbdc1df61ff31a39", "commit_rev":
"8235558bd0db43818dac5be9794340e5b7ed9e57"}
Message was sent while issue was closed.
Description was changed from ========== [SAB] Test262 Agent harness ========== to ========== [SAB] Test262 Agent harness Review-Url: https://codereview.chromium.org/2658933004 Cr-Commit-Position: refs/heads/master@{#43357} Committed: https://chromium.googlesource.com/v8/v8/+/8235558bd0db43818dac5be9794340e5b7e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:60001) as https://chromium.googlesource.com/v8/v8/+/8235558bd0db43818dac5be9794340e5b7e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
