Add recipe for split AMP/local CQ.

Add recipe for split AMP/local CQ.

Add a new recipe that triggers AMP test suites, runs local non-AMP tests, and
then collects AMP test results.

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

[navabi, 2015-04-22 16:16:47]

navabi@google.com changed reviewers:
+ jbudorick@chromium.org, luqui@chromium.org, navabi@google.com,
phajdan@google.com

[navabi, 2015-04-22 16:16:48]

This is an initial and incomplete patch illustrating the approach to adding
split amp testing to the android try bot. The idea is to trigger AMP-supported
test suites, and then while they run, run the non-AMP-supported test suits, and
finally collect the AMP-supported test results after the local tests have run.

If AMP tests fail, we will run them at the end locally.

Pawel and Luke, please review how I intend to change the test_utils module and
the chromium_trybot recipe.

John, when I get back, I'll come sit with you to ask you questions on how to use
the AMP api. I have some questions on getting the results from the AMP bots.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/chromium_linux.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/chromium_linux.py:291: 'Android Tests (amp
split)': {
This will first be an FYI bot to test the split AMP recipe.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/chromium_linux.py:302: 'amp_tests': [
amp_tests need to be specified separately, because they are not run sequentially
like the other tests. They are triggered, the other tests are run sequentially,
and then the amp test results are collected.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:664: # Need to return step_result
here
need to figure out how to return the step result here. This needs to return the
same info that the LocalGTestTest::run function returns. (help from jbudorick@)

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:667: self._trigger(api, suffix)
I will change this to return self._local_tests.run(). Thus, the AMPGTestTest.run
method can be used to run the test sequentially as if it was not an AMP test.
This will be used when triggering failed (e.g. because devices are not
available) or when we are re-running a test without the patch.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
File
scripts/slave/recipes/chromium.expected/full_chromium_linux_Android_Tests__amp_split_.json
(right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium.expected/full_chromium_linux_Android_Tests__amp_split_.json:444:
},
need to figure out why this recipe is not triggering amp tests.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.py:369: amp_tests =
list(bot_config.get('amp_tests', []))
This isn't doing what I expected for the 'Android Tests (amp split)' bot. From
the expectations, it looks like it is coming back with an empty list.

[Paweł Hajdan Jr., 2015-04-22 16:25:09]

phajdan.jr@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2015-04-22 16:25:10]

I'm glad this is using the chromium and chromium_trybot recipes.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/chromium_linux.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/chromium_linux.py:291: 'Android Tests (amp
split)': {
On 2015/04/22 at 16:16:48, navabi wrote:
> This will first be an FYI bot to test the split AMP recipe.

Why not chromium_fyi.py in that case? I don't think we should go directly to
main waterfall with this.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:667: self._trigger(api, suffix)
On 2015/04/22 at 16:16:48, navabi wrote:
> I will change this to return self._local_tests.run(). Thus, the
AMPGTestTest.run method can be used to run the test sequentially as if it was
not an AMP test. This will be used when triggering failed (e.g. because devices
are not available) or when we are re-running a test without the patch.

Please take a look at how swarming tests run. There are pre_run and post_run
hooks, which can be used for trigger and collect respectively. We don't fallback
to local runs if trigger fails.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/test_utils/api.py:69: amp_tests - iterable of
objects of type AMPGTestTest
This shouldn't be needed - see my comment in steps.py

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipes/chrom...
scripts/slave/recipes/chromium_trybot.py:369: amp_tests =
list(bot_config.get('amp_tests', []))
On 2015/04/22 at 16:16:48, navabi wrote:
> This isn't doing what I expected for the 'Android Tests (amp split)' bot. From
the expectations, it looks like it is coming back with an empty list.

That's because bot_config here refers to the trybot. Looks like there is no
trybot configured to mirror 'Android Tests (amp split)', and even then, no
special logic should be needed in this file.

[jbudorick, 2015-04-22 16:35:40]

