Add swarming support for IsolatedScriptTest.

Add swarming support for IsolatedScriptTest.

Implement SwarmingIsolatedScriptTest.

R=phajdan.jr@chromium.org,kbr@chromium.org,sergiyb@chromium.org


BUG=507796
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=297354

[nednguyen, 2015-10-26 14:54:31]

Reviewers: I don't know how to actually test this, please teach me!

[nednguyen, 2015-10-26 15:03:50]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-26 15:03:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424693002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424693002/20001

[commit-bot: I haz the power, 2015-10-26 15:06:35]

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

[commit-bot: I haz the power, 2015-10-26 15:06:36]

Dry run: This issue passed the CQ dry run.

[Paweł Hajdan Jr., 2015-10-26 17:00:41]

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:999: if suffix == 'without
patch':  # pragma: no cover
Whoa, retrying everything would be much better than not doing anything.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1006: idempotent=None,
extra_args=args)
Why None as opposed to e.g. False?

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1014: failures = ['%s
(entire test suite)' % self.name]
I think this means valid=False.

Otherwise, if e.g. the entire test suite is broken like that on HEAD, the recipe
would start PASSING try jobs that break it even further.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1027: class
IsolatedScriptTest(Test):
What's the reason for introducing another indirection layer here?

Existing code followed this pattern because there was a use case that no longer
exists.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:696: # SwarmingTelemetryGPUTest
class in recipe_modules/chromium/steps.py. Note
Why does this refer to a specific class? There may be more callers, right?

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.py:235: yield (
Are all of these tests _strictly_ needed (i.e. we wouldn't get 100% coverage
without them)?

[Ken Russell (switch to Gerrit), 2015-10-26 18:55:43]

Couple of initial comments.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1014: failures = ['%s
(entire test suite)' % self.name]
On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> I think this means valid=False.
> 
> Otherwise, if e.g. the entire test suite is broken like that on HEAD, the
recipe
> would start PASSING try jobs that break it even further.

I'm not sure about that. Please see https://codereview.chromium.org/1372723002
which fixed a major problem where totally broken test runs were treated as
passing. I think valid needs to be True in this case.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1027: class
IsolatedScriptTest(Test):
On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> What's the reason for introducing another indirection layer here?
> 
> Existing code followed this pattern because there was a use case that no
longer
> exists.

It'd be fine IMO to remove this indirection, FWIW.

[Ken Russell (switch to Gerrit), 2015-10-26 21:29:10]

Description was changed from

==========
Add swarming support for IsolatedScripTest.

Implement SwarmingIsolatedScriptTest.

R=phajdan.jr@chromium.org,kbr@chromium.org,sergiyb@chromium.org


BUG=507796
==========

to

==========
Add swarming support for IsolatedScriptTest.

Implement SwarmingIsolatedScriptTest.

R=phajdan.jr@chromium.org,kbr@chromium.org,sergiyb@chromium.org


BUG=507796
==========

[Ken Russell (switch to Gerrit), 2015-10-26 21:35:13]

I defer to Pawel's and Sergiy's reviews. Overall the code seems fine, but I
agree with Pawel's comment about adding exactly those tests that are needed.

Corrected a couple of minor typos in the CL description.

The way to test this, I think, is to make 100% sure it's not going to regress
the existing isolated_scripts entries in src/testing/buildbot/*.json, land this,
and then send tryjobs through which change some of those entries (on the Linux,
Mac and Windows bots at first) to add the entry:

        "swarming": {
          "can_use_on_swarming_builders": true
        },

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:696: # SwarmingTelemetryGPUTest
class in recipe_modules/chromium/steps.py. Note
On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> Why does this refer to a specific class? There may be more callers, right?

I suspect this is just a leftover comment.

[nednguyen, 2015-10-27 00:54:29]

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:999: if suffix == 'without
patch':  # pragma: no cover
On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> Whoa, retrying everything would be much better than not doing anything.

But not doing anything here mean we gonna rerun all the tests?

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1006: idempotent=None,
extra_args=args)
On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> Why None as opposed to e.g. False?

Done.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1027: class
IsolatedScriptTest(Test):
On 2015/10/26 18:55:43, Ken Russell wrote:
> On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> > What's the reason for introducing another indirection layer here?
> > 
> > Existing code followed this pattern because there was a use case that no
> longer
> > exists.
> 
> It'd be fine IMO to remove this indirection, FWIW.

Done.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:696: # SwarmingTelemetryGPUTest
class in recipe_modules/chromium/steps.py. Note
On 2015/10/26 21:35:13, Ken Russell wrote:
> On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> > Why does this refer to a specific class? There may be more callers, right?
> 
> I suspect this is just a leftover comment.

Done.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.py:235: yield (
On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> Are all of these tests _strictly_ needed (i.e. we wouldn't get 100% coverage
> without them)?

I got one test removed. The rest are needed for 100% coverage.

[nednguyen, 2015-10-27 00:58:09]

https://codereview.chromium.org/1424693002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:350: idempotent=False, **kwargs):
Pawel: changing the default of this from False to None modify the test
expectation. Is idempotent=False is also the correct value to use here?

[Paweł Hajdan Jr., 2015-10-27 11:12:52]

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:999: if suffix == 'without
patch':  # pragma: no cover
On 2015/10/27 at 00:54:29, nednguyen wrote:
> On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> > Whoa, retrying everything would be much better than not doing anything.
> 
> But not doing anything here mean we gonna rerun all the tests?

Oh, I realized it's "pass" and not "return". Sorry, it makes no sense to me.

TODO sounds good, but please remove the noop "if".

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1014: failures = ['%s
(entire test suite)' % self.name]
On 2015/10/26 at 18:55:43, Ken Russell wrote:
> On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> > I think this means valid=False.
> > 
> > Otherwise, if e.g. the entire test suite is broken like that on HEAD, the
recipe
> > would start PASSING try jobs that break it even further.
> 
> I'm not sure about that. Please see https://codereview.chromium.org/1372723002
which fixed a major problem where totally broken test runs were treated as
passing.

Yup, returning no failures where there are in fact failures sounds like a bug.
In fact, we might still have some holes when e.g. exit code is non-zero but the
code claims there were no failures. Feel free to file a bug to make the code
more robust for that case.

> I think valid needs to be True in this case.

Could you explain more? Do we ever treat valid=False as success? That would
sound like a terrible bug to me.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:696: # SwarmingTelemetryGPUTest
class in recipe_modules/chromium/steps.py. Note
On 2015/10/27 at 00:54:29, nednguyen wrote:
> On 2015/10/26 21:35:13, Ken Russell wrote:
> > On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> > > Why does this refer to a specific class? There may be more callers, right?
> > 
> > I suspect this is just a leftover comment.
> 
> Done.

It's better now, but I'd still prefer these references to be removed (it's super
easy for that to get out of sync with code), and just say that *caller* should
treat None as invalid test results.

https://codereview.chromium.org/1424693002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:350: idempotent=False, **kwargs):
On 2015/10/27 at 00:58:09, nednguyen wrote:
> Pawel: changing the default of this from False to None modify the test
expectation. Is idempotent=False is also the correct value to use here?

False makes sense to me. I don't see any _existing_ expectations changing, which
is good.

[Ken Russell (switch to Gerrit), 2015-10-27 12:55:56]

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1014: failures = ['%s
(entire test suite)' % self.name]
On 2015/10/27 11:12:52, Paweł Hajdan Jr. wrote:
> On 2015/10/26 at 18:55:43, Ken Russell wrote:
> > On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> > > I think this means valid=False.
> > > 
> > > Otherwise, if e.g. the entire test suite is broken like that on HEAD, the
> recipe
> > > would start PASSING try jobs that break it even further.
> > 
> > I'm not sure about that. Please see
https://codereview.chromium.org/1372723002
> which fixed a major problem where totally broken test runs were treated as
> passing.
> 
> Yup, returning no failures where there are in fact failures sounds like a bug.
> In fact, we might still have some holes when e.g. exit code is non-zero but
the
> code claims there were no failures. Feel free to file a bug to make the code
> more robust for that case.
> 
> > I think valid needs to be True in this case.
> 
> Could you explain more? Do we ever treat valid=False as success? That would
> sound like a terrible bug to me.

Looking again at https://codereview.chromium.org/1372723002 and remembering some
of the earlier symptoms, leaving valid=true in this case will display the fact
that the entire test suite failed (due to an exception, for example) rather than
"TEST RESULTS WERE INVALID". I think it's clearer to explicitly say that the
entire test suite failed, but don't feel that strongly about it.

[nednguyen, 2015-10-27 16:36:48]

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:999: if suffix == 'without
patch':  # pragma: no cover
On 2015/10/27 11:12:52, Paweł Hajdan Jr. wrote:
> On 2015/10/27 at 00:54:29, nednguyen wrote:
> > On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> > > Whoa, retrying everything would be much better than not doing anything.
> > 
> > But not doing anything here mean we gonna rerun all the tests?
> 
> Oh, I realized it's "pass" and not "return". Sorry, it makes no sense to me.
> 
> TODO sounds good, but please remove the noop "if".

Done.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium_tests/steps.py:1014: failures = ['%s
(entire test suite)' % self.name]
On 2015/10/27 12:55:55, Ken Russell wrote:
> On 2015/10/27 11:12:52, Paweł Hajdan Jr. wrote:
> > On 2015/10/26 at 18:55:43, Ken Russell wrote:
> > > On 2015/10/26 17:00:40, Paweł Hajdan Jr. wrote:
> > > > I think this means valid=False.
> > > > 
> > > > Otherwise, if e.g. the entire test suite is broken like that on HEAD,
the
> > recipe
> > > > would start PASSING try jobs that break it even further.
> > > 
> > > I'm not sure about that. Please see
> https://codereview.chromium.org/1372723002
> > which fixed a major problem where totally broken test runs were treated as
> > passing.
> > 
> > Yup, returning no failures where there are in fact failures sounds like a
bug.
> > In fact, we might still have some holes when e.g. exit code is non-zero but
> the
> > code claims there were no failures. Feel free to file a bug to make the code
> > more robust for that case.
> > 
> > > I think valid needs to be True in this case.
> > 
> > Could you explain more? Do we ever treat valid=False as success? That would
> > sound like a terrible bug to me.
> 
> Looking again at https://codereview.chromium.org/1372723002 and remembering
some
> of the earlier symptoms, leaving valid=true in this case will display the fact
> that the entire test suite failed (due to an exception, for example) rather
than
> "TEST RESULTS WERE INVALID". I think it's clearer to explicitly say that the
> entire test suite failed, but don't feel that strongly about it.

Set "valid=False" here.

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/1424693002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:696: # SwarmingTelemetryGPUTest
class in recipe_modules/chromium/steps.py. Note
On 2015/10/27 11:12:52, Paweł Hajdan Jr. wrote:
> On 2015/10/27 at 00:54:29, nednguyen wrote:
> > On 2015/10/26 21:35:13, Ken Russell wrote:
> > > On 2015/10/26 17:00:41, Paweł Hajdan Jr. wrote:
> > > > Why does this refer to a specific class? There may be more callers,
right?
> > > 
> > > I suspect this is just a leftover comment.
> > 
> > Done.
> 
> It's better now, but I'd still prefer these references to be removed (it's
super
> easy for that to get out of sync with code), and just say that *caller* should
> treat None as invalid test results.

Done.

[nednguyen, 2015-10-27 16:37:27]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-27 16:37:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424693002/60001

[commit-bot: I haz the power, 2015-10-27 16:43:30]

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

[commit-bot: I haz the power, 2015-10-27 16:43:31]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2015-10-27 16:52:45]

Looks good from my standpoint.

[Paweł Hajdan Jr., 2015-10-28 13:21:40]

LGTM

[nednguyen, 2015-10-28 13:21:52]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-10-28 13:22:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424693002/60001

[commit-bot: I haz the power, 2015-10-28 13:25:08]

Committed patchset #4 (id:60001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=297354