Add unit tests for overrides in multi-repo workflow

Fix multi-repo workflow overrides and add tests

See https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvDBgAJ for context.

This also reverts https://codereview.chromium.org/2657973006 (30a672d26b45289947783932d8062fc70dfb1611; didn't fix an issue I was trying to) and https://codereview.chromium.org/2343733003 (f7ada90c926c78704f048767e29e8ae836d8a65b; introduced the regression).

BUG=none

Review-Url: https://codereview.chromium.org/2664223002
Committed: https://github.com/luci/recipes-py/commit/4fb4d4bdfda85253b842648abf8588749c68a5eb

[Paweł Hajdan Jr., 2017-01-31 17:43:51]

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

[commit-bot: I haz the power, 2017-01-31 17:43:59]

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, 2017-01-31 17:49:46]

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

[commit-bot: I haz the power, 2017-01-31 17:49:46]

Dry run: Try jobs failed on following builders:
  Recipes-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/340e745b0cb36910)

[iannucci, 2017-01-31 18:22:54]

iannucci@chromium.org changed reviewers:
+ dnj@chromium.org, iannucci@chromium.org

[iannucci, 2017-01-31 18:22:54]

These lgtm. These tests should definitely pass, and so adjusting the code as
appropriate to do that is... well... appropriate :D.

Dan, is there some test that you had in mind when doing the original change?
Could we add that test now to ensure that it is currently working (or not?).
Maybe something else regressed?

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py
File unittests/override_test.py (right):

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py#...
unittests/override_test.py:39: cm.exception.output)
hm... it's a bit weird to use cm out of scope like this, but I guess it's
necessary?

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py#...
unittests/override_test.py:68: self.train_recipes(repos['c'])
should there be a way to assert the output of training? e.g. we should be able
to see that c calls a_module.foo, which runs step 'baz' here, correct?

https://codereview.chromium.org/2664223002/diff/1/unittests/repo_test_util.py
File unittests/repo_test_util.py (right):

https://codereview.chromium.org/2664223002/diff/1/unittests/repo_test_util.py...
unittests/repo_test_util.py:180: def train_recipes(self, repo, overrides=None):
docstring please

[dnj, 2017-01-31 18:27:29]

> Dan, is there some test that you had in mind when doing the original change?
> Could we add that test now to ensure that it is currently working (or not?).
> Maybe something else regressed?

I don't really remember. If something breaks, we'll go from there.

[Paweł Hajdan Jr., 2017-01-31 18:30:54]

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py
File unittests/override_test.py (right):

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py#...
unittests/override_test.py:39: cm.exception.output)
On 2017/01/31 18:22:54, iannucci wrote:
> hm... it's a bit weird to use cm out of scope like this, but I guess it's
> necessary?

This is straight from python docs,
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises .

The variable remains valid, and the scope is intended to be a block where we
catch the exception, not where we inspect it. Seems reasonable to me.

https://codereview.chromium.org/2664223002/diff/1/unittests/override_test.py#...
unittests/override_test.py:68: self.train_recipes(repos['c'])
On 2017/01/31 18:22:54, iannucci wrote:
> should there be a way to assert the output of training? e.g. we should be able
> to see that c calls a_module.foo, which runs step 'baz' here, correct?

That's an internal part of update_recipe call, which itself calls train_recipes
anyway.

I put it again in the test to further emphasize failure when overriding.

In theory we could make this even more elaborate as you said and inspect the
contents of the new expectations, but I'm not sure if that's worth it. I'd
prefer each thing to be tested orthogonally (i.e. simulation tests tested
somewhere else from here), and for that reason deliberately avoided using
autoroller here and instead put the right revisions manually.

https://codereview.chromium.org/2664223002/diff/1/unittests/repo_test_util.py
File unittests/repo_test_util.py (right):

https://codereview.chromium.org/2664223002/diff/1/unittests/repo_test_util.py...
unittests/repo_test_util.py:180: def train_recipes(self, repo, overrides=None):
On 2017/01/31 18:22:54, iannucci wrote:
> docstring please 

Yup, done.

I just wasn't sure what you'd think about the general approach. :)

