Step Functions: Mock feature for StartSyncExecution #12712
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
I have read the CLA Document and I hereby sign the CLA |
| @@ -772,6 +772,33 @@ def send_task_failure( | |||
| raise TaskDoesNotExist() | |||
| raise InvalidToken("Invalid token") | |||
|
|
|||
| @staticmethod | |||
| def _extract_base_arn(state_machine_arn: str) -> str: | |||
There was a problem hiding this comment.
There are multiple types of ARNs used in Step Functions, so the term “base ARN” might be a bit misleading. It might be clearer to move the logic for stripping the mock test case name into a different function, perhaps in _get_mock_test_case below. That way, we can both reuse the logic and make its purpose more immediately clear, returning the actual state machine ARN along with the mock test case name, if present. What do you think?
There was a problem hiding this comment.
I considered your suggestion first but decided for 2 functions instead of 1 to respect the single responsibility principle, since extracting the ARN and getting the mock test case seemed to be two unrelated tasks. I am not too opinionated about this though :)
Hence, I gave a try to your suggestion just now, and realised it's not super straightforward because:
- we need the extracted ARN to get the
state_machine_clone(line 825) - we need the
state_machine_clone.nameto get themock_test_case_name(line 867)
-> we have a circular dependency.
How would you suggest to solve this? Can I get the state machine name some other way than through the clone?
As an alternative I would suggest to simply rename _extract_base_arn to _get_state_machine_arn.
Let me know what you think.
| @@ -889,11 +899,13 @@ def start_sync_execution( | |||
| **kwargs, | |||
| ) -> StartSyncExecutionOutput: | |||
| self._validate_state_machine_arn(state_machine_arn) | |||
|
|
|||
| base_arn = self._extract_base_arn(state_machine_arn) | |||
There was a problem hiding this comment.
Following from the comment above, this could be reduced to a single statement with the one on line 943 (e.g. base_arn, mock_test_case = <util>(state_machine_arn)
| @@ -507,6 +509,27 @@ def launch_and_record_mocked_execution( | |||
| return execution_arn | |||
|
|
|||
|
|
|||
| def launch_and_record_mocked_sync_execution( | |||
There was a problem hiding this comment.
Thank you for adding this utility!
| execution_arn = launch_and_record_mocked_execution( | ||
| target_aws_client, sfn_snapshot, state_machine_arn, execution_input, test_name | ||
| ) | ||
| if state_machine_type == StateMachineType.EXPRESS: |
There was a problem hiding this comment.
There are STANDARD and EXPRESS state machines. LocalStack wants to enable the mock feature for both. However, when using the StartSyncExecution API action, only EXPRESS state machines can be executed, otherwise an error is raised. This utility seems to be limited to recording the executions through StartExecution only, as it invokes the asynchronous await logic.
What do you think about reverting the changes in this function, and adding the snapshotting features for mocked executions through StartSyncExecution in your launch_and_record_mocked_sync_execution? Also considering create_and_record_express_sync_execution. This could also allow us to test for failures if a STANDARD with mocking through StartSyncExecution.
There was a problem hiding this comment.
True! This is a use case I had forgotten.
Addressed here 78574bb, let me know what you think.
I added a create_and_record_mocked_sync_execution that always uses StateMachineType.EXPRESS, and left the argument state_machine_type for create_and_record_mocked_execution to make it possible to test the combination of StateMachineType.EXPRESS and StartExecution in the future.
Again, just to make sure we are on the same page, this is my understanding of how it works:
| Workflow Type | Asynchronous (StartExecution) |
Synchronous (StartSyncExecution) |
|---|---|---|
| Standard | ✅ Supported | ❌ Not supported |
| Express | ✅ Supported | ✅ Supported |
Motivation
Extend the Step Functions mocking capability to support
StartSyncExecution, enabling consistent testing across both Standard and Express state machines #12711Changes
LOCALSTACK_SFN_MOCK_CONFIG) when starting a state machine viaStartSyncExecutiontests/aws/services/stepfunctions/v2/mocking/test_base_scenarios.py::TestBaseScenarios::test_lambda_service_invoketo verify that the mocked service integration mechanism also works when usingStartSyncExecutionTesting
start-executionTODO
tests/aws/services/stepfunctions/v2/mocking/test_base_callbacks.py::TestBaseScenarios::test_sqs_wait_for_task_tokento verify that the callback mechanism (sync2) returns correctly when executed viaStartSyncExecution