Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601393002:
[test] Allow command-line flags to be turned on per test262 test (Closed)
|
Created:
3 years, 11 months ago by Dan Ehrenberg Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[test] Allow command-line flags to be turned on per test262 test
This patch changes the test262 infrastructure to pass individual flags,
specified in the status file, for tests for experimental features, rather
than passing --harmony for all runs. With this change, it should be
easier to run test262 tests in automation when developing new features.
The new workflow would be, when adding a flag, include the flag in the
test expectations file, and when removing the flag, remove the lines from
the test expectations file. This way, the status file does not have to
change when staging or unstaging, and you get the benefit of the automated
tests before staging starts.
R=adamk
CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng
Review-Url: https://codereview.chromium.org/2601393002
Cr-Commit-Position: refs/heads/master@{#42249}
Committed: https://chromium.googlesource.com/v8/v8/+/f62f846cd37be72339436ba2fbad811eb15ceee5
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by littledan@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...
Description was changed from ========== [test262] Allow command-line flags to be turned on per test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk ========== to ========== [test262] Allow command-line flags to be turned on per test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
adamk@chromium.org changed reviewers: + machenbach@chromium.org
I like the idea here, but I'd like machenbach to review this as it's something of a change to the way our status files look (this ties them more directly to V8 flags, rather than test-runner features like "variants"). Realize he's OOO, but I think he's back Friday.
lgtm with comments. The caveat might be that there are some code smells lurking in the way status files merge outcomes among different sections and between glob-rules and specific rules. I'd be fine with using test262 as a testing ground for this in practice. Please check that the tests are indeed executed as expected (I assume they'd fail if the flags aren't passed right?). https://codereview.chromium.org/2601393002/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/2601393002/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:143: ([flag for flag in testcase.outcomes if flag.startswith("--")])) Maybe add an access method for this in testcase. Also for the case below, i.e. one method for getting flag outcomes and one for non-flag outcomes. Storing it in outcomes is a neat trick to reuse the status-file logic.
I have verified that the flags are actually being turned on--the way that I figured out the test262.status change was to run the tests with the new testcfg.py and fix the errors that came up (due to --harmony being removed); they pass now because the logic works. It is true that the new flags logic does not mesh well with the existing expressivity of the status files. For example, there's no way to say, "pass this flag to the test and check whether it fails". I couldn't think of a particular immediate need for that in the test262/new feature development case, and I also wasn't sure what the syntax should be. The Blink 'virtual test suites' give a cleaner model; I think that'd be a good direction to go in if we feel pain from the ad-hoc nature of this setup, but virtual test suites are more work to use than this. Let me know if this seems good to commit as is, or if you'd like further changes. https://codereview.chromium.org/2601393002/diff/1/test/test262/testcfg.py File test/test262/testcfg.py (right): https://codereview.chromium.org/2601393002/diff/1/test/test262/testcfg.py#new... test/test262/testcfg.py:143: ([flag for flag in testcase.outcomes if flag.startswith("--")])) On 2017/01/05 13:34:42, Michael Achenbach (OOO) wrote: > Maybe add an access method for this in testcase. Also for the case below, i.e. > one method for getting flag outcomes and one for non-flag outcomes. Storing it > in outcomes is a neat trick to reuse the status-file logic. I'm a little reluctant to go and touch testcase, when currently, this patch is test262-only logic. I like your idea of thinking of this as a testing ground; if this is extended to other test suites, I'll do refactoring cleanups like that.
Description was changed from ========== [test262] Allow command-line flags to be turned on per test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng ========== to ========== [test] Allow command-line flags to be turned on per test262 test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng ==========
Ack, lgtm
The CQ bit was checked by littledan@chromium.org
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
Try jobs failed on following builders: 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_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15109)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2601393002/#ps20001 (title: "Rebase")
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": 20001, "attempt_start_ts": 1484184568827700,
"parent_rev": "785cedf1ee6671399c2b2b7f6c6a8119f55195b4", "commit_rev":
"f62f846cd37be72339436ba2fbad811eb15ceee5"}
Message was sent while issue was closed.
Description was changed from ========== [test] Allow command-line flags to be turned on per test262 test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng ========== to ========== [test] Allow command-line flags to be turned on per test262 test This patch changes the test262 infrastructure to pass individual flags, specified in the status file, for tests for experimental features, rather than passing --harmony for all runs. With this change, it should be easier to run test262 tests in automation when developing new features. The new workflow would be, when adding a flag, include the flag in the test expectations file, and when removing the flag, remove the lines from the test expectations file. This way, the status file does not have to change when staging or unstaging, and you get the benefit of the automated tests before staging starts. R=adamk CQ_INCLUDE_TRYBOTS=master.tryserver.v8:v8_linux_noi18n_rel_ng Review-Url: https://codereview.chromium.org/2601393002 Cr-Commit-Position: refs/heads/master@{#42249} Committed: https://chromium.googlesource.com/v8/v8/+/f62f846cd37be72339436ba2fbad811eb15... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/f62f846cd37be72339436ba2fbad811eb15... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