I had more comments, but they basically boiled down to "this should more closely
mirror swarming, since the trigger/collect mechanism is very similar," which
Pawel covered.

I hadn't thought about using this on the main waterfall, but I agree that the
deployment plan should be:
 1. fyi bot
 2. main waterfall + CQ

since I know we want CQ + main waterfall to behave the same way.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:652: deffered_step_result =
api.amp.collect_test_suite(
deferred_step_result

I'm somewhat surprised that this made it through simulation, since you spelled
it correctly below.

[navabi, 2015-05-07 19:21:59]

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/chromium_linux.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/chromium_linux.py:291: 'Android Tests (amp
split)': {
On 2015/04/22 16:25:09, Paweł Hajdan Jr. wrote:
> On 2015/04/22 at 16:16:48, navabi wrote:
> > This will first be an FYI bot to test the split AMP recipe.
> 
> Why not chromium_fyi.py in that case? I don't think we should go directly to
> main waterfall with this.

Done.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:652: deffered_step_result =
api.amp.collect_test_suite(
On 2015/04/22 16:35:40, jbudorick wrote:
> deferred_step_result
> 
> I'm somewhat surprised that this made it through simulation, since you spelled
> it correctly below.

Done.

https://codereview.chromium.org/1104533002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/chromium/steps.py:667: self._trigger(api, suffix)
On 2015/04/22 16:25:09, Paweł Hajdan Jr. wrote:
> On 2015/04/22 at 16:16:48, navabi wrote:
> > I will change this to return self._local_tests.run(). Thus, the
> AMPGTestTest.run method can be used to run the test sequentially as if it was
> not an AMP test. This will be used when triggering failed (e.g. because
devices
> are not available) or when we are re-running a test without the patch.
> 
> Please take a look at how swarming tests run. There are pre_run and post_run
> hooks, which can be used for trigger and collect respectively. We don't
fallback
> to local runs if trigger fails.

Done.

[navabi, 2015-05-07 20:02:50]

This does not pass the recipe_simulation_tests.py:

  File
"/usr/local/google/code/buildbot/build/scripts/slave/recipe_modules/chromium/steps.py",
line 727, in pre_run
    return self._test.pre_run(api, suffix)
  File
"/usr/local/google/code/buildbot/build/scripts/slave/recipe_modules/chromium/steps.py",
line 656, in pre_run
    amp_arguments = api.amp.amp_arguments(
  File "/usr/local/google/code/buildbot/build/scripts/slave/recipe_util.py",
line 39, in __getattr__
    "RecipeApi has no dependency %r. (Add it to DEPS?)" % (key,))
ModuleInjectionError: RecipeApi has no dependency 'amp'. (Add it to DEPS?)

The reason is because I'm using the amp recipe_module in chromium/steps.py and I
don't have it in the DEPS of chromium/__init__.py.

But when I put it in the DEPS, I get the following:
  File "/usr/local/google/code/buildbot/build/scripts/slave/recipe_loader.py",
line 109, in <dictcomp>
    return { dep: self.load(NamedDependency(dep)) for dep in deps }
  File "/usr/local/google/code/buildbot/build/scripts/slave/recipe_loader.py",
line 99, in load
    'Cyclic dependency when trying to load %s' % name)
AssertionError: Cyclic dependency when trying to load
/usr/local/google/code/buildbot/build/scripts/slave/recipe_modules/chromium

Because amp has the chromium module in its DEPS.

luqui@, what do you suggest here? Trying to extract stuff out of the amp module
or removing this restriction on circular dependencies of modules?

