From bec94610c37b4634b44adc2498eedb3176c8b87c Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 18 Apr 2024 17:37:14 -0400 Subject: [PATCH] Fix E-stopped runs being marked as `stopped` instead of `failed`. We permanently set the run result to `FAILED` or `STOPPED` when a stop is requested, not just when the engine is `.finish()`'d. This seems premature to me, but, taking it for granted, it means we need to make sure that we choose `FAILED` if the stop was requested because of an E-stop. Also, because of that premature setting of the run result, we need to make sure that when the E-stop-induced `FinishAction` comes in, the run result already being set doesn't stop it from setting the run error. --- .../opentrons/protocol_engine/state/commands.py | 16 +++++++++------- .../state/test_command_store_old.py | 11 ++++++++--- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 1ae0cb1ed68..b5805251046 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -345,9 +345,11 @@ def handle_action(self, action: Action) -> None: # noqa: C901 elif isinstance(action, StopAction): if not self._state.run_result: self._state.queue_status = QueueStatus.PAUSED - self._state.run_result = RunResult.STOPPED if action.from_estop: self._state.stopped_by_estop = True + self._state.run_result = RunResult.FAILED + else: + self._state.run_result = RunResult.STOPPED elif isinstance(action, FinishAction): if not self._state.run_result: @@ -361,12 +363,12 @@ def handle_action(self, action: Action) -> None: # noqa: C901 else: self._state.run_result = RunResult.STOPPED - if action.error_details: - self._state.run_error = self._map_run_exception_to_error_occurrence( - action.error_details.error_id, - action.error_details.created_at, - action.error_details.error, - ) + if not self._state.run_error and action.error_details: + self._state.run_error = self._map_run_exception_to_error_occurrence( + action.error_details.error_id, + action.error_details.created_at, + action.error_details.error, + ) elif isinstance(action, HardwareStoppedAction): self._state.queue_status = QueueStatus.PAUSED diff --git a/api/tests/opentrons/protocol_engine/state/test_command_store_old.py b/api/tests/opentrons/protocol_engine/state/test_command_store_old.py index a859ae7573b..52d5aa961ce 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_store_old.py @@ -600,8 +600,13 @@ def test_command_store_handles_finish_action_with_stopped() -> None: assert subject.state.run_result == RunResult.STOPPED -@pytest.mark.parametrize("from_estop", [True, False]) -def test_command_store_handles_stop_action(from_estop: bool) -> None: +@pytest.mark.parametrize( + ["from_estop", "expected_run_result"], + [(True, RunResult.FAILED), (False, RunResult.STOPPED)], +) +def test_command_store_handles_stop_action( + from_estop: bool, expected_run_result: RunResult +) -> None: """It should mark the engine as non-gracefully stopped on StopAction.""" subject = CommandStore(is_door_open=False, config=_make_config()) @@ -615,7 +620,7 @@ def test_command_store_handles_stop_action(from_estop: bool) -> None: assert subject.state == CommandState( command_history=CommandHistory(), queue_status=QueueStatus.PAUSED, - run_result=RunResult.STOPPED, + run_result=expected_run_result, run_completed_at=None, is_door_blocking=False, run_error=None,