Make MockExecutive behave more like MockExecutive2.

Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use MockExecutive everywhere.

BUG=626115
Committed: https://crrev.com/b29e220bd93d7ba7d6d7287ae6789aa46a31b096
Cr-Commit-Position: refs/heads/master@{#440557}

[qyearsley, 2016-12-19 22:48:59]

Description was changed from

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

BUG=626115
==========

to

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use
MockExecutive everywhere.

BUG=626115
==========

[qyearsley, 2016-12-19 22:49:28]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org

[qyearsley, 2016-12-19 22:49:29]

https://codereview.chromium.org/2588863002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android_unittest.py
(right):

https://codereview.chromium.org/2588863002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android_unittest.py:97:
port._check_devices = lambda _: None
The reason why this is added:

With the previous version of MockExecutive, _check_devices would never be called
because in AndroidPort.check_build, it would call Port.check_build which would
return something truthy.

(With this change to MockExecutive, MockExecutive.run_command will return 0 by
default if return_exit_code is given, whereas previously it would return a
non-empty string.)

Anyway, I don't believe we necessarily want to test _check_devices in this unit
test, since the real _check_devices method depends on interacting with external
devices.

See:
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

https://codereview.chromium.org/2588863002/diff/20001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/auto_rebaseline_unittest.py
(right):

https://codereview.chromium.org/2588863002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/auto_rebaseline_unittest.py:248:
['git', 'cl', 'set_close'],
The reason why this extra command is run is:

Previously in MockExecutive.run_command, the argument return_exit_code would
only make the function return 0 if should_return_zero_when_run was passed when
constructing the MockExecutive [1]. That was never passed, so in practice,
return_exit_code had no effect for MockExecutive.

After this change, MockExecutive returns self._exit_code (default 0) when
return_exit_code=True is passed.

This means that in auto_rebaseline.py [2] when running the tests,
issue_already_closed is 0, and an additional command is run.

In terms of what this means for this test: It means we're always going through
the code path where `git config branch.${branch}.rietveldissue` has an exit code
of zero, implying that the issue is isn't yet closed, so it calls `git cl
set_close`. The actual behavior of the code isn't changed, only the codepath
that's tested.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

[qyearsley, 2016-12-20 22:23:18]

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

[qyearsley, 2016-12-20 22:23:25]

qyearsley@chromium.org changed reviewers:
+ jeffcarp@chromium.org

[commit-bot: I haz the power, 2016-12-20 22:24:03]

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-12-20 22:27:02]

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

[commit-bot: I haz the power, 2016-12-20 22:27:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[qyearsley, 2016-12-20 22:32:55]

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

[commit-bot: I haz the power, 2016-12-20 22:33:52]

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-12-20 23:39:24]

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

[commit-bot: I haz the power, 2016-12-20 23:39:25]

Dry run: This issue passed the CQ dry run.

[jeffcarp, 2016-12-22 22:48:46]

lgtm. How much more work is it to unify MockExecutive and MockExecutive2? If
it's just a matter of renaming a bunch of invocations then it seems like it'd be
ok to include in this CL.

https://codereview.chromium.org/2588863002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):

https://codereview.chromium.org/2588863002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:45:
time_return_val=123):
Do you want to re-shuffle the indentation here? (nbd)

[qyearsley, 2016-12-22 23:06:47]

On 2016/12/22 at 22:48:46, jeffcarp wrote:
> lgtm. How much more work is it to unify MockExecutive and MockExecutive2? If
it's just a matter of renaming a bunch of invocations then it seems like it'd be
ok to include in this CL.
> 
>
https://codereview.chromium.org/2588863002/diff/40001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py (right):
> 
>
https://codereview.chromium.org/2588863002/diff/40001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:45:
time_return_val=123):
> Do you want to re-shuffle the indentation here? (nbd)

Yeah, the indentation does look a bit wonky with a short first line and long
second line. Reshuffled it to be one line per arg now.

[qyearsley, 2016-12-22 23:08:50]

On 2016/12/22 at 23:06:47, qyearsley wrote:
> On 2016/12/22 at 22:48:46, jeffcarp wrote:
> >
https://codereview.chromium.org/2588863002/diff/40001/third_party/WebKit/Tool...
> > third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py:45:
time_return_val=123):
> > Do you want to re-shuffle the indentation here? (nbd)
> 
> Yeah, the indentation does look a bit wonky with a short first line and long
second line. Reshuffled it to be one line per arg now.

On 2016/12/22 at 22:48:46, jeffcarp wrote:
> lgtm. How much more work is it to unify MockExecutive and MockExecutive2? If
it's just a matter of renaming a bunch of invocations then it seems like it'd be
ok to include in this CL.

I don't *believe* it's much work, but it will involve a lot of small changes
across a lot of files, so I think I'd still like to commit this and make a
separate CL for remove MockExecutive2, just in case that's more complicated than
expected.

[qyearsley, 2016-12-22 23:08:52]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-12-22 23:08:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
jeffcarp@chromium.org
Link to the patchset: https://codereview.chromium.org/2588863002/#ps60001
(title: "Reformat args lists for MockHost, MockSystemHost.")

[commit-bot: I haz the power, 2016-12-22 23:09:20]

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-12-23 00:25:34]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482448132977350,
"parent_rev": "3c3f73551dbc6f0dcc5d0556705ba4d622a926f4", "commit_rev":
"6176c13e6c5307bda68bcd5981e96c5d86d7e0fc"}

[commit-bot: I haz the power, 2016-12-23 00:26:06]

Description was changed from

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use
MockExecutive everywhere.

BUG=626115
==========

to

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use
MockExecutive everywhere.

BUG=626115

Review-Url: https://codereview.chromium.org/2588863002
==========

[commit-bot: I haz the power, 2016-12-23 00:26:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-23 00:28:58]

Description was changed from

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use
MockExecutive everywhere.

BUG=626115

Review-Url: https://codereview.chromium.org/2588863002
==========

to

==========
Make MockExecutive behave more like MockExecutive2.

The purpose of this CL is to prepare for merging MockExecutive and
MockExecutive2.

The plan after this CL is to make another CL to remove MockExecutive2 and use
MockExecutive everywhere.

BUG=626115

Committed: https://crrev.com/b29e220bd93d7ba7d6d7287ae6789aa46a31b096
Cr-Commit-Position: refs/heads/master@{#440557}
==========

[commit-bot: I haz the power, 2016-12-23 00:28:59]

Patchset 4 (id:??) landed as
https://crrev.com/b29e220bd93d7ba7d6d7287ae6789aa46a31b096
Cr-Commit-Position: refs/heads/master@{#440557}