[navabi, 2015-05-07 20:16:49]

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:656: amp_arguments =
api.amp.amp_arguments(
this is one reason i need the amp module here.

[jbudorick, 2015-05-07 20:31:47]

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:287:
api.chromium_android.run_test_suite(self.target_name, **kwargs)
chromium.steps already uses chromium_android, which also depends on chromium.
I'm not quite sure how -- chromium_android isn't in chromium.__init__ either --
but maybe we can emulate that?

[Paweł Hajdan Jr., 2015-05-08 08:55:11]

Please generate recipe expectations for this change.

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
Why do we need this fallback logic? We don't do this anywhere else.

If it's really needed, why not perform the local run in the "run" method as
opposed to "post_run"? That'd be more consistent with how other tests are
running.

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:710: elif amp_test:
Do we need to modify GTestTest? Why wouldn't callers just use AMPGTestTest
directly?

[navabi, 2015-05-18 15:51:49]

The reason I did not generate expectations is because of the recipe module
dependency problem I explained. As soon as I find a solution to that, I'll
generate the expectations.

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:287:
api.chromium_android.run_test_suite(self.target_name, **kwargs)
On 2015/05/07 20:31:46, jbudorick wrote:
> chromium.steps already uses chromium_android, which also depends on chromium.
> I'm not quite sure how -- chromium_android isn't in chromium.__init__ either
--
> but maybe we can emulate that?

Thanks. I'll take a look and see if I can figure it out.

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> Why do we need this fallback logic? We don't do this anywhere else.

It is needed. We need this fallback logic because AMP may not have devices
available. When that is the case, the AMP tests, like the non-AMP tests, will
need to run locally. This will allow us to use AMP without requiring their lab
to immediately have full capacity.

The comment was meant to explain this. Let me know if you want more details in
the comment.

> If it's really needed, why not perform the local run in the "run" method as
> opposed to "post_run"? That'd be more consistent with how other tests are
> running.

We only want to run locally if we have triggered the AMP job, waiting for
devices to become available, done everything else we need to do (i.e. completed
the non-AMP tests, and find that the AMP tests were unable to run. In this case,
we run locally?

Are you suggesting moving the return statement on line 677 into the local run
method and then on line 677 just calling the local run method?

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:710: elif amp_test:
On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> Do we need to modify GTestTest? Why wouldn't callers just use AMPGTestTest
> directly?

This was to make it like SwarmingGTestTest. Notice how the callers dont just use
SwarmingGTestTest directly. In fact, my original patch used the AMPGTestTest
directly, but in this patch I made it more like the swarming tests.

[navabi, 2015-06-05 01:36:22]

Luke, can you PTAL and let me know if you can tell why recipe_simulation_test.py
is missing coverage.

I added stuff to example.py which explicitly calls pre_run. While I can
understand the lines for post_run, I'm not sure why pre_run is not covered.

https://codereview.chromium.org/1104533002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/example.py (right):

https://codereview.chromium.org/1104533002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/example.py:31: test.pre_run(api, '')
I don't know why I keep getting the recipe_simulation_test.py failure below:


Trained 2041 tests in 18.217s (ran 2041 tests)

Coverage Report                                                                 
                               Stmts   Miss  Cover   Missing
---------------------------------------------------------------------------------------------------------------------------------------------
/usr/local/google/code/buildbot/build/scripts/slave/recipe_modules/chromium/steps
                                571      7    99%   660-667, 674-687
---------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                           
                               16558      7    99%   

FATAL: Test coverage is not at 100%.


The lines 660-667 correspond to the pre_run function in AMPGTestTest, and here
I'm calling pre_run.

(Note: Before I had the steps the chromium recipes performs, but I was seeing
this error, so I made an explicit call to pre_run and still fails for coverage
of those lines).

[navabi, 2015-06-06 00:14:01]

PTAL

luqui@ helped me get the expectations right (thanks luqui@). The expectation
looks good (i.e. triggers amp tests, installs on local devices, runs local tests
and then collects amp test results before finishing).

[jbudorick, 2015-06-06 00:39:20]

Really like how this looks.

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/chromium_fyi.py (right):

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1525: device_name=['Nexus
5'], device_os=['4.4.2', '4.4.3'],
All of the current devices are 4.4.2, so I think we can either just use that or
drop this kwarg altogether.

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1526: device_timeout=60),
Also, I'm not sure about using a device timeout. If one step hits it, all of the
rest of them are going to hit it, too, at which point we've added n minutes to
the cycle time before falling back on local devices.

This will especially be the case if we start out with so many suites.

[navabi, 2015-06-06 01:39:08]

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/chromium_fyi.py (right):

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1525: device_name=['Nexus
5'], device_os=['4.4.2', '4.4.3'],
On 2015/06/06 00:39:20, jbudorick wrote:
> All of the current devices are 4.4.2, so I think we can either just use that
or
> drop this kwarg altogether.

Done. I'll left the ['4.4.2'] to make it explicit.

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1526: device_timeout=60),
On 2015/06/06 00:39:20, jbudorick wrote:
> Also, I'm not sure about using a device timeout. If one step hits it, all of
the
> rest of them are going to hit it, too, at which point we've added n minutes to
> the cycle time before falling back on local devices.
> 
> This will especially be the case if we start out with so many suites.

I didn't realize this adds to the cycle time. I thought it was asynchronous.
Trigger a job, go on, and then wait 60 seconds before raising an exception if no
devices come online.

That way, nothing is added to cycle time, as the local tests will start to run
and content test alone takes 8 mins.

If this synchronously waits 60 seconds for a device, we should definitely not do
it.

I have removed it. Please confirm that it synchronously waits.

[navabi, 2015-06-06 01:56:53]

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:654:
device_name=self._device_name,
is it cool if device_timeout is missing from amp_arguments? Does amp just then
not wait?

[Paweł Hajdan Jr., 2015-06-08 09:33:21]

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
On 2015/05/18 at 15:51:49, navabi wrote:
> On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> > Why do we need this fallback logic? We don't do this anywhere else.
> 
> It is needed. We need this fallback logic because AMP may not have devices
available. When that is the case, the AMP tests, like the non-AMP tests, will
need to run locally. This will allow us to use AMP without requiring their lab
to immediately have full capacity.
> 
> The comment was meant to explain this. Let me know if you want more details in
the comment.

I think it'd be helpful to include what you said above in the comment.

> > If it's really needed, why not perform the local run in the "run" method as
> > opposed to "post_run"? That'd be more consistent with how other tests are
> > running.
> 
> We only want to run locally if we have triggered the AMP job, waiting for
devices to become available, done everything else we need to do (i.e. completed
the non-AMP tests, and find that the AMP tests were unable to run. In this case,
we run locally?
> 
> Are you suggesting moving the return statement on line 677 into the local run
method and then on line 677 just calling the local run method?

Yes, I think it'd be better to move self._local_test.run call (guarded by
self._trigger_successful) to the run method.

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:641: def name(self):
I don't see "failures" method being defined by this class. Similarly I see no
has_valid_results method. Please make sure they're present.

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:672: return
self._local_test.run(api, suffix)
How are failures of the local test run propagated to the users of this class?

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:686: self._failures = [str(f)]
Where is self._failures read?

[jbudorick, 2015-06-08 13:17:35]

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/chromium_fyi.py (right):

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1526: device_timeout=60),
On 2015/06/06 at 01:39:07, navabi wrote:
> On 2015/06/06 00:39:20, jbudorick wrote:
> > Also, I'm not sure about using a device timeout. If one step hits it, all of
the
> > rest of them are going to hit it, too, at which point we've added n minutes
to
> > the cycle time before falling back on local devices.
> > 
> > This will especially be the case if we start out with so many suites.
> 
> I didn't realize this adds to the cycle time. I thought it was asynchronous.
Trigger a job, go on, and then wait 60 seconds before raising an exception if no
devices come online.
> 
> That way, nothing is added to cycle time, as the local tests will start to run
and content test alone takes 8 mins.
> 
> If this synchronously waits 60 seconds for a device, we should definitely not
do it.
> 
> I have removed it. Please confirm that it synchronously waits.

Yeah, device selection is synchronous. Test execution is not, and that's where
the trigger/collect split comes in.

device_timeout makes the test runner wait that long for a device matching the
given characteristics to be available. When no device_timeout is provided, the
trigger will fail if no matching device is available immediately.

[navabi, 2015-06-18 19:47:48]

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
On 2015/06/08 09:33:21, Paweł Hajdan Jr. wrote:
> On 2015/05/18 at 15:51:49, navabi wrote:
> > On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> > > Why do we need this fallback logic? We don't do this anywhere else.
> > 
> > It is needed. We need this fallback logic because AMP may not have devices
> available. When that is the case, the AMP tests, like the non-AMP tests, will
> need to run locally. This will allow us to use AMP without requiring their lab
> to immediately have full capacity.
> > 
> > The comment was meant to explain this. Let me know if you want more details
in
> the comment.
> 
> I think it'd be helpful to include what you said above in the comment.
> 
> > > If it's really needed, why not perform the local run in the "run" method
as
> > > opposed to "post_run"? That'd be more consistent with how other tests are
> > > running.
> > 
> > We only want to run locally if we have triggered the AMP job, waiting for
> devices to become available, done everything else we need to do (i.e.
completed
> the non-AMP tests, and find that the AMP tests were unable to run. In this
case,
> we run locally?
> > 
> > Are you suggesting moving the return statement on line 677 into the local
run
> method and then on line 677 just calling the local run method?
> 
> Yes, I think it'd be better to move self._local_test.run call (guarded by
> self._trigger_successful) to the run method.

Done and done.

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:710: elif amp_test:
On 2015/05/18 15:51:49, navabi wrote:
> On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> > Do we need to modify GTestTest? Why wouldn't callers just use AMPGTestTest
> > directly?
> 
> This was to make it like SwarmingGTestTest. Notice how the callers dont just
use
> SwarmingGTestTest directly. In fact, my original patch used the AMPGTestTest
> directly, but in this patch I made it more like the swarming tests.

Done. Now the callers use AMPGTestTest directly.

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/chromium_fyi.py (right):

https://codereview.chromium.org/1104533002/diff/120001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/chromium_fyi.py:1526: device_timeout=60),
On 2015/06/08 13:17:35, jbudorick (ooo til 2015-06-18) wrote:
> On 2015/06/06 at 01:39:07, navabi wrote:
> > On 2015/06/06 00:39:20, jbudorick wrote:
> > > Also, I'm not sure about using a device timeout. If one step hits it, all
of
> the
> > > rest of them are going to hit it, too, at which point we've added n
minutes
> to
> > > the cycle time before falling back on local devices.
> > > 
> > > This will especially be the case if we start out with so many suites.
> > 
> > I didn't realize this adds to the cycle time. I thought it was asynchronous.
> Trigger a job, go on, and then wait 60 seconds before raising an exception if
no
> devices come online.
> > 
> > That way, nothing is added to cycle time, as the local tests will start to
run
> and content test alone takes 8 mins.
> > 
> > If this synchronously waits 60 seconds for a device, we should definitely
not
> do it.
> > 
> > I have removed it. Please confirm that it synchronously waits.
> 
> Yeah, device selection is synchronous. Test execution is not, and that's where
> the trigger/collect split comes in.
> 
> device_timeout makes the test runner wait that long for a device matching the
> given characteristics to be available. When no device_timeout is provided, the
> trigger will fail if no matching device is available immediately.

Removed device_timeout.

[navabi, 2015-06-18 21:20:58]

PTAL.

Comments are getting confused. Here is a summary of how I addressed Pawel's
comments:

I added the comment for why we run locally when pre_run fails to trigger job.

I did not move the local run call into the run method of AMPGTestTest. The
reason is because the chromium recipe calls pre_run, run and post_run. I don't
want that run call to do anything (similar to how SwarmingTests work). Only in
post run do I want to make the call to run tests locally if trigger failed.

self._failures was never used in AMPGTestTest. Thus, instead of assigning it, I
removed the try block so that the StepFailure gets raised. It will be caught by
the chromium recipe and appended to a list of failures.

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
On 2015/06/18 19:47:48, navabi wrote:
> On 2015/06/08 09:33:21, Paweł Hajdan Jr. wrote:
> > On 2015/05/18 at 15:51:49, navabi wrote:
> > > On 2015/05/08 08:55:11, Paweł Hajdan Jr. wrote:
> > > > Why do we need this fallback logic? We don't do this anywhere else.
> > > 
> > > It is needed. We need this fallback logic because AMP may not have devices
> > available. When that is the case, the AMP tests, like the non-AMP tests,
will
> > need to run locally. This will allow us to use AMP without requiring their
lab
> > to immediately have full capacity.
> > > 
> > > The comment was meant to explain this. Let me know if you want more
details
> in
> > the comment.
> > 
> > I think it'd be helpful to include what you said above in the comment.
> > 
> > > > If it's really needed, why not perform the local run in the "run" method
> as
> > > > opposed to "post_run"? That'd be more consistent with how other tests
are
> > > > running.
> > > 
> > > We only want to run locally if we have triggered the AMP job, waiting for
> > devices to become available, done everything else we need to do (i.e.
> completed
> > the non-AMP tests, and find that the AMP tests were unable to run. In this
> case,
> > we run locally?
> > > 
> > > Are you suggesting moving the return statement on line 677 into the local
> run
> > method and then on line 677 just calling the local run method?
> > 
> > Yes, I think it'd be better to move self._local_test.run call (guarded by
> > self._trigger_successful) to the run method.
> 
> Done and done.

Actually done and undone. We can't have the call to local test in the classes
run method. The reason is because the recipe does the following: pre_run, then
run, then post_run. That's why swarming tests do nothing for their run method.
It is not until post_run, when we determine the trigger failed that we want to
run tests locally.

We could always put something in the run method that says, if we haven't tried
to trigger, don't run, and if we did try to trigger but failed do run. But that
becomes much less clear as to what is going on.

[Paweł Hajdan Jr., 2015-06-19 10:46:09]

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

https://codereview.chromium.org/1104533002/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/steps.py:675: # If we were unable to
successfully trigger the AMP job, run locally.
On 2015/06/18 at 21:20:58, navabi wrote:
> Actually done and undone. We can't have the call to local test in the classes
run method. The reason is because the recipe does the following: pre_run, then
run, then post_run.

That's correct, the order of calls is like you said above. I don't see how that
prevents running the local fallback in the run method. Could you explain more?

It seems to me run could do "if not self._trigger_successful: run local
fallback", and post_run could do "if self._trigger_successful: collect AMP
results". Is there something that would break with that logic?

> That's why swarming tests do nothing for their run method.

I'm confused by above statement. swarming doesn't do local fallback, and so it
always collects results asynchronously in post_run; there's nothing that could
possibly be done in the run method for swarming.

> It is not until post_run, when we determine the trigger failed that we want to
run tests locally.

Could you explain more why? self._trigger_successful is set in pre_run, so it
should be available in both run and post_run.

> We could always put something in the run method that says, if we haven't tried
to trigger, don't run, and if we did try to trigger but failed do run. But that
becomes much less clear as to what is going on.

I'm indeed confused by the way above logic is phrased. Is it the same though as
saying (in run): if trigger failed, run the local fallback? That sounds simple
and clear to me.

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:641: def name(self):
On 2015/06/08 at 09:33:21, Paweł Hajdan Jr. wrote:
> I don't see "failures" method being defined by this class. Similarly I see no
has_valid_results method. Please make sure they're present.

Have you addressed this comment? It may be fine for now if it's a conscious
decision - just note you *will* have to implement these to run on CQ.

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:665: except
api.step.StepFailure:
Did you intend not to re-raise the failure? If it's intentional, please add a
comment why. In general it should not be silenced.

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:672: else:
nit: No need for an "else" after early return.

[navabi, 2015-06-19 19:43:56]

> 
> That's correct, the order of calls is like you said above. I don't see how
that
> prevents running the local fallback in the run method. Could you explain more?
> 
> It seems to me run could do "if not self._trigger_successful: run local
> fallback", and post_run could do "if self._trigger_successful: collect AMP
> results". Is there something that would break with that logic?
> 
> > That's why swarming tests do nothing for their run method.
> 
> I'm confused by above statement. swarming doesn't do local fallback, and so it
> always collects results asynchronously in post_run; there's nothing that could
> possibly be done in the run method for swarming.
> 
> > It is not until post_run, when we determine the trigger failed that we want
to
> run tests locally.
> 
> Could you explain more why? self._trigger_successful is set in pre_run, so it
> should be available in both run and post_run.

I'm not really sure how else to explain it, but I'll try :). Apologies if it
sounds like I'm repeating the same thing I said before:

