-
Notifications
You must be signed in to change notification settings - Fork 88
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. | ||||||
|
||||||
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", | ||||||
|
"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s", | |
"Chaining Choice state: Overwriting %s's current default_choice (%s) with %s", |
I think we should be consistent with calling it a state
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.
Agreed! Done in the latest commit :)
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.
Actually the SDK does refer to "steps" with states being a type of 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) | ||
|
||
) | ||
assert log_message in caplog.text | ||
|
||
assert s2_choice.default == s1_pass | ||
assert s2_choice.next_step is None # Choice steps do not have next_step | ||
|
||
|
||
|
||
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!