[test] Key variant flags by variant name everywhere.

[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}

[Michael Achenbach, 2015-07-27 12:37:45]

machenbach@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Michael Achenbach, 2015-07-27 12:37:46]

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.

[Jakob Kummerow, 2015-07-27 14:36:54]

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?

[Michael Achenbach, 2015-07-27 15:05:04]

> 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).

[Michael Achenbach, 2015-07-28 12:28:52]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 12:28:58]

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

[commit-bot: I haz the power, 2015-07-28 12:30:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 12:30:48]

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)

[Michael Achenbach, 2015-07-28 12:33:12]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 12:33:22]

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

[Michael Achenbach, 2015-07-28 12:41:17]

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.

[commit-bot: I haz the power, 2015-07-28 12:43:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 12:43:36]

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...)

[Michael Achenbach, 2015-07-28 12:48:23]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 12:48:34]

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

[commit-bot: I haz the power, 2015-07-28 13:10:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-28 13:10:47]

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)

[Jakob Kummerow, 2015-07-28 16:14:06]

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.

[Michael Achenbach, 2015-07-28 19:52:39]

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...

[Michael Achenbach, 2015-07-28 21:13:14]

Patchset #10 (id:180001) has been deleted

[Michael Achenbach, 2015-07-28 21:14:09]

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...

[Michael Achenbach, 2015-07-28 21:14:16]

The CQ bit was checked by machenbach@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-28 21:14:29]

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

[Michael Achenbach, 2015-07-29 07:11:47]

The CQ bit was unchecked by machenbach@chromium.org

[Michael Achenbach, 2015-07-29 07:12:02]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2015-07-29 07:12:03]

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")

[commit-bot: I haz the power, 2015-07-29 07:12:13]

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

[commit-bot: I haz the power, 2015-07-29 07:14:21]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-07-29 07:14:39]

Patchset 10 (id:??) landed as
https://crrev.com/4fe08abde6045f09eacb7a9bc350be235671cea9
Cr-Commit-Position: refs/heads/master@{#29899}