You are suggesting in post_run I do this:
if not self._trigger_successful:
  return self.run(api, suffix)

And the the AMPGTestTest run method will be:
def run(self, api, suffix):
  return self._local_test.run(api, suffix)

Am I understanding your suggestion correctly?

If so, this will not work. Because the recipe calls, pre_run, run and then
post_run. When it calls run it will run the test locally. We do not want to run
the test locally. Because we only want to run the test locally if the it failed
to trigger a remote job.

Now I think you are suggesting to fix this add extra logic. That would require
moving the check to for "if not self._trigger_successful". This would work but
would be less clear than the current code. Then in the post_run, do we check the
condition again "if not self._trigger_successful" to make it clear the intention
of calling locally is only if it failed to trigger. Of course, we don't need to
check it as the run method will already have it, but then the post_run call just
calls run first and knows it will check the trigger. This seems unclear to me.
The way it is now seems easier to ready.

Let me know if you are suggesting something else.

[navabi, 2015-06-19 19:49:29]

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:641: def name(self):
On 2015/06/19 10:46:09, Paweł Hajdan Jr. wrote:
> On 2015/06/08 at 09:33:21, Paweł Hajdan Jr. wrote:
> > I don't see "failures" method being defined by this class. Similarly I see
no
> has_valid_results method. Please make sure they're present.
> 
> Have you addressed this comment? It may be fine for now if it's a conscious
> decision - just note you *will* have to implement these to run on CQ.

