[test] Enable test status filtering by variant

[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
Committed: https://crrev.com/03f51248226a00cefb545f0afbbb81f36671ae35
Cr-Commit-Position: refs/heads/master@{#38342}

[Michael Achenbach, 2016-08-02 12:54:04]

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

[commit-bot: I haz the power, 2016-08-02 12:54:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Achenbach, 2016-08-02 12:58:04]

Description was changed from

==========
[test] Introduce the notion of virtual test cases

BUG=
==========

to

==========
[test] Introduce the notion of virtual test cases

BUG=v8:5238
==========

[Michael Achenbach, 2016-08-02 13:09:48]

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

[commit-bot: I haz the power, 2016-08-02 13:09:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Michael Achenbach, 2016-08-02 13:16:34]

Description was changed from

==========
[test] Introduce the notion of virtual test cases

BUG=v8:5238
==========

to

==========
[test] Introduce the notion of virtual test cases

This adds the possibility to reference test cases in the
status file by a virtual name that includes the variant
under which the test is running.

Example:
'test-case': [PASS, SLOW]
'virtual/foo/test-case': [FAIL]
'virtual/bar/test-case': [SKIP]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail,
while it will be skipped in variant bar.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

[Michael Achenbach, 2016-08-02 13:19:00]

machenbach@chromium.org changed reviewers:
+ jkummerow@chromium.org, jochen@chromium.org, mstarzinger@chromium.org

[Michael Achenbach, 2016-08-02 13:19:00]

PTAL. To ease review, patch 1 doesn't move status lines, just modifies them in
place. Patch 3 moves them to their correct blocks.

[commit-bot: I haz the power, 2016-08-02 13:39:48]

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

[commit-bot: I haz the power, 2016-08-02 13:39:49]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2016-08-02 14:13:09]

I'm not overly happy with this. Why introduce a new way to express conditional
expectations? I'd find it much more intuitive to add "variant" as one of the
conditions that the status file can check on, e.g.:

['variant == "ignition_turbofan"', {
  foo/bar: [SKIP],
}],

I realize that supporting this would require some plumbing/refactoring, but it
seems like with the VariantGenerator machinery we already have a framework in
place that should be able to handle this without too much trouble. 
(Probably: TestSuite.ReadStatusFile needs to remember variant-specific outcomes,
and then FilterTestCasesByStatus and the creation of variant_tests have to be
combined into a monolithic piece of logic; which would also avoid the need to
iterate over all tests twice.)

Does the implemented approach have any particular advantage over the above?

If you stick with it, why "virtual" and not "variant"?

https://codereview.chromium.org/2203013002/diff/40001/tools/run-tests.py
File tools/run-tests.py (right):

https://codereview.chromium.org/2203013002/diff/40001/tools/run-tests.py#newc...
tools/run-tests.py:839: # (i.e. contrieved test paths containing the variant
name and the test
nit: s/contrieved/contrived/

[Michael Achenbach, 2016-08-02 14:35:21]

> I'm not overly happy with this. Why introduce a new way to express conditional
> expectations?

The 'virtual' notion is inspired by the layout test driver. It allows for more
features that this CL does not yet provide but which could be added in the
future, e.g.:
- Construct custom virtual nodes, e.g. not just variants, but other flag sets
we're passing, e.g. --isolates and others (of course we could also generalize
what a variant is )
- Have real directories mapping files that exist only under particular virtual
nodes, e.g. test cases that only exist in certain variants or test cases that
require different auxiliary files under different variants, e.g. different
webkit test expectation files.

> I'd find it much more intuitive to add "variant" as one of the
> conditions that the status file can check on, e.g.:
> 
> ['variant == "ignition_turbofan"', {
>   foo/bar: [SKIP],
> }],
> 
> I realize that supporting this would require some plumbing/refactoring, but it
> seems like with the VariantGenerator machinery we already have a framework in
> place that should be able to handle this without too much trouble. 
> (Probably: TestSuite.ReadStatusFile needs to remember variant-specific
outcomes,
> and then FilterTestCasesByStatus and the creation of variant_tests have to be
> combined into a monolithic piece of logic; which would also avoid the need to
> iterate over all tests twice.)

I chose the current approach mainly because it's a way lower hanging fruit.

> Does the implemented approach have any particular advantage over the above?

My gut feeling says it's easier to maintain because the implementation is
simple.

> If you stick with it, why "virtual" and not "variant"?

Because of layout-test parlance. We could also call it variant, I don't care.

[Jakob Kummerow, 2016-08-02 14:45:49]

On 2016/08/02 14:35:21, Michael Achenbach (slow) wrote:
> The 'virtual' notion is inspired by the layout test driver. It allows for more
> features that this CL does not yet provide but which could be added in the
> future, 

I still think it's an alternative approach to the conditional test expectations
we already have, and doesn't enable anything the latter can't do.

> My gut feeling says it's easier to maintain because the implementation is
> simple.

My gut feeling says that having several different mechanisms for similar
functionality quickly gets messy and unmaintainable (even if they're simple
individually).


That said, I don't feel strongly enough about it to block this. The test runner
is mostly for you to maintain anyway :-P

