De-duplicate steps between chromium and chromium_trybot recipes

De-duplicate steps between chromium and chromium_trybot recipes

BUG=none

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

[agable, 2014-06-27 18:34:42]

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium/chromium_chromiumos.py (left):

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/chromium_chromiumos.py:60:
steps.Deps2SubmodulesTest(),
Why is this removed in this CL? I mean, it should be removed, because submodules
are no longer supported, but maybe it should be a separate CL. Also, I don't see
any corresponding expectations updates.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/steps.py:6: class Test(object):
I don't understand why you moved this out of test_utils.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:38: def
determine_new_failures(self, api, tests, deapply_patch_fn):
I really don't like all of these functions taking an API as input. Self already
is an api! All the other modules are on self.m! Having two APIs to choose from
is a Bad Idea.

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium.py:18: 'test_utils',
I find it to be an anti-pattern for this recipe to depend on the test_utils
module now, even though it never calls api.test_utils.

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium.py:222: test_steps = [t.run(api, '') for t in
bot_config.get('tests', [])]
Why not have suffix='' as a default argument everywhere?

[Paweł Hajdan Jr., 2014-06-27 18:58:23]

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium/chromium_chromiumos.py (left):

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/chromium_chromiumos.py:60:
steps.Deps2SubmodulesTest(),
On 2014/06/27 18:34:42, agable wrote:
> Why is this removed in this CL? I mean, it should be removed, because
submodules
> are no longer supported, but maybe it should be a separate CL. Also, I don't
see
> any corresponding expectations updates.

That's right, because nothing changes. Deps2Git from the tryserver runs
deps2submodules under the hood.

I'm happy to add a TODO or something to sort this out as a follow-up.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/steps.py:6: class Test(object):
On 2014/06/27 18:34:42, agable wrote:
> I don't understand why you moved this out of test_utils.

Suggestions what to do are welcome. Can I easily access this in steps.py? If so,
how?

Please just tell me what you consider the best way to structure this and I can
do that - I just need to understand.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:38: def
determine_new_failures(self, api, tests, deapply_patch_fn):
On 2014/06/27 18:34:42, agable wrote:
> I really don't like all of these functions taking an API as input. Self
already
> is an api! All the other modules are on self.m! Having two APIs to choose from
> is a Bad Idea.

Suggestions what to replace that are welcome.

I could use self (or self.m) instead, however then test_utils' DEPS would need
to include things required by steps.py in the chromium recipe module.

It's a tradeoff, either way works for me although the solution from this patch
seems to make more sense to me. Maybe there is a third way I'm just not seeing -
recommendations are welcome.

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium.py:18: 'test_utils',
On 2014/06/27 18:34:42, agable wrote:
> I find it to be an anti-pattern for this recipe to depend on the test_utils
> module now, even though it never calls api.test_utils.

Suggestions how to change this are welcome. One way I can definitely do is add a
TODO to clean this up. I'm also happy to structure things some other way if you
have something specific in mind.

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipes/chromi...
scripts/slave/recipes/chromium.py:222: test_steps = [t.run(api, '') for t in
bot_config.get('tests', [])]
On 2014/06/27 18:34:42, agable wrote:
> Why not have suffix='' as a default argument everywhere?

I considered that, I somewhat prefer it to be explicit.

If it's a strong opinion, I'm happy to change.

[agable, 2014-06-27 21:04:23]

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
File scripts/slave/recipe_modules/chromium/chromium_chromiumos.py (left):

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/chromium_chromiumos.py:60:
steps.Deps2SubmodulesTest(),
On 2014/06/27 18:58:22, Paweł Hajdan Jr. wrote:
> On 2014/06/27 18:34:42, agable wrote:
> > Why is this removed in this CL? I mean, it should be removed, because
> submodules
> > are no longer supported, but maybe it should be a separate CL. Also, I don't
> see
> > any corresponding expectations updates.
> 
> That's right, because nothing changes. Deps2Git from the tryserver runs
> deps2submodules under the hood.
> 
> I'm happy to add a TODO or something to sort this out as a follow-up.

That's terrifying that deps2git runs it, and that having this step then
*doesn't* run it because it's already been run. Or something like that.
Aaaaaaahh.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/chromium/steps.py:6: class Test(object):
On 2014/06/27 18:58:22, Paweł Hajdan Jr. wrote:
> On 2014/06/27 18:34:42, agable wrote:
> > I don't understand why you moved this out of test_utils.
> 
> Suggestions what to do are welcome. Can I easily access this in steps.py? If
so,
> how?
> 
> Please just tell me what you consider the best way to structure this and I can
> do that - I just need to understand.

As I said in person, I think that this file should not exist. This class should
be in test_utils, and everything else in chromium. Chromium can access it via a
dependency on test_utils, and self.m.test_utils.Test.

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

https://codereview.chromium.org/339183013/diff/1/scripts/slave/recipe_modules...
scripts/slave/recipe_modules/test_utils/api.py:38: def
determine_new_failures(self, api, tests, deapply_patch_fn):
On 2014/06/27 18:58:22, Paweł Hajdan Jr. wrote:
> On 2014/06/27 18:34:42, agable wrote:
> > I really don't like all of these functions taking an API as input. Self
> already
> > is an api! All the other modules are on self.m! Having two APIs to choose
from
> > is a Bad Idea.
> 
> Suggestions what to replace that are welcome.
> 
> I could use self (or self.m) instead, however then test_utils' DEPS would need
> to include things required by steps.py in the chromium recipe module.
> 
> It's a tradeoff, either way works for me although the solution from this patch
> seems to make more sense to me. Maybe there is a third way I'm just not seeing
-
> recommendations are welcome.

If you do the reorganization I describe in the comment above, then passing api
won't be necessary here.

[agable, 2014-06-28 00:05:13]

LGTM % comments.

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:45: api - recipe API
Please call this parent_api (caller_api?) or something like that, to distinguish
it from the api that is present at self.m. Also, would be really good to have a
comment right here explaining why it is necessary to pass the caller's api
through to the Test classes.

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipes/bl...
File scripts/slave/recipes/blink_trybot.py (right):

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipes/bl...
scripts/slave/recipes/blink_trybot.py:257: class
BlinkTest(api.chromium.steps.Test):
Can we move this into steps.py with everything else? The mismatch is
incongruous. Alternatively, a blink recipe_module with just this in its own
steps.py, but that'd be more work.

[Paweł Hajdan Jr., 2014-06-28 00:19:01]

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/test_utils/api.py (right):

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/test_utils/api.py:45: api - recipe API
On 2014/06/28 00:05:13, agable wrote:
> Please call this parent_api (caller_api?) or something like that, to
distinguish
> it from the api that is present at self.m. Also, would be really good to have
a
> comment right here explaining why it is necessary to pass the caller's api
> through to the Test classes.

Done.

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipes/bl...
File scripts/slave/recipes/blink_trybot.py (right):

https://codereview.chromium.org/339183013/diff/20001/scripts/slave/recipes/bl...
scripts/slave/recipes/blink_trybot.py:257: class
BlinkTest(api.chromium.steps.Test):
On 2014/06/28 00:05:13, agable wrote:
> Can we move this into steps.py with everything else? The mismatch is
> incongruous. Alternatively, a blink recipe_module with just this in its own
> steps.py, but that'd be more work.

Done.

[Paweł Hajdan Jr., 2014-06-28 00:19:03]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-06-28 00:19:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/339183013/40001

[commit-bot: I haz the power, 2014-06-28 00:20:43]

Change committed as 280461

[Sergiy Byelozyorov, 2014-06-30 12:46:19]

https://codereview.chromium.org/339183013/diff/40001/scripts/slave/recipe_mod...
File scripts/slave/recipe_modules/chromium/steps.py (right):

https://codereview.chromium.org/339183013/diff/40001/scripts/slave/recipe_mod...
scripts/slave/recipe_modules/chromium/steps.py:13: def name(self):  # pragma: no
cover
What does pragma mean here and below? I thought that was C++...

[iannucci, 2014-06-30 18:00:59]

On 2014/06/30 12:46:19, Sergiy Byelozyorov wrote:
>
https://codereview.chromium.org/339183013/diff/40001/scripts/slave/recipe_mod...
> File scripts/slave/recipe_modules/chromium/steps.py (right):
> 
>
https://codereview.chromium.org/339183013/diff/40001/scripts/slave/recipe_mod...
> scripts/slave/recipe_modules/chromium/steps.py:13: def name(self):  # pragma:
no
> cover
> What does pragma mean here and below? I thought that was C++...

We use python-coverage to ensure that all recipe-releated code has 100% line
coverage via the recipe simulation tests. This marker indicates that a suite
isn't to be counted as coverable (which, in this case, is because it's a base
implementation which is never called as it is properly overridden by all
subclasses)