I see. Can you show me where these are used in the recipe? I do not see any call
to self.failures or self.has_valid_results in the chromium recipe.

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:665: except
api.step.StepFailure:
On 2015/06/19 10:46:09, Paweł Hajdan Jr. wrote:
> Did you intend not to re-raise the failure? If it's intentional, please add a
> comment why. In general it should not be silenced.

No, I did not mean to. I have comments for it on line 669. If it seems more
clear I can repeat the comment about being unable to trigger job here too. It's
a little superfluous to keep explaining the same logic in multiple places, but
this is the same reason as that comment.

If it fails to trigger a test, we don't want to fail the test, rather we want to
note that it failed to trigger an AMP job and will have to run locally.

[Paweł Hajdan Jr., 2015-06-22 14:31:41]

On 2015/06/19 at 19:43:56, navabi wrote:
> I'm not really sure how else to explain it, but I'll try :). Apologies if it
sounds like I'm repeating the same thing I said before:
> 
> You are suggesting in post_run I do this:
> if not self._trigger_successful:
>   return self.run(api, suffix)

No. Why would post_run call the run method which is called earlier anyway?

I was thinking about putting the api.amp.collect_test_suite call there (and any
remaining dependencies).

Also, shouldn't the condition be reversed, i.e. "if self._trigger_successful"?
My understanding is that we only want to collect results from AMP when the
trigger was successful.