So LG TM if you find someone else who likes this approach.

[Michael Achenbach, 2016-08-02 15:11:39]

On 2016/08/02 14:45:49, Jakob wrote:
> On 2016/08/02 14:35:21, Michael Achenbach (slow) wrote:
> > The 'virtual' notion is inspired by the layout test driver. It allows for
more
> > features that this CL does not yet provide but which could be added in the
> > future, 
> 
> I still think it's an alternative approach to the conditional test
expectations
> we already have, and doesn't enable anything the latter can't do.

How about this: We could use the previous syntax (like you suggest), but still
keep reading the status file before variant creation. We'd generate the same
rules internally as in this CL using 'virtual/variant/' prefix or some other
magic name. Then we could reuse the current variant generation and status
filtering approach.

Simplest way to achieve this would be to evaluate the status file N times, once
for each variant, and each time set a different variant variable to True and
then add new rules with 'virtual/...' offsets. I guess that wouldn't have a
significant performance impact, but probably there are other approaches.

I wouldn't wanna commit to writing an AST for this purpose to allow generic
combinations like:
msan and ignition_turbofan and arch==arm
where msan and arch==arm are statically known, but ignition_turbofan must be
lazily evaluated, dependent on the variant.

[jochen (gone - plz use gerrit), 2016-08-02 15:32:31]

lgtm

I think it's preferable over the current way to add variants.

I agree that we shouldn't have two systems, but migrate variants to virtual test
suites soonish

[Michael Achenbach, 2016-08-02 15:39:36]

> I agree that we shouldn't have two systems, but migrate variants to virtual
test
> suites soonish

Not sure what you mean by the latter. Do you mean to deprecate the non-virtual
way entirely and only have e.g.:
virtual/arm64/debug/foo
or similar? There we might loose some expressiveness as we have quite a lot of
complicated conditional expressions.

[Jakob Kummerow, 2016-08-02 16:55:13]

On 2016/08/02 15:11:39, Michael Achenbach (slow) wrote:
> How about this: We could use the previous syntax (like you suggest), but still
> keep reading the status file before variant creation. We'd generate the same
> rules internally as in this CL using 'virtual/variant/' prefix or some other
> magic name. Then we could reuse the current variant generation and status
> filtering approach.
> 
> Simplest way to achieve this would be to evaluate the status file N times,
once
> for each variant, and each time set a different variant variable to True and
> then add new rules with 'virtual/...' offsets. I guess that wouldn't have a
> significant performance impact, but probably there are other approaches.

I'm a little confused.
Using our existing syntax and evaluating the status file once per variant sounds
like a reasonable compromise to me. That gives us desirable behavior right away,
and if the simple implementation does turn out to be too slow, it can be
replaced/optimized later without having to change the status files.
But that approach doesn't have anything to do with "virtual" prefixes, neither
explicit nor internally-generated, or does it?

> I wouldn't wanna commit to writing an AST for this purpose to allow generic
> combinations like:
> msan and ignition_turbofan and arch==arm
> where msan and arch==arm are statically known, but ignition_turbofan must be
> lazily evaluated, dependent on the variant.

Oh, right, that's a detail I hadn't thought of. Hm...

[Michael Achenbach, 2016-08-03 08:30:29]

Sketched a different solution in patch 4. This permits the old status file
syntax and doesn't require any status file changes. Will clean it up if that's
useful.

I allow using variant keywords in toplevel sections. In nested rules it would be
forbidden. This creates additional "virtual" rules which reuse the other logic
in this patch.

[jochen (gone - plz use gerrit), 2016-08-03 10:37:41]

On 2016/08/02 at 15:39:36, machenbach wrote:
> > I agree that we shouldn't have two systems, but migrate variants to virtual
test
> > suites soonish
> 
> Not sure what you mean by the latter. Do you mean to deprecate the non-virtual
way entirely and only have e.g.:
> virtual/arm64/debug/foo
> or similar? There we might loose some expressiveness as we have quite a lot of
complicated conditional expressions.

No, I mean the turbofan and ignition variants

[Michael Achenbach, 2016-08-03 10:40:57]

On 2016/08/03 10:37:41, jochen wrote:
> On 2016/08/02 at 15:39:36, machenbach wrote:
> > > I agree that we shouldn't have two systems, but migrate variants to
virtual
> test
> > > suites soonish
> > 
> > Not sure what you mean by the latter. Do you mean to deprecate the
non-virtual
> way entirely and only have e.g.:
> > virtual/arm64/debug/foo
> > or similar? There we might loose some expressiveness as we have quite a lot
of
> complicated conditional expressions.
> 
> No, I mean the turbofan and ignition variants

OK - that should be independent of the syntax choice here.

[Michael Achenbach, 2016-08-03 13:54:18]

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

[commit-bot: I haz the power, 2016-08-03 13:54:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 14:02:00]

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

[commit-bot: I haz the power, 2016-08-03 14:02:01]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6245)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[Michael Achenbach, 2016-08-03 14:16:22]

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

[commit-bot: I haz the power, 2016-08-03 14:16:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 14:56:10]

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

