Step Functions: Mock Mode Improvements #12560
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MEPalma
added
the
semver: minor
LocalStack Community integration with Pro 2 files ±0 2 suites ±0 1h 42m 12s ⏱️ +24s Results for commit dba8979. ± Comparison against base commit 936c697. This pull request removes 14 and adds 20 tests. Note that renamed tests count towards both.This pull request removes 14 skipped tests and adds 14 skipped tests. Note that renamed tests count towards both.This pull request skips 1 test.♻️ This comment has been updated with latest results. |
MEPalma
changed the title
MEPalma
requested review from
joe4dev,
gregfurman and
dominikschubert
as code owners
joe4dev
approved these changes
Apr 30, 2025
| @@ -334,6 +328,9 @@ def _after_eval_execution( | |||
| normalised_parameters: dict, | |||
| state_credentials: StateCredentials, | |||
| ) -> None: | |||
| # TODO: In Mock mode, when simulating a failure, the mock response is handled by | |||
There was a problem hiding this comment.
praise: Thanks for clarifying this parity limitation, specifying where and why clearly 👍
| outcome = self._eval_wait_for_task_token( | ||
| env=env, | ||
| timeout_seconds=timeout_seconds, | ||
| callback_endpoint=callback_endpoint, | ||
| heartbeat_endpoint=heartbeat_endpoint, | ||
| ) | ||
| elif env.is_mocked_mode(): |
| return self.mock_test_case is not None | ||
| return ( | ||
| self.mock_test_case is not None | ||
| and self.next_state_name in self.mock_test_case.state_mocked_responses |
There was a problem hiding this comment.
thought: interesting that the test case itself is unsufficient and we need to check forthe next state as well. Should we add a comment clarifying in which scenarios this is needed?
| message, | ||
| error_type, | ||
| ) | ||
| LOG.info( |
There was a problem hiding this comment.
question: What changes will/can be applied in this case of having validation errors?
Same for JSONDecodeError.
There was a problem hiding this comment.
This was hinting to the users' ability to edit the file and try again without restarting localstack. However, I'll comment this out with a TODO as this is currently not being tested ahead of 4.4
There was a problem hiding this comment.
I see. Maybe, we can reformulate this using active voice suggesting to the user to update and save the file, indicating that no LocalStack restart is needed. Maybe something alone the lines:
Please update and save the file and try again. No LocalStack restart needed.
Do we need a dedicated info message for this? Wouldn't it be nicer to include actionable advice right next to the error message containing potential validation suggestions?
There was a problem hiding this comment.
As this feature won't make it publicly into 4.4 I'd be happy to revise this in future changes; the rephrasing should be nice. For the sakes of good order, these were the motivations I had for the current design:
- On parse error, the runtime provides multiple detailed logs for all of the issues encountered. This is done with one log
errormessage per error and are of the form"Mock configuration file error at <location>: <message> (<type>)". This allows the user to easily read each error about the configuration file. I am of the opinion that chaining all of the errors in one log would not always be readable, especially if there are multiple errors. Hence:- Appending at the end of this the suggestion of hot-reloading being available I would say is not ideal as it would not be seen by all.
- At the same time, appending this suggestion at the end of each message seems perhaps excessive and not always readable is the error is long, but otherwise I reckon closer to a better solution
- Informing (
info) the user regarding the hot-reloading feature is different in nature from an error message (error). However, using an active language in doing so might reduce the significance of this consideration
| "ErrorEquals": [ | ||
| "States.ALL" | ||
| ], | ||
| // The aws demo calls for "MaxAttempts: 3" and 4 retry outcomes in "RetryPath" test case. |
There was a problem hiding this comment.
docs: Can we add a reference/source to the aws demo?
| ], | ||
| // The aws demo calls for "MaxAttempts: 3" and 4 retry outcomes in "RetryPath" test case. | ||
| // However, through snapshot testing, we know that this is 1 too many retry outcomes for | ||
| // this definition. Hence, in an effort to keep parity with AWS Step Functions, we the |
There was a problem hiding this comment.
nit: missing verb "we the attempts numbers was adjusted to 4."
| assert event_10["type"] == "TaskFailed" | ||
|
|
||
| event_13 = events[13] | ||
| assert json.loads(event_13["taskSucceededEventDetails"]["output"]) == { |
There was a problem hiding this comment.
praise: kudos for tackling all these manual asserts 💯
| @markers.aws.validated | ||
| @markers.snapshot.skip_snapshot_verify( | ||
| paths=[ | ||
| # Reason: skipping events validation because in mock‐failure mode the |
There was a problem hiding this comment.
praise: thank you for giving an explicit reason here 🙇
| "previousEventId": 13, | ||
| "stateExitedEventDetails": { | ||
| "name": "ParallelState", | ||
| "output": "[{\"ExecutedVersion\":\"$LATEST\",\"Payload\":[\"string-literal\"],\"SdkHttpMetadata\":{\"AllHttpHeaders\":{\"X-Amz-Executed-Version\":[\"$LATEST\"],\"x-amzn-Remapped-Content-Length\":[\"0\"],\"Connection\":[\"keep-alive\"],\"x-amzn-RequestId\":[\"c455c86f-6cb5-4d5d-8a71-4f0a98b33059\"],\"Content-Length\":[\"18\"],\"Date\":[\"Mon, 28 Apr 2025 12:36:05 GMT\"],\"X-Amzn-Trace-Id\":[\"Root=1-680f7635-0182b6d14f9778376d202e25;Parent=7880c132e0e2009b;Sampled=0;Lineage=1:816810b0:0\"],\"Content-Type\":[\"application/json\"]},\"HttpHeaders\":{\"Connection\":\"keep-alive\",\"Content-Length\":\"18\",\"Content-Type\":\"application/json\",\"Date\":\"Mon, 28 Apr 2025 12:36:05 GMT\",\"X-Amz-Executed-Version\":\"$LATEST\",\"x-amzn-Remapped-Content-Length\":\"0\",\"x-amzn-RequestId\":\"c455c86f-6cb5-4d5d-8a71-4f0a98b33059\",\"X-Amzn-Trace-Id\":\"Root=1-680f7635-0182b6d14f9778376d202e25;Parent=7880c132e0e2009b;Sampled=0;Lineage=1:816810b0:0\"},\"HttpStatusCode\":200},\"SdkResponseMetadata\":{\"RequestId\":\"c455c86f-6cb5-4d5d-8a71-4f0a98b33059\"},\"StatusCode\":200},{\"ExecutedVersion\":\"$LATEST\",\"Payload\":\"input-event-['string-literal']\",\"SdkHttpMetadata\":{\"AllHttpHeaders\":{\"X-Amz-Executed-Version\":[\"$LATEST\"],\"x-amzn-Remapped-Content-Length\":[\"0\"],\"Connection\":[\"keep-alive\"],\"x-amzn-RequestId\":[\"bee07bb2-41e2-4c9f-9f6f-1a70696cc112\"],\"Content-Length\":[\"32\"],\"Date\":[\"Mon, 28 Apr 2025 12:36:05 GMT\"],\"X-Amzn-Trace-Id\":[\"Root=1-680f7635-5bd01853792e73951459090c;Parent=56aa9955dca169e9;Sampled=0;Lineage=1:786b2a01:0\"],\"Content-Type\":[\"application/json\"]},\"HttpHeaders\":{\"Connection\":\"keep-alive\",\"Content-Length\":\"32\",\"Content-Type\":\"application/json\",\"Date\":\"Mon, 28 Apr 2025 12:36:05 GMT\",\"X-Amz-Executed-Version\":\"$LATEST\",\"x-amzn-Remapped-Content-Length\":\"0\",\"x-amzn-RequestId\":\"bee07bb2-41e2-4c9f-9f6f-1a70696cc112\",\"X-Amzn-Trace-Id\":\"Root=1-680f7635-5bd01853792e73951459090c;Parent=56aa9955dca169e9;Sampled=0;Lineage=1:786b2a01:0\"},\"HttpStatusCode\":200},\"SdkResponseMetadata\":{\"RequestId\":\"bee07bb2-41e2-4c9f-9f6f-1a70696cc112\"},\"StatusCode\":200}]", |
There was a problem hiding this comment.
note[non-blocking]: this would require some transformation containing many changing items. It's ok to skip them, maybe worth mentioning the reason in the snapshot skip.
There was a problem hiding this comment.
Yes indeed, transforming this field would make it easier to assert and work on; but it is also true that with SFN we have some desire to match the string formatting too #11953.
We have an open project on this aspect, and will definitely like to come back to this in future efforts
| return execution_arn | ||
|
|
||
|
|
||
| def create_and_run_mock( |
simonrw
pushed a commit
that referenced
this pull request
May 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently, the Step Functions interpreter’s support for Mocked Integrations has several limitations. Specifically:
Changes
Future Work
A known limitation is that, in mock mode, when simulating a failure, the
TaskSubmittedevent is not recorded in the event history. This should be addressed in future efforts in this space. The automatic reloading on changes of the mock configuration file should also be tested.