Split Placeholder into InputPlaceholder and OutputPlaceholder.

Split Placeholder into InputPlaceholder and OutputPlaceholder.

This CL is a prerequisite for https://codereview.chromium.org/1773273003/.

BUG=593198
Committed: https://github.com/luci/recipes-py/commit/f66f2c7d031225091ea1fd98d993bbb4dc73cfe7

[stgao, 2016-03-11 08:52:19]

Patchset #1 (id:1) has been deleted

[stgao, 2016-03-11 23:39:03]

Description was changed from

==========
Split Placeholder into InputPlaceholder and OutputPlaceholder.

BUG=593198
==========

to

==========
Split Placeholder into InputPlaceholder and OutputPlaceholder.

This CL is a prerequisite for https://codereview.chromium.org/1773273003/.

BUG=593198
==========

[stgao, 2016-03-11 23:39:44]

stgao@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org

[stgao, 2016-03-11 23:39:45]

Hi folks, do you mind a review?

[iannucci, 2016-03-12 03:36:13]

https://chromiumcodereview.appspot.com/1785543004/diff/40001/recipe_engine/re...
File recipe_engine/recipe_test_api.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/40001/recipe_engine/re...
recipe_engine/recipe_test_api.py:59: self.input_placeholder_data =
collections.defaultdict(list)
I don't think there needs to be input_placeholder_data here; inputs are never
mocked in tests. This file shouldn't need to change very much at all, just
clarify that 'placeholder_data' means output placeholders.

https://chromiumcodereview.appspot.com/1785543004/diff/40001/recipe_engine/st...
File recipe_engine/step_runner.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/40001/recipe_engine/st...
recipe_engine/step_runner.py:458: tdata =
step_test.pop_input_placeholder(item.name_pieces)
not needed: inputs are never mocked

https://chromiumcodereview.appspot.com/1785543004/diff/40001/recipe_engine/st...
recipe_engine/step_runner.py:525: if key != 'stdin':
this looks wrong to me...? Not sure what this is supposed to be for?

[stgao, 2016-03-18 23:01:09]

Patchset #3 (id:60001) has been deleted

[stgao, 2016-03-18 23:04:40]

Patchset #3 (id:80001) has been deleted

[stgao, 2016-03-22 05:29:56]

Patchset #3 (id:100001) has been deleted

[stgao, 2016-03-22 05:56:43]

Comments are addressed. PTAL :)

https://codereview.chromium.org/1785543004/diff/40001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/1785543004/diff/40001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:59: self.input_placeholder_data =
collections.defaultdict(list)
On 2016/03/12 03:36:12, iannucci wrote:
> I don't think there needs to be input_placeholder_data here; inputs are never
> mocked in tests. This file shouldn't need to change very much at all, just
> clarify that 'placeholder_data' means output placeholders.

Done. Also update other docstrings and error messages.

https://codereview.chromium.org/1785543004/diff/40001/recipe_engine/step_runn...
File recipe_engine/step_runner.py (right):

https://codereview.chromium.org/1785543004/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:458: tdata =
step_test.pop_input_placeholder(item.name_pieces)
On 2016/03/12 03:36:12, iannucci wrote:
> not needed: inputs are never mocked

Reverted to the old approach.

We still need a default mock for inputs, otherwise exceptions will be thrown
when the underlying code check test_data.enabled (e.g. in
raw_io.InputDataPlaceholder.render).

https://codereview.chromium.org/1785543004/diff/40001/recipe_engine/step_runn...
recipe_engine/step_runner.py:525: if key != 'stdin':
On 2016/03/12 03:36:12, iannucci wrote:
> this looks wrong to me...? Not sure what this is supposed to be for?

Reverted.

However, I thought we don't want to include all inputs in the step result, as
recipes should not refer to the inputs. I might have misunderstood what you said
earlier.

For the ``input_phs`` above, do we also want to include them in the step result?

[iannucci, 2016-03-22 22:22:48]

lgtm w/ comments

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
File recipe_engine/step_runner.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
recipe_engine/step_runner.py:476: assert isinstance(placeholder,
util.InputPlaceholder), key
assertion message

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
recipe_engine/step_runner.py:478: assert isinstance(placeholder,
util.OutputPlaceholder), key
same

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/u...
File recipe_engine/util.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/u...
recipe_engine/util.py:74: pass
I think this method can be removed entirely, right? Or we can at least change it
to `cleanup(self)`, since it's handled by a completely different loop.

[stgao, 2016-03-22 23:19:17]

Comments are addressed.

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
File recipe_engine/step_runner.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
recipe_engine/step_runner.py:476: assert isinstance(placeholder,
util.InputPlaceholder), key
On 2016/03/22 22:22:48, iannucci wrote:
> assertion message

Done.

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/s...
recipe_engine/step_runner.py:478: assert isinstance(placeholder,
util.OutputPlaceholder), key
On 2016/03/22 22:22:48, iannucci wrote:
> same

Done.

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/u...
File recipe_engine/util.py (right):

https://chromiumcodereview.appspot.com/1785543004/diff/120001/recipe_engine/u...
recipe_engine/util.py:74: pass
On 2016/03/22 22:22:48, iannucci wrote:
> I think this method can be removed entirely, right? Or we can at least change
it
> to `cleanup(self)`, since it's handled by a completely different loop.

Renaming sounds good to me. Done.

However, when process stderr, stdout, and stdio, we have to do branching.

[stgao, 2016-03-23 05:02:23]

Patchset #5 (id:160001) has been deleted

[iannucci, 2016-03-23 18:45:30]

lgtm

[stgao, 2016-03-23 18:52:32]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2016-03-23 18:52:44]

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

[commit-bot: I haz the power, 2016-03-23 18:54:56]

Description was changed from

==========
Split Placeholder into InputPlaceholder and OutputPlaceholder.

This CL is a prerequisite for https://codereview.chromium.org/1773273003/.

BUG=593198
==========

to

==========
Split Placeholder into InputPlaceholder and OutputPlaceholder.

This CL is a prerequisite for https://codereview.chromium.org/1773273003/.

BUG=593198

Committed:
https://github.com/luci/recipes-py/commit/f66f2c7d031225091ea1fd98d993bbb4dc7...
==========

[commit-bot: I haz the power, 2016-03-23 18:54:57]

Committed patchset #6 (id:200001) as
https://github.com/luci/recipes-py/commit/f66f2c7d031225091ea1fd98d993bbb4dc7...