[commit-bot: I haz the power, 2016-08-03 14:56:11]

Dry run: This issue passed the CQ dry run.

[Michael Achenbach, 2016-08-03 15:21:23]

Description was changed from

==========
[test] Introduce the notion of virtual test cases

This adds the possibility to reference test cases in the
status file by a virtual name that includes the variant
under which the test is running.

Example:
'test-case': [PASS, SLOW]
'virtual/foo/test-case': [FAIL]
'virtual/bar/test-case': [SKIP]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail,
while it will be skipped in variant bar.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

to

==========
[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

[Michael Achenbach, 2016-08-03 15:22:21]

PTAL at patch 9

https://codereview.chromium.org/2203013002/diff/160001/tools/testrunner/objec...
File tools/testrunner/objects/testcase.py (right):

https://codereview.chromium.org/2203013002/diff/160001/tools/testrunner/objec...
tools/testrunner/objects/testcase.py:32: def __init__(self, suite, path,
variant=None, flags=None,
This is set when copied by variant.

[Michael Achenbach, 2016-08-03 15:22:28]

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

[commit-bot: I haz the power, 2016-08-03 15:22:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 15:43:20]

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

[commit-bot: I haz the power, 2016-08-03 15:43:21]

Dry run: This issue passed the CQ dry run.

[Jakob Kummerow, 2016-08-04 11:42:47]

lgtm

[Michael Achenbach, 2016-08-04 12:29:33]

Please also take a look at the tests in patch 10.

[Jakob Kummerow, 2016-08-04 13:08:31]

tests LGTM with comment.

https://codereview.chromium.org/2203013002/diff/180001/tools/testrunner/local...
File tools/testrunner/local/testsuite.py (right):

https://codereview.chromium.org/2203013002/diff/180001/tools/testrunner/local...
tools/testrunner/local/testsuite.py:182: t.outcomes = rules[testname]
s/=/|=/ here (cf line 195 below) should address the FIXME in the test.

[Michael Achenbach, 2016-08-04 14:12:06]

Fixed - also ensured immutable rules and wildcards in patch 12

[Jakob Kummerow, 2016-08-04 14:17:12]

still LGTM.

https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
File tools/testrunner/local/statusfile_unittest.py (right):

https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
tools/testrunner/local/statusfile_unittest.py:54: # Sanity check that we can do
the same calls on a non-fozen object.
nit: s/fozen/frozen/

https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
File tools/testrunner/local/utils.py (right):

https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
tools/testrunner/local/utils.py:143: raise Exception('Tried to mutate a fozen
dict')
nit: s/fozen/frozen/

https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
tools/testrunner/local/utils.py:146: raise Exception('Tried to mutate a fozen
dict')
nit: s/fozen/frozen/

[tandrii(chromium), 2016-08-04 14:17:26]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2016-08-04 14:17:27]

lgtm

[Michael Achenbach, 2016-08-04 14:22:11]

On 2016/08/04 14:17:12, Jakob wrote:
> still LGTM.
> 
>
https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
> File tools/testrunner/local/statusfile_unittest.py (right):
> 
>
https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
> tools/testrunner/local/statusfile_unittest.py:54: # Sanity check that we can
do
> the same calls on a non-fozen object.
> nit: s/fozen/frozen/
> 
>
https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
> File tools/testrunner/local/utils.py (right):
> 
>
https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
> tools/testrunner/local/utils.py:143: raise Exception('Tried to mutate a fozen
> dict')
> nit: s/fozen/frozen/
> 
>
https://codereview.chromium.org/2203013002/diff/220001/tools/testrunner/local...
> tools/testrunner/local/utils.py:146: raise Exception('Tried to mutate a fozen
> dict')
> nit: s/fozen/frozen/

All done

[Michael Achenbach, 2016-08-04 14:23:09]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-08-04 14:23:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
jkummerow@chromium.org, tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2203013002/#ps240001
(title: "Nits")

[commit-bot: I haz the power, 2016-08-04 14:23:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 14:41:14]

Description was changed from

==========
[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

to

==========
[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

[commit-bot: I haz the power, 2016-08-04 14:41:16]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-08-04 14:42:25]

Description was changed from

==========
[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238
==========

to

==========
[test] Enable test status filtering by variant

This adds the possibility to address test cases in the
status file with the variant under which the test is running.
This is only allowed in top-level sections.

Example:
[{
  'test-case': [PASS, SLOW],
}]

['variant == foo', {
  'test-case': [FAIL],
}]

The test case "test-case" is marked as slow in all variants.
Additionally, in variant foo, it'll be expected to fail.

This CL also exemplifies the new feature with test cases
running under the ignition_turbofan variant. The
corresponding legacy flag is deprecated.

BUG=v8:5238

Committed: https://crrev.com/03f51248226a00cefb545f0afbbb81f36671ae35
Cr-Commit-Position: refs/heads/master@{#38342}
==========

[commit-bot: I haz the power, 2016-08-04 14:42:26]

Patchset 13 (id:??) landed as
https://crrev.com/03f51248226a00cefb545f0afbbb81f36671ae35
Cr-Commit-Position: refs/heads/master@{#38342}