Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 5 months ago by Michael Achenbach Modified:
5 years, 4 months ago Reviewers:
Jakob Kummerow CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[test] Key variant flags by variant name everywhere.
This allows variants to be named on test failures (follow
up) and then to be used in the test runner for a repro.
This also speeds up variant iteration for test262 and fixes
a bug with variants for benchmarks.
BUG=chromium:511215
NOTREECHECKS=true
LOG=n
Committed: https://crrev.com/4fe08abde6045f09eacb7a9bc350be235671cea9
Cr-Commit-Position: refs/heads/master@{#29899}
Patch Set 1 #Patch Set 2 : builder #Patch Set 3 : #
Total comments: 10
Patch Set 4 : Experiment #Patch Set 5 : Add test262 #Patch Set 6 : Other suites + cleanup #Patch Set 7 : moar cleanup #Patch Set 8 : Typo #Patch Set 9 : Fix import #Patch Set 10 : Fix test262-es6 #
Created: 5 years, 4 months ago
Messages
Total messages: 29 (12 generated)
machenbach@chromium.org changed reviewers: + jkummerow@chromium.org
PTAL https://codereview.chromium.org/1245623005/diff/40001/test/benchmarks/testcfg.py File test/benchmarks/testcfg.py (left): https://codereview.chromium.org/1245623005/diff/40001/test/benchmarks/testcfg... test/benchmarks/testcfg.py:189: return [[], ["--turbo"]] This completely ignored the status file. The new approach will e.g. allow to limit a single benchmark to no-variants in the status file. https://codereview.chromium.org/1245623005/diff/40001/test/preparser/testcfg.py File test/preparser/testcfg.py (right): https://codereview.chromium.org/1245623005/diff/40001/test/preparser/testcfg.... test/preparser/testcfg.py:130: return lambda _: testsuite.DEFAULT_VARIANT Here, status file filtering is ignored as it doesn't make much sense with only the default variant. https://codereview.chromium.org/1245623005/diff/40001/test/test262-es6/testcf... File test/test262-es6/testcfg.py (right): https://codereview.chromium.org/1245623005/diff/40001/test/test262-es6/testcf... test/test262-es6/testcfg.py:74: This precomputes the variants, so that on iteration over the tests, no lists have to be generated. https://codereview.chromium.org/1245623005/diff/40001/test/test262-es6/testcf... test/test262-es6/testcfg.py:99: return variants.both Minor drawback: In the both-case, also after this patch it won't be possible to only rerun the exact test call. You'll be able to rerun the variant name, e.g. turbofan and the test runner will execute a nostrict and strict run. I thought about generalizing also nostrict and strict as named variants that can be combined with the others, but ran into too many potential test-runner slow-down and over-design.
This feels like a *lot* of complexity. Dynamically selected callables that return callables that return iterables? At the very least, I'd like some more descriptive naming, to make it easier to understand what's going on, see comments below. Wouldn't the following design simplify things: - let each test suite provide a default list of named variants (just the names, not the corresponding flag lists) - let the command-line args filter those default variants as needed - let each test case, by means of the status file, further filter the variants for that test as needed - once the list of variant names per test has been computed, have a translation step that translates them to a list of lists of flags. This translation could again be implemented per-testsuite to keep custom variant definitions local. https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... File tools/testrunner/local/testsuite.py (right): https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:55: def FilterVariants(variants, variant_filter): nit: VariantsFilterBuilder would be a more appropriate name, no? https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:65: def DefaultVariant(self, testcase): nit: this method and its friends below are private to this class, right? Please let their names begin with an underscore. (Don't forget to update the overrides too.) https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:79: def __call__(self, testcase): nit: instead of messing with Python object internals, can you just call this function Build(self, testcase)? https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:83: Returns: An iterable over variant tuples (name, list of flags). nit: IIUC this comment is a lie. It returns a callable that returns an iterable. Which kind of proves that this design is overly complex. https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:137: def VariantFlagsBuilder(self): nit: I don't like clashing names of unrelated things. Please either rename this to GetVariantFlagsBuilder, or the class above to DefaultVariantFlagsBuilder, or similar. https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... tools/testrunner/local/testsuite.py:141: def VariantFlags(self, variant_filter): nit: this doesn't return flags any more, but a function producing flags. Please rename accordingly. Maybe VariantFlagsGenerator or VariantFlagsFactory or something to that effect?
> Wouldn't the following design simplify things: > - let each test suite provide a default list of named variants (just the names, > not the corresponding flag lists) > - let the command-line args filter those default variants as needed > - let each test case, by means of the status file, further filter the variants > for that test as needed > - once the list of variant names per test has been computed, have a translation > step that translates them to a list of lists of flags. This translation could > again be implemented per-testsuite to keep custom variant definitions local. Yea I thought about something like that. The question is, do we want to make suite-custom variant names public in the overall runner? E.g. a turbofan variant with [--turbo, --always-opt], and a plain-turbofan variant with [--turbo]. Similar e.g. default_nostrict and default_strict etc? The last bit of your suggestion might cost a little performance on test262 if e.g. the nostrict and strict variant flags are computed for each test case (not more than currently though).
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/patch-status/1245623005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245623005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8154)
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/patch-status/1245623005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245623005/140001
PTAL. I'm back to dictionaries and implemented your suggestion of filtering names first and letting the test suite turn the names into flags by a custom method. The generator class stayed to capture the state and the precomputation in there. My measurements show that the dictionary lookups aren's significant, therefore I use now dicts and sets everywhere. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > File tools/testrunner/local/testsuite.py (right): > > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:55: def FilterVariants(variants, > variant_filter): > nit: VariantsFilterBuilder would be a more appropriate name, no? It's now called VariantsGenerator. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:65: def DefaultVariant(self, testcase): > nit: this method and its friends below are private to this class, right? Please > let their names begin with an underscore. (Don't forget to update the overrides > too.) Redesigned. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:79: def __call__(self, testcase): > nit: instead of messing with Python object internals, can you just call this > function Build(self, testcase)? Redesigned. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:83: Returns: An iterable over variant tuples > (name, list of flags). > nit: IIUC this comment is a lie. It returns a callable that returns an iterable. > Which kind of proves that this design is overly complex. Redesigned. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:137: def VariantFlagsBuilder(self): > nit: I don't like clashing names of unrelated things. Please either rename this > to GetVariantFlagsBuilder, or the class above to DefaultVariantFlagsBuilder, or > similar. This is now renamed to be a factory method. > https://codereview.chromium.org/1245623005/diff/40001/tools/testrunner/local/... > tools/testrunner/local/testsuite.py:141: def VariantFlags(self, variant_filter): > nit: this doesn't return flags any more, but a function producing flags. Please > rename accordingly. Maybe VariantFlagsGenerator or VariantFlagsFactory or > something to that effect? Renamed to CreateVariantGenerator.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
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/patch-status/1245623005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245623005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/8048)
Yes, I like this approach much better. LGTM. What's up with the v8_linux_rel bot? Looks like it doesn't have test262-es6.
On 2015/07/28 16:14:06, Jakob wrote: > Yes, I like this approach much better. LGTM. > > What's up with the v8_linux_rel bot? Looks like it doesn't have test262-es6. Worse, that's another bug that requires fixing. There's a some problem in test262-es6's testcfg and python throws an error on load. It ignores the error and uses the default google testsuite instead :/ I'll take a look...
Patchset #10 (id:180001) has been deleted
On 2015/07/28 19:52:39, Michael Achenbach wrote: > On 2015/07/28 16:14:06, Jakob wrote: > > Yes, I like this approach much better. LGTM. > > > > What's up with the v8_linux_rel bot? Looks like it doesn't have test262-es6. > > Worse, that's another bug that requires fixing. There's a some problem in > test262-es6's testcfg and python throws an error on load. It ignores the error > and uses the default google testsuite instead :/ I'll take a look... Patch 10 fixes test262-es6. I tested with test262 by mistake on my laptop...
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/patch-status/1245623005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245623005/200001
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/1245623005/#ps200001 (title: "Fix test262-es6")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1245623005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1245623005/200001
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/4fe08abde6045f09eacb7a9bc350be235671cea9 Cr-Commit-Position: refs/heads/master@{#29899} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