> And the the AMPGTestTest run method will be:
> def run(self, api, suffix):
>   return self._local_test.run(api, suffix)
> 
> Am I understanding your suggestion correctly?

I recommended above to also be guarded by "if not self._trigger_successful".
After that it should be fine for me.

> If so, this will not work. Because the recipe calls, pre_run, run and then
post_run. When it calls run it will run the test locally. We do not want to run
the test locally. Because we only want to run the test locally if the it failed
to trigger a remote job.

What prevents us from having "if not self._trigger_successful" in run as I
recommended above? Is there some reason that wouldn't work?

> Now I think you are suggesting to fix this add extra logic. That would require
moving the check to for "if not self._trigger_successful". This would work but
would be less clear than the current code. Then in the post_run, do we check the
condition again "if not self._trigger_successful" to make it clear the intention
of calling locally is only if it failed to trigger. Of course, we don't need to
check it as the run method will already have it, but then the post_run call just
calls run first and knows it will check the trigger. This seems unclear to me.
The way it is now seems easier to ready.

I'm confused why post_run would call run. I didn't intend to suggest anything
like that, and in fact I agree it's strange.

Do my answers above make the intended shape of the code make more sense?

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/1104533002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium/steps.py:641: def name(self):
On 2015/06/19 at 19:49:28, navabi wrote:
> I see. Can you show me where these are used in the recipe? I do not see any
call to self.failures or self.has_valid_results in the chromium recipe.

Please see determine_new_failures in
build/scripts/slave/recipe_modules/test_utils/api.py

As mentioned before, this is not strictly needed now, but will become a problem
once you try to run this on a tryserver (and it *will* be required to make CQ
use this).

[navabi, 2015-06-22 21:47:07]

> I'm confused why post_run would call run. I didn't intend to suggest anything
> like that, and in fact I agree it's strange.

If it is not clear why post_run has to call run, I think we need to discuss this
offline.
 
> Do my answers above make the intended shape of the code make more sense?

No. I think one of us is not understanding what the other is saying.

[navabi, 2015-06-23 07:44:10]

I moved the call to run local tests into the run method. Since we know
immediately after trying to trigger and AMP job whether it succeeded or not, by
the time we get to the run call we know whether or not run locally.

Since this is an FYI bot, I'd like to land this and then add the parts about
failures. PTAL.

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1104533002/diff/160001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:672: else:
On 2015/06/19 10:46:09, Paweł Hajdan Jr. wrote:
> nit: No need for an "else" after early return.

Done.

[Paweł Hajdan Jr., 2015-06-23 08:33:23]

LGTM!

[jbudorick, 2015-06-23 21:04:30]

https://codereview.chromium.org/1104533002/diff/200001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py (right):

https://codereview.chromium.org/1104533002/diff/200001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/chromium_fyi.py:1506:
'parent_buildername': 'Android Builder',
This should be 'Android Builder (dbg)', and we should see expectation changes
for that builder.

[navabi, 2015-06-23 21:38:30]

PTAL

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/chromium_tests/steps.py (right):

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/chromium_tests/steps.py:643: def
compile_targets(self, api):
had to add this for recipe_simulation_tests.py.

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipes/...
File
scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Builder__dbg_.json
(right):

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipes/...
scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Builder__dbg_.json:202:
"net_unittests_apk",
are these the expectation changes you had in mind? These were added because I
had to implement compile_targets in chromium_tests/steps.py.

[jbudorick, 2015-06-23 21:39:57]

lgtm

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipes/...
File
scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Builder__dbg_.json
(right):

https://codereview.chromium.org/1104533002/diff/220001/scripts/slave/recipes/...
scripts/slave/recipes/chromium.expected/full_chromium_fyi_Android_Builder__dbg_.json:202:
"net_unittests_apk",
On 2015/06/23 at 21:38:30, navabi wrote:
> are these the expectation changes you had in mind? These were added because I
had to implement compile_targets in chromium_tests/steps.py.

Yep, this is what I expected.

[navabi, 2015-06-23 22:02:26]

The CQ bit was checked by navabi@google.com

[navabi, 2015-06-23 22:02:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1104533002/#ps220001
(title: "Parent builder Android Builder (dbg) and expectation changes.")

[commit-bot: I haz the power, 2015-06-23 22:02:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1104533002/220001

[commit-bot: I haz the power, 2015-06-23 22:06:59]

Committed patchset #12 (id:220001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295808