[Paweł Hajdan Jr., 2017-01-31 18:32:50]

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

[commit-bot: I haz the power, 2017-01-31 18:32:54]

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

[Paweł Hajdan Jr., 2017-01-31 18:34:11]

Description was changed from

==========
Add unit tests for overrides in multi-repo workflow

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/bCdyzcKx...
for context.

BUG=none
==========

to

==========
Fix multi-repo workflow overrides and add tests

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD...
for context.

This also reverts https://codereview.chromium.org/2657973006 (didn't fix an
issue I was trying to) and https://codereview.chromium.org/2343733003
(introduced the regression).

BUG=none
==========

[commit-bot: I haz the power, 2017-01-31 18:35:44]

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

[commit-bot: I haz the power, 2017-01-31 18:35:45]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-01-31 18:37:21]

Actually now that I think about it, asserting on the expectation output would be
wrong (as you say), but maybe getting the engine
to produce an 'actual' manifest of which things it depended on? Just to make
sure that when you say a@rev1, it's actually getting a@rev1.

[iannucci, 2017-01-31 18:38:18]

In CL messages, please prefer to link to the commits rather than the CLs.
Rietveld won't last forever, but the git hashes might.

[iannucci, 2017-01-31 18:38:39]

other than that lgtm

[Paweł Hajdan Jr., 2017-01-31 18:46:57]

Description was changed from

==========
Fix multi-repo workflow overrides and add tests

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD...
for context.

This also reverts https://codereview.chromium.org/2657973006 (didn't fix an
issue I was trying to) and https://codereview.chromium.org/2343733003
(introduced the regression).

BUG=none
==========

to

==========
Fix multi-repo workflow overrides and add tests

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD...
for context.

This also reverts https://codereview.chromium.org/2657973006
(30a672d26b45289947783932d8062fc70dfb1611; didn't fix an issue I was trying to)
and https://codereview.chromium.org/2343733003
(f7ada90c926c78704f048767e29e8ae836d8a65b; introduced the regression).

BUG=none
==========

[Paweł Hajdan Jr., 2017-01-31 18:51:49]

Updated commit message to use git shas. FWIW I'd expect Rietveld to stay up,
just in read-only mode.

Also filed https://bugs.chromium.org/p/chromium/issues/detail?id=687257 for your
manifest suggestion as discussed over IM.

Thank you for great review! :)

[Paweł Hajdan Jr., 2017-01-31 19:01:59]

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

[commit-bot: I haz the power, 2017-01-31 19:02:05]

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, 2017-01-31 19:04:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485889319126800,
"parent_rev": "30a672d26b45289947783932d8062fc70dfb1611", "commit_rev":
"4fb4d4bdfda85253b842648abf8588749c68a5eb"}

[commit-bot: I haz the power, 2017-01-31 19:04:57]

Description was changed from

==========
Fix multi-repo workflow overrides and add tests

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD...
for context.

This also reverts https://codereview.chromium.org/2657973006
(30a672d26b45289947783932d8062fc70dfb1611; didn't fix an issue I was trying to)
and https://codereview.chromium.org/2343733003
(f7ada90c926c78704f048767e29e8ae836d8a65b; introduced the regression).

BUG=none
==========

to

==========
Fix multi-repo workflow overrides and add tests

See
https://groups.google.com/a/chromium.org/d/msg/infra-dev/8Hryqf4HzaQ/x5Nb0RvD...
for context.

This also reverts https://codereview.chromium.org/2657973006
(30a672d26b45289947783932d8062fc70dfb1611; didn't fix an issue I was trying to)
and https://codereview.chromium.org/2343733003
(f7ada90c926c78704f048767e29e8ae836d8a65b; introduced the regression).

BUG=none

Review-Url: https://codereview.chromium.org/2664223002
Committed:
https://github.com/luci/recipes-py/commit/4fb4d4bdfda85253b842648abf8588749c6...
==========

[commit-bot: I haz the power, 2017-01-31 19:04:59]

Committed patchset #3 (id:40001) as
https://github.com/luci/recipes-py/commit/4fb4d4bdfda85253b842648abf8588749c6...