-
Notifications
You must be signed in to change notification settings - Fork 19
fix: remove timeout from Invoke Config #340
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 1 commit
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 |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| import pytest | ||
|
|
||
| from aws_durable_execution_sdk_python.config import Duration, InvokeConfig | ||
| from aws_durable_execution_sdk_python.config import InvokeConfig | ||
| from aws_durable_execution_sdk_python.exceptions import ( | ||
| CallableRuntimeError, | ||
| ExecutionError, | ||
|
|
@@ -218,8 +218,8 @@ def test_invoke_handler_already_started(status): | |
|
|
||
|
|
||
| @pytest.mark.parametrize("status", [OperationStatus.STARTED, OperationStatus.PENDING]) | ||
| def test_invoke_handler_already_started_with_timeout(status): | ||
| """Test invoke_handler when operation is already started with timeout config.""" | ||
| def test_invoke_handler_already_started_suspends(status): | ||
| """Test invoke_handler when operation is already started suspends indefinitely.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
| mock_state.durable_execution_arn = "test_arn" | ||
|
|
||
|
|
@@ -232,9 +232,9 @@ def test_invoke_handler_already_started_with_timeout(status): | |
| mock_result = CheckpointedResult.create_from_operation(operation) | ||
| mock_state.get_checkpoint_result.return_value = mock_result | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_seconds(30)) | ||
| config = InvokeConfig[str, str]() | ||
|
|
||
| with pytest.raises(TimedSuspendExecution): | ||
| with pytest.raises(SuspendExecution): | ||
| invoke_handler( | ||
| function_name="test_function", | ||
| payload="test_input", | ||
|
|
@@ -261,7 +261,7 @@ def test_invoke_handler_new_operation(): | |
| started = CheckpointedResult.create_from_operation(started_op) | ||
| mock_state.get_checkpoint_result.side_effect = [not_found, started] | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_minutes(1)) | ||
| config = InvokeConfig[str, str]() | ||
|
|
||
| with pytest.raises( | ||
| SuspendExecution, match="Invoke invoke8 started, suspending for completion" | ||
|
|
@@ -288,62 +288,6 @@ def test_invoke_handler_new_operation(): | |
| assert operation_update.chained_invoke_options.function_name == "test_function" | ||
|
|
||
|
|
||
| def test_invoke_handler_new_operation_with_timeout(): | ||
| """Test invoke_handler when starting a new operation with timeout.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
| mock_state.durable_execution_arn = "test_arn" | ||
|
|
||
| not_found = CheckpointedResult.create_not_found() | ||
| started_op = Operation( | ||
| operation_id="invoke_test", | ||
| operation_type=OperationType.CHAINED_INVOKE, | ||
| status=OperationStatus.STARTED, | ||
| ) | ||
| started = CheckpointedResult.create_from_operation(started_op) | ||
| mock_state.get_checkpoint_result.side_effect = [not_found, started] | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_seconds(30)) | ||
|
|
||
| with pytest.raises(TimedSuspendExecution): | ||
| invoke_handler( | ||
| function_name="test_function", | ||
| payload="test_input", | ||
| state=mock_state, | ||
| operation_identifier=OperationIdentifier( | ||
| "invoke9", OperationSubType.CHAINED_INVOKE, None, "test_invoke" | ||
| ), | ||
| config=config, | ||
| ) | ||
|
|
||
|
|
||
| def test_invoke_handler_new_operation_no_timeout(): | ||
| """Test invoke_handler when starting a new operation without timeout.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
| mock_state.durable_execution_arn = "test_arn" | ||
|
|
||
| not_found = CheckpointedResult.create_not_found() | ||
| started_op = Operation( | ||
| operation_id="invoke_test", | ||
| operation_type=OperationType.CHAINED_INVOKE, | ||
| status=OperationStatus.STARTED, | ||
| ) | ||
| started = CheckpointedResult.create_from_operation(started_op) | ||
| mock_state.get_checkpoint_result.side_effect = [not_found, started] | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_seconds(0)) | ||
|
|
||
| with pytest.raises(SuspendExecution): | ||
| invoke_handler( | ||
| function_name="test_function", | ||
| payload="test_input", | ||
| state=mock_state, | ||
| operation_identifier=OperationIdentifier( | ||
| "invoke10", OperationSubType.CHAINED_INVOKE, None, "test_invoke" | ||
| ), | ||
| config=config, | ||
| ) | ||
|
|
||
|
|
||
| def test_invoke_handler_no_config(): | ||
| """Test invoke_handler when no config is provided.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
|
|
@@ -1067,8 +1011,8 @@ def test_invoke_immediate_response_already_completed(): | |
| assert mock_state.get_checkpoint_result.call_count == 1 | ||
|
|
||
|
|
||
| def test_invoke_immediate_response_with_timeout_immediate_success(): | ||
| """Test immediate success with timeout configuration.""" | ||
| def test_invoke_immediate_response_immediate_success(): | ||
| """Test immediate success response.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
| mock_state.durable_execution_arn = "test_arn" | ||
|
|
||
|
|
@@ -1079,13 +1023,13 @@ def test_invoke_immediate_response_with_timeout_immediate_success(): | |
| operation_type=OperationType.CHAINED_INVOKE, | ||
| status=OperationStatus.SUCCEEDED, | ||
| chained_invoke_details=ChainedInvokeDetails( | ||
| result=json.dumps("timeout_result") | ||
| result=json.dumps("immediate_result") | ||
| ), | ||
| ) | ||
| succeeded = CheckpointedResult.create_from_operation(succeeded_op) | ||
| mock_state.get_checkpoint_result.side_effect = [not_found, succeeded] | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_seconds(30)) | ||
| config = InvokeConfig[str, str]() | ||
|
|
||
| result = invoke_handler( | ||
| function_name="test_function", | ||
|
|
@@ -1098,15 +1042,12 @@ def test_invoke_immediate_response_with_timeout_immediate_success(): | |
| ) | ||
|
|
||
| # Verify result was returned without suspend | ||
| assert result == "timeout_result" | ||
| assert result == "immediate_result" | ||
| assert mock_state.get_checkpoint_result.call_count == 2 | ||
|
|
||
|
|
||
| def test_invoke_immediate_response_with_timeout_no_immediate_response(): | ||
| """Test no immediate response with timeout configuration. | ||
|
|
||
| When no immediate response, operation should suspend with timeout. | ||
| """ | ||
| def test_invoke_immediate_response_no_immediate_response(): | ||
|
Contributor
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.
Contributor
Author
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 catch, thanks @ayushiahjolia, CI flagged the same thing ( I've pushed a follow-up commit that drops the PR-added copies, so the more thorough versions on main (lines 817 and 936) now execute as intended. Lint is clean locally and all 42 tests in |
||
| """Test no immediate response — operation suspends indefinitely.""" | ||
| mock_state = Mock(spec=ExecutionState) | ||
| mock_state.durable_execution_arn = "test_arn" | ||
|
|
||
|
|
@@ -1120,10 +1061,9 @@ def test_invoke_immediate_response_with_timeout_no_immediate_response(): | |
| started = CheckpointedResult.create_from_operation(started_op) | ||
| mock_state.get_checkpoint_result.side_effect = [not_found, started] | ||
|
|
||
| config = InvokeConfig[str, str](timeout=Duration.from_seconds(30)) | ||
| config = InvokeConfig[str, str]() | ||
|
|
||
| # Verify operation suspends with timeout | ||
| with pytest.raises(TimedSuspendExecution): | ||
| with pytest.raises(SuspendExecution): | ||
| invoke_handler( | ||
| function_name="test_function", | ||
| payload="test_input", | ||
|
|
||
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.
pytest.raises(SuspendExecution) does not prove the change.
TimedSuspendExecution is a subclass of SuspendExecution (exceptions.py:330), so this assertion would still pass if we regressed to raising TimedSuspendExecution.
suggestion:
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.
this is a nit :-) not blocking