Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885023004:
[step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. (Closed)
|
Created:
3 years, 7 months ago by iannucci Modified:
3 years, 7 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
Description[step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure.
Previously an exception here would potentially allow the recipe to attempt
to obtain the active_result in a finally clause, which would obscure the
original error.
With this change, both exceptions will render.
R=tansell@chromium.org
BUG=721466
Review-Url: https://codereview.chromium.org/2885023004
Committed: https://github.com/luci/recipes-py/commit/632d231c826e13b02ade64d77ed2a41e25aa6fb3
Patch Set 1 #Patch Set 2 : include test #Patch Set 3 : fix test #Patch Set 4 : ... actually add expectation #
Total comments: 9
Patch Set 5 : fix nits #
Messages
Total messages: 32 (23 generated)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Running the test recipe shows:
.... some normal stuff ...
some step
@@@STEP_CLOSED@@@
@@@SEED_STEP@bad step@@@
@@@SEED_STEP@Placeholder Exception@@@
@@@STEP_CURSOR@Placeholder Exception@@@
@@@STEP_STARTED@@@
@@@STEP_LOG_LINE@exception@Traceback (most recent call last):@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/step_runner.py", line 177, in
open_step@@@
@@@STEP_LOG_LINE@exception@ step_config,
recipe_test_api.DisabledTestData()@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/step_runner.py", line 537, in
render_step@@@
@@@STEP_LOG_LINE@exception@ new_cmd.extend(item.render(tdata))@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipes/engine_tests/step_stack_exhaustion.py", line
16, in render@@@
@@@STEP_LOG_LINE@exception@ raise StepFailure("EXPLOSION")@@@
@@@STEP_LOG_LINE@exception@StepFailure: Step Failure in None@@@
@@@STEP_LOG_END@exception@@@
@@@STEP_CLOSED@@@
@@@SEED_STEP@Uncaught Exception@@@
@@@STEP_CURSOR@Uncaught Exception@@@
@@@STEP_STARTED@@@
@@@STEP_LOG_LINE@exception@Traceback (most recent call last):@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/run.py", line 373, in _old_run@@@
@@@STEP_LOG_LINE@exception@ recipe_result = recipe_script.run(api,
properties)@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/loader.py", line 98, in run@@@
@@@STEP_LOG_LINE@exception@ self.run_steps, properties, self.PROPERTIES,
api=api)@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/loader.py", line 619, in
invoke_with_properties@@@
@@@STEP_LOG_LINE@exception@ **additional_args)@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/loader.py", line 580, in
_invoke_with_properties@@@
@@@STEP_LOG_LINE@exception@ return callable_obj(*props, **additional_args)@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipes/engine_tests/step_stack_exhaustion.py", line
28, in RunSteps@@@
@@@STEP_LOG_LINE@exception@ api.step.active_result # this will raise an
exception@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_modules/step/api.py", line 74, in
active_result@@@
@@@STEP_LOG_LINE@exception@ return self.step_client.previous_step_result()@@@
@@@STEP_LOG_LINE@exception@ File
"/s/infra/infra/recipes-py/recipe_engine/recipe_api.py", line 187, in
previous_step_result@@@
@@@STEP_LOG_LINE@exception@ 'No steps have been run yet, and you are asking
for a previous step '@@@
@@@STEP_LOG_LINE@exception@ValueError: No steps have been run yet, and you are
asking for a previous step result.@@@
@@@STEP_LOG_END@exception@@@
@@@STEP_CLOSED@@@
@@@SEED_STEP@Failure reason@@@
@@@STEP_CURSOR@Failure reason@@@
@@@STEP_STARTED@@@
@@@STEP_LOG_LINE@reason@Uncaught Exception: ValueError('No steps have been run
yet, and you are asking for a previous step result.',)@@@
@@@STEP_LOG_END@reason@@@
@@@STEP_CLOSED@@@
Which is ~equivalent to the bug. Landing this would reveal the actual underlying
error that's currently causing 721466.
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in an finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 ========== to ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in an finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 ==========
iannucci@chromium.org changed reviewers: + phajdan.jr@chromium.org, tandrii@chromium.org
+tandrii and phajdan
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/362e28441f185010)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
grammar nazi newbie question: why "an finally clause" and not "a finally clause"?
Description was changed from ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in an finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 ========== to ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in a finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 ==========
On 2017/05/17 07:50:34, tandrii(chromium) wrote: > grammar nazi newbie question: why "an finally clause" and not "a finally > clause"? ha, oops, fixed :)
lgtm with pep8 nazi nits https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... File recipe_engine/step_runner.py (right): https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... recipe_engine/step_runner.py:179: step_config = None # Make sure we use rendered step config. pep8 nazi: two spaces before # http://legacy.python.org/dev/peps/pep-0008/#inline-comments https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... recipe_engine/step_runner.py:424: step_config = None # Make sure we use rendered step config. ah, i see where it came from. Anyway, s/e #/e #/ https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... File recipes/engine_tests/step_stack_exhaustion.expected/basic.json (right): https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... recipes/engine_tests/step_stack_exhaustion.expected/basic.json:14: } i see, great! https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... File recipes/engine_tests/step_stack_exhaustion.py (right): https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... recipes/engine_tests/step_stack_exhaustion.py:1: # Copyright 2015 The LUCI Authors. All rights reserved. s/2015/2017 https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... recipes/engine_tests/step_stack_exhaustion.py:28: api.step.active_result # this will raise a ValueError s/t #/t #/
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... File recipe_engine/step_runner.py (right): https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... recipe_engine/step_runner.py:179: step_config = None # Make sure we use rendered step config. On 2017/05/17 08:00:18, tandrii(chromium) wrote: > pep8 nazi: two spaces before # > http://legacy.python.org/dev/peps/pep-0008/#inline-comments Done. https://codereview.chromium.org/2885023004/diff/60001/recipe_engine/step_runn... recipe_engine/step_runner.py:424: step_config = None # Make sure we use rendered step config. On 2017/05/17 08:00:18, tandrii(chromium) wrote: > ah, i see where it came from. Anyway, s/e #/e #/ Done. https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... File recipes/engine_tests/step_stack_exhaustion.py (right): https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... recipes/engine_tests/step_stack_exhaustion.py:1: # Copyright 2015 The LUCI Authors. All rights reserved. On 2017/05/17 08:00:18, tandrii(chromium) wrote: > s/2015/2017 Done. https://codereview.chromium.org/2885023004/diff/60001/recipes/engine_tests/st... recipes/engine_tests/step_stack_exhaustion.py:28: api.step.active_result # this will raise a ValueError On 2017/05/17 08:00:18, tandrii(chromium) wrote: > s/t #/t #/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/2885023004/#ps80001 (title: "fix nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495008584730000,
"parent_rev": "c2d0f5d807793894df762930bc5d2729297658bf", "commit_rev":
"632d231c826e13b02ade64d77ed2a41e25aa6fb3"}
Message was sent while issue was closed.
Description was changed from ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in a finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 ========== to ========== [step_runner] Add a Placeholder Exception step when rendering placeholders raises a StepFailure. Previously an exception here would potentially allow the recipe to attempt to obtain the active_result in a finally clause, which would obscure the original error. With this change, both exceptions will render. R=tansell@chromium.org BUG=721466 Review-Url: https://codereview.chromium.org/2885023004 Committed: https://github.com/luci/recipes-py/commit/632d231c826e13b02ade64d77ed2a41e25a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/recipes-py/commit/632d231c826e13b02ade64d77ed2a41e25a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
