-
Notifications
You must be signed in to change notification settings - Fork 89
Make Choice states chainable #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
23601e5
eb6fc49
6563f1d
851023f
2c72de7
bce03fb
c4811f3
44882aa
aa82ca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -218,9 +218,21 @@ def next(self, next_step): | |||||
Returns: | ||||||
State or Chain: Next state or chain that will be transitioned to. | ||||||
""" | ||||||
if self.type in ('Choice', 'Succeed', 'Fail'): | ||||||
if self.type in ('Succeed', 'Fail'): | ||||||
raise ValueError('Unexpected State instance `{step}`, State type `{state_type}` does not support method `next`.'.format(step=next_step, state_type=self.type)) | ||||||
|
||||||
# By design, choice states do not have the Next field. Setting default to make it chainable. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explain the there's some great info in the gist that @wong-a created that could be paraphrased. Might also be helpful to link to the language spec There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! |
||||||
if self.type is 'Choice': | ||||||
if self.default is not None: | ||||||
logger.warning( | ||||||
"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we should be consistent with calling it a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! Done in the latest commit :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the SDK does refer to "steps" with states being a type of step. |
||||||
self.state_id, | ||||||
self.default.state_id, | ||||||
next_step.state_id | ||||||
) | ||||||
self.default_choice(next_step) | ||||||
return self.default | ||||||
|
||||||
self.next_step = next_step | ||||||
return self.next_step | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
# permissions and limitations under the License. | ||
from __future__ import absolute_import | ||
|
||
import logging | ||
import pytest | ||
|
||
from stepfunctions.exceptions import DuplicateStatesInChain | ||
|
@@ -328,12 +329,6 @@ def test_append_states_after_terminal_state_will_fail(): | |
chain.append(Succeed('Succeed')) | ||
chain.append(Pass('Pass2')) | ||
|
||
with pytest.raises(ValueError): | ||
chain = Chain() | ||
chain.append(Pass('Pass')) | ||
chain.append(Choice('Choice')) | ||
chain.append(Pass('Pass2')) | ||
|
||
|
||
def test_chaining_steps(): | ||
s1 = Pass('Step - One') | ||
|
@@ -372,6 +367,33 @@ def test_chaining_steps(): | |
assert s1.next_step == s2 | ||
assert s2.next_step == s3 | ||
|
||
|
||
def test_chaining_choice(caplog): | ||
s1_pass = Pass('Step - One') | ||
s2_choice = Choice('Step - Two') | ||
s3_pass = Pass('Step - Three') | ||
|
||
with caplog.at_level(logging.WARNING): | ||
chain1 = Chain([s1_pass, s2_choice, s3_pass]) | ||
assert caplog.text == '' # No warning | ||
assert chain1.steps == [s1_pass, s2_choice, s3_pass] | ||
assert s1_pass.next_step == s2_choice | ||
assert s2_choice.default == s3_pass | ||
assert s2_choice.next_step is None # Choice steps do not have next_step | ||
assert s3_pass.next_step is None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When writing unit tests for these modules we should consider making assertions on the generated JSON too. One of the main responsibilities is generating ASL so we should make sure it's correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! Will include assertions in the generated JSON in the next commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed offline, there is already a unit test (test_choice_state_creation) that ensures the Choice state creation generates the expected ASL |
||
|
||
# Chain s2_choice when default_choice is already set will trigger Warning | ||
with caplog.at_level(logging.WARNING): | ||
wong-a marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Chain([s2_choice, s1_pass]) | ||
log_message = ( | ||
"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s" % | ||
(s2_choice.state_id, s3_pass.state_id, s1_pass.state_id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: might be more clear to just establish the expected string and call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! Made the changes in latest commit |
||
) | ||
assert log_message in caplog.text | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does the log include other stuff? curious why we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it includes the logging type (in our case WARNING) and the line(and file) where it occurred. |
||
assert s2_choice.default == s1_pass | ||
assert s2_choice.next_step is None # Choice steps do not have next_step | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm not sure whether this is an idiomatic Python thing, but I'd extract this into a separate test: the way I interpret the control flow, there are 2 scenarios and sets of assertions. The 2 tests from what i can tell:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, it makes sense to divide the test in 2! |
||
|
||
|
||
def test_catch_fail_for_unsupported_state(): | ||
s1 = Pass('Step - One') | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer these in a separate PR to observe our guidance to focus on the change being introduced. this PR is small so it doesn't take away focus, but just something to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
Removed the changes into another PR!