Fix resume forgetting previous job outcome (bugfix) (#1095)

* Use old job result if already set

* Subcommand use the old job result if set

* Metabox scenario to test this

* Clarify docstring

checkbox-ng/checkbox_ng/launcher/subcommands.py

@@ -509,10 +509,15 @@ def _get_autoresume_outcome_last_job(self, metadata):
         is used to automatically resume a session and assign an outcome to the
         job that interrupted the session. If the interruption is due to a
         noreturn job (for example, reboot), the job will be marked as passed,
-        else, if the job made Checkbox crash, it will be marked as crash
+        else, if the job made Checkbox crash, it will be marked as crash. If
+        the job has a recorded outcome (so the session was interrupted after
+        assigning the outcome and before starting a new job) it will be used
+        instead.
         """
         job_state = self.sa.get_job_state(metadata.running_job_name)
-        if "noreturn" in (job_state.job.flags or set()):
+        if job_state.result.outcome:
+            return job_state.result.outcome
+        elif "noreturn" in (job_state.job.flags or set()):
             return IJobResult.OUTCOME_PASS
         return IJobResult.OUTCOME_CRASH
 

checkbox-ng/checkbox_ng/launcher/test_subcommands.py

@@ -343,6 +343,9 @@ def test__get_autoresume_outcome_last_job_noreturn(self):
         self_mock = MagicMock()
         job_state = self_mock.sa.get_job_state()
         job_state.job.flags = "noreturn"
+        job_state.result.outcome = None
+        job_state.result.comments = None
+
         metadata_mock = MagicMock()
         metadata_mock.running_job_name = "running_metadata_job_name"
 
@@ -352,10 +355,13 @@ def test__get_autoresume_outcome_last_job_noreturn(self):
 
         self.assertEqual(outcome, IJobResult.OUTCOME_PASS)
 
-    def test__get_autoresume_outcome_last_job(self):
+    def test__get_autoresume_outcome_last_job_crashed(self):
         self_mock = MagicMock()
         job_state = self_mock.sa.get_job_state()
         job_state.job.flags = ""
+        job_state.result.outcome = None
+        job_state.result.comments = None
+
         metadata_mock = MagicMock()
         metadata_mock.running_job_name = "running_metadata_job_name"
 
@@ -365,6 +371,22 @@ def test__get_autoresume_outcome_last_job(self):
 
         self.assertEqual(outcome, IJobResult.OUTCOME_CRASH)
 
+    def test__get_autoresume_outcome_last_job_already_set(self):
+        self_mock = MagicMock()
+        job_state = self_mock.sa.get_job_state()
+        job_state.job.flags = ""
+        job_state.result.outcome = IJobResult.OUTCOME_PASS
+        job_state.result.comments = "Pre resume comment"
+
+        metadata_mock = MagicMock()
+        metadata_mock.running_job_name = "running_metadata_job_name"
+
+        outcome = Launcher._get_autoresume_outcome_last_job(
+            self_mock, metadata_mock
+        )
+
+        self.assertEqual(outcome, IJobResult.OUTCOME_PASS)
+
     def test__resumed_session(self):
         self_mock = MagicMock()
 

checkbox-ng/plainbox/impl/session/remote_assistant.py

@@ -765,7 +765,13 @@ def resume_by_id(self, session_id=None, overwrite_result_dict={}):
                     result_dict["outcome"] = IJobResult.OUTCOME_PASS
         except (json.JSONDecodeError, FileNotFoundError):
             the_job = self._sa.get_job(self._last_job)
-            if the_job.plugin == "shell":
+            job_state = self._sa.get_job_state(the_job.id)
+            # the last running job already had a result
+            if job_state.result.outcome:
+                result_dict["outcome"] = job_state.result.outcome
+                result_dict["comments"] = job_state.result.comments or ""
+            # job didnt have a result, lets automatically calculate it
+            elif the_job.plugin == "shell":
                 if "noreturn" in the_job.get_flag_set():
                     result_dict["outcome"] = IJobResult.OUTCOME_PASS
                     result_dict["comments"] = (

checkbox-ng/plainbox/impl/session/test_remote_assistant.py

@@ -229,6 +229,8 @@ def test_resume_by_id_with_result_no_file_noreturn(
         rsa._sa.get_resumable_sessions.return_value = [resumable_session]
         rsa.get_rerun_candidates.return_value = []
         rsa._state = remote_assistant.Idle
+        job_state = rsa._sa.get_job_state.return_value
+        job_state.result.outcome = None
 
         mock_meta = mock.Mock()
         mock_meta.app_blob = b'{"testplan_id": "tp_id"}'
@@ -267,6 +269,8 @@ def test_resume_by_id_with_result_no_file_normal(self, mock_load_configs):
         rsa._sa.get_resumable_sessions.return_value = [resumable_session]
         rsa.get_rerun_candidates.return_value = []
         rsa._state = remote_assistant.Idle
+        job_state = rsa._sa.get_job_state.return_value
+        job_state.result.outcome = None
 
         mock_meta = mock.Mock()
         mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}'
@@ -294,6 +298,43 @@ def test_resume_by_id_with_result_no_file_normal(self, mock_load_configs):
 
         rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True)
 
+    @mock.patch("plainbox.impl.session.remote_assistant.load_configs")
+    def test_resume_by_id_with_result_no_file_already_set(
+        self, mock_load_configs
+    ):
+        rsa = mock.Mock()
+        resumable_session = mock.Mock()
+        resumable_session.id = "session_id"
+        rsa._sa.get_resumable_sessions.return_value = [resumable_session]
+        rsa.get_rerun_candidates.return_value = []
+        rsa._state = remote_assistant.Idle
+        job_state = rsa._sa.get_job_state.return_value
+        job_state.result.outcome = IJobResult.OUTCOME_PASS
+        job_state.result.comments = None
+
+        mock_meta = mock.Mock()
+        mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}'
+
+        rsa.resume_session.return_value = mock_meta
+        os_path_exists_mock = mock.Mock()
+
+        rsa._sa.get_job.return_value.plugin = "shell"
+
+        with mock.patch("os.path.exists", os_path_exists_mock):
+            os_path_exists_mock.return_value = False
+            rsa._sa.get_job.return_value.get_flag_set.return_value = {}
+
+            remote_assistant.RemoteSessionAssistant.resume_by_id(rsa)
+
+        mjr = MemoryJobResult(
+            {
+                "outcome": IJobResult.OUTCOME_PASS,
+                "comments": "",
+            }
+        )
+
+        rsa._sa.use_job_result.assert_called_with(rsa._last_job, mjr, True)
+
     @mock.patch("plainbox.impl.session.remote_assistant.load_configs")
     def test_resume_by_id_with_result_file_not_json(self, mock_load_configs):
         rsa = mock.Mock()
@@ -302,6 +343,8 @@ def test_resume_by_id_with_result_file_not_json(self, mock_load_configs):
         rsa._sa.get_resumable_sessions.return_value = [resumable_session]
         rsa.get_rerun_candidates.return_value = []
         rsa._state = remote_assistant.Idle
+        job_state = rsa._sa.get_job_state.return_value
+        job_state.result.outcome = None
 
         mock_meta = mock.Mock()
         mock_meta.app_blob = b'{"launcher": "", "testplan_id": "tp_id"}'

metabox/metabox/metabox-provider/units/resume.pxu

@@ -16,9 +16,26 @@ command:
  PID=`pgrep -f checkbox-cli`
  kill $PID
 
+id: pass-rerun
+unit: job
+_summary: Test that passes only at the second try
+flags: simple
+command:
+ [ -f $PLAINBOX_SESSION_SHARE/will_pass.txt ] && exit 0
+ echo "will pass next time" > $PLAINBOX_SESSION_SHARE/will_pass.txt
+ exit 1
+
 unit: test plan
 id: checkbox-crash-then-reboot
 _name: Checkbox crash then reboot
 include:
  checkbox-crasher
  reboot-emulator
+
+unit: test plan
+id: pass-only-rerun
+_name: Pass only on rerun
+_summary: Test that passes only at the secont try
+include:
+ pass-rerun
+ basic-shell-failing

metabox/metabox/scenarios/restart/agent_respawn.py

@@ -22,7 +22,8 @@
     SelectTestPlan,
     Send,
     Expect,
-    Start
+    Start,
+    Signal,
 )
 from metabox.core.scenario import Scenario
 from metabox.core.utils import tag
@@ -102,3 +103,60 @@ class AutoResumeAfterCrashAutoLocal(Scenario):
         AssertPrinted("job passed"),
         AssertPrinted("Emulate the reboot"),
     ]
+
+
+@tag("resume", "manual")
+class ResumeAfterFinishPreserveOutputLocal(Scenario):
+    modes = ["local"]
+    launcher = "# no launcher"
+    steps = [
+        Start(),
+        Expect("Select test plan"),
+        SelectTestPlan("2021.com.canonical.certification::pass-only-rerun"),
+        Send(keys.KEY_ENTER),
+        Expect("Press (T) to start"),
+        Send("T"),
+        Expect("Select jobs to re-run"),
+        Send(keys.KEY_SPACE),
+        Expect("[X]"),
+        Send("r"),
+        Expect("Select jobs to re-run"),
+        Signal(keys.SIGINT),
+        Start(),
+        Expect("Select jobs to re-run"),
+        Send("f"),
+        Expect("job passed"),
+        Expect("job failed"),
+    ]
+
+@tag("resume", "manual")
+class ResumeAfterFinishPreserveOutputRemote(Scenario):
+    modes = ["remote"]
+    launcher = "# no launcher"
+    steps = [
+        Start(),
+        Expect("Select test plan"),
+        SelectTestPlan("2021.com.canonical.certification::pass-only-rerun"),
+        Send(keys.KEY_ENTER),
+        Expect("Press (T) to start"),
+        Send("T"),
+        Expect("Select jobs to re-run"),
+        Send(keys.KEY_SPACE),
+        Expect("[X]"),
+        Send("r"),
+        Expect("Select jobs to re-run"),
+        Signal(keys.SIGINT),
+        Expect("(X) Nothing"),
+        Send(keys.KEY_DOWN + keys.KEY_SPACE),
+        Expect("(X) Stop"),
+        Send(keys.KEY_DOWN + keys.KEY_SPACE),
+        Expect("(X) Pause"),
+        Send(keys.KEY_DOWN + keys.KEY_SPACE),
+        Expect("(X) Exit"),
+        Send(keys.KEY_ENTER),
+        Start(),
+        Expect("Select jobs to re-run"),
+        Send("f"),
+        Expect("job passed"),
+        Expect("job failed"),
+    ]