From 2490731e1ded17adfc47e429689693b17299ebc3 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Tue, 5 Mar 2024 18:37:17 +0800
Subject: [PATCH 01/10] Ensure that jobs that have an after suspend counterpart
 are run before suspend

Some jobs should be run before *and* after suspend. To take this into
account, the `siblings` field was developed, along with the
`also-after-suspend` and `also-after-suspend-manual` flags[2].

These would let Checkbox spawn a similar job with an added dependency on
the suspend job (either the manual or the automated version of it).

The problem is that the original job did not have any dependency to
force it to be run *before* the suspend job.

This was not an issue for test plans organized manually, using regular
expressions, because you could have an include section that looks like:

    storage_.*
    com.canonical.certification::suspend/suspend_advanced_auto
    after-suspend-storage.*

However, now that template ids can be added in test plans[3], this is a
problem.

This patch will make sure the jobs that need to run before suspend are
added as dependencies of their related suspend job, so that regardless
of the order in the test plan, they will be run in the proper order.

Fix: #1010

[1]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#job-siblings-field
[2]
https://checkbox.readthedocs.io/en/stable/reference/units/job.html#also-after-suspend-flag
[3]
https://checkbox.readthedocs.io/en/latest/reference/units/test-plan.html
---
 checkbox-ng/plainbox/configuration.py |  30 +++++++
 checkbox-ng/plainbox/impl/ctrl.py     | 109 +++++++++++++++++++++++++-
 checkbox-ng/plainbox/impl/depmgr.py   |  10 ++-
 3 files changed, 144 insertions(+), 5 deletions(-)
 create mode 100644 checkbox-ng/plainbox/configuration.py

diff --git a/checkbox-ng/plainbox/configuration.py b/checkbox-ng/plainbox/configuration.py
new file mode 100644
index 000000000..2bb0dec1b
--- /dev/null
+++ b/checkbox-ng/plainbox/configuration.py
@@ -0,0 +1,30 @@
+# This file is part of Checkbox.
+#
+# Copyright 2024 Canonical Ltd.
+# Written by:
+#   Pierre Equoy <pierre.equoy@canonical.com>
+#
+# Checkbox is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3,
+# as published by the Free Software Foundation.
+
+#
+# Checkbox is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
+
+"""
+:mod:`plainbox.impl.configuration` -- Configuration variables
+=============================================================
+"""
+
+
+class Suspend:
+    AUTO_JOB_ID = "com.canonical.certification::suspend/suspend_advanced_auto"
+    MANUAL_JOB_ID = "com.canonical.certification::suspend/suspend_advanced"
+    AUTO_FLAG = "also-after-suspend"
+    MANUAL_FLAG = "also-after-suspend-manual"
diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py
index 03aee471b..01752c8bf 100644
--- a/checkbox-ng/plainbox/impl/ctrl.py
+++ b/checkbox-ng/plainbox/impl/ctrl.py
@@ -53,6 +53,7 @@ class to select the best method to execute a command of a particular job.  This
 
 from plainbox.abc import IJobResult
 from plainbox.abc import ISessionStateController
+from plainbox.configuration import Suspend
 from plainbox.i18n import gettext as _
 from plainbox.impl import get_plainbox_dir
 from plainbox.impl.depmgr import DependencyDuplicateError
@@ -102,12 +103,14 @@ class CheckBoxSessionStateController(ISessionStateController):
           of resource definitions.
     """
 
-    def get_dependency_set(self, job):
+    def get_dependency_set(self, job, job_list=None):
         """
         Get the set of direct dependencies of a particular job.
 
         :param job:
             A IJobDefinition instance that is to be visited
+        :param job_list:
+            List of jobs to check dependencies from
         :returns:
             set of pairs (dep_type, job_id)
 
@@ -125,15 +128,64 @@ def get_dependency_set(self, job):
             resource_deps = job.get_resource_dependencies()
         except ResourceProgramError:
             resource_deps = ()
+        suspend_job_id_list = [
+            Suspend.AUTO_JOB_ID,
+            Suspend.MANUAL_JOB_ID,
+        ]
+        if job.id in suspend_job_id_list:
+            suspend_deps = self._get_before_suspend_dependency_set(
+                job.id, job_list
+            )
+        else:
+            suspend_deps = set()
         result = set(
             itertools.chain(
                 zip(itertools.repeat(direct), direct_deps),
                 zip(itertools.repeat(resource), resource_deps),
                 zip(itertools.repeat(ordering), after_deps),
+                zip(itertools.repeat(ordering), suspend_deps),
             )
         )
         return result
 
+    def _get_before_suspend_dependency_set(self, suspend_job_id, job_list):
+        """
+        Get the set of after dependencies of a suspend job.
+
+        Jobs that have a ``also-after-suspend[-manual]`` flag should be run
+        before their associated suspend job. Similary, jobs that declare a
+        sibling with a dependency on a suspend job should be run before said
+        suspend job. This function finds these jobs and add them as a
+        dependency for their associated suspend job.
+
+        :param suspend_job_id:
+            The id of a suspend job. One of the following is expected:
+            Suspend.AUTO_JOB_ID or Suspend.MANUAL_JOB_ID.
+        :param job_list:
+            List of jobs to search dependencies on.
+        :returns:
+            A set of job ids that need to be run before the suspend job
+        """
+        suspend_deps = set()
+        expected_flag = ""
+        if suspend_job_id == Suspend.AUTO_JOB_ID:
+            expected_flag = Suspend.AUTO_FLAG
+        elif suspend_job_id == Suspend.MANUAL_JOB_ID:
+            expected_flag = Suspend.MANUAL_FLAG
+        if not expected_flag:
+            return suspend_deps
+        for dep_job in job_list:
+            if dep_job.flags and expected_flag in dep_job.flags:
+                suspend_deps.add(dep_job.id)
+            if dep_job.siblings:
+                for sibling_data in json.loads(dep_job.tr_siblings()):
+                    if (
+                        sibling_data.get("depends")
+                        and suspend_job_id in sibling_data["depends"]
+                    ):
+                        suspend_deps.add(dep_job.id)
+        return suspend_deps
+
     def get_inhibitor_list(self, session_state, job):
         """
         Get a list of readiness inhibitors that inhibit a particular job.
@@ -236,8 +288,63 @@ def get_inhibitor_list(self, session_state, job):
                     related_job=dep_job_state.job,
                 )
                 inhibitors.append(inhibitor)
+        if job.id in [Suspend.AUTO_JOB_ID, Suspend.MANUAL_JOB_ID]:
+            for inhibitor in self._get_suspend_inhibitor_list(
+                session_state, job
+            ):
+                inhibitors.append(inhibitor)
         return inhibitors
 
+    def _get_suspend_inhibitor_list(self, session_state, suspend_job):
+        """
+        Get a list of readiness inhibitors that inhibit a suspend job.
+
+        Jobs that have a ``also-after-suspend[-manual]`` flag should be run
+        before their associated suspend job. Similary, jobs that declare a
+        sibling with a dependency on a suspend job should be run before said
+        suspend job. This function finds these jobs and add them as a
+        inhibitor for their associated suspend job.
+
+        :param session_state:
+            A SessionState instance that is used to interrogate the
+            state of the session where it matters for a particular
+            job. Currently this is used to access resources and job
+            results.
+        :param suspend_job:
+            A suspend job.
+        :returns:
+            List of JobReadinessInhibitor
+        """
+        suspend_inhibitors = []
+        expected_flag = ""
+        if suspend_job.id == Suspend.AUTO_JOB_ID:
+            expected_flag = Suspend.AUTO_FLAG
+        elif suspend_job.id == Suspend.MANUAL_JOB_ID:
+            expected_flag = Suspend.MANUAL_FLAG
+        if not expected_flag:
+            return suspend_inhibitors
+        for job_id, job_state in session_state.job_state_map.items():
+            if job_state.job.flags and expected_flag in job_state.job.flags:
+                if job_state.result.outcome == IJobResult.OUTCOME_NONE:
+                    inhibitor = JobReadinessInhibitor(
+                        cause=InhibitionCause.PENDING_DEP,
+                        related_job=job_state.job,
+                    )
+                    suspend_inhibitors.append(inhibitor)
+            if job_state.job.siblings:
+                for sibling_data in json.loads(job_state.job.tr_siblings()):
+                    if (
+                        sibling_data.get("depends")
+                        and suspend_job.id in sibling_data["depends"]
+                    ):
+                        if job_state.result.outcome == IJobResult.OUTCOME_NONE:
+                            inhibitor = JobReadinessInhibitor(
+                                cause=InhibitionCause.PENDING_DEP,
+                                related_job=job_state.job,
+                            )
+                        suspend_inhibitors.append(inhibitor)
+        return suspend_inhibitors
+
     def observe_result(self, session_state, job, result, fake_resources=False):
         """
         Notice the specified test result and update readiness state.
diff --git a/checkbox-ng/plainbox/impl/depmgr.py b/checkbox-ng/plainbox/impl/depmgr.py
index f91bc2f5b..d0e9c7c07 100644
--- a/checkbox-ng/plainbox/impl/depmgr.py
+++ b/checkbox-ng/plainbox/impl/depmgr.py
@@ -340,12 +340,12 @@ def _solve(self, visit_list=None):
         if visit_list is None:
             visit_list = self._job_list
         for job in visit_list:
-            self._visit(job)
+            self._visit(job=job, visit_list=visit_list)
         logger.debug(_("Done solving"))
         # Return the solution
         return self._solution
 
-    def _visit(self, job, trail=None):
+    def _visit(self, job, visit_list, trail=None):
         """
         Internal method of DependencySolver.
 
@@ -367,7 +367,9 @@ def _visit(self, job, trail=None):
             # If the trail was not specified start a trail for this node
             if trail is None:
                 trail = [job]
-            for dep_type, job_id in job.controller.get_dependency_set(job):
+            for dep_type, job_id in job.controller.get_dependency_set(
+                job, visit_list
+            ):
                 # Dependency is just an id, we need to resolve it
                 # to a job instance. This can fail (missing dependencies)
                 # so let's guard against that.
@@ -383,7 +385,7 @@ def _visit(self, job, trail=None):
                     logger.debug(_("Visiting dependency: %r"), next_job)
                     # Update the trail as we visit that node
                     trail.append(next_job)
-                    self._visit(next_job, trail)
+                    self._visit(next_job, visit_list, trail)
                     trail.pop()
             # We've visited (recursively) all dependencies of this node,
             # let's color it black and append it to the solution list.

From 34c39a5fd2b1a9670da3b870fac0b8494dd088ae Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Tue, 5 Mar 2024 18:37:32 +0800
Subject: [PATCH 02/10] Add unit tests

---
 checkbox-ng/plainbox/impl/test_ctrl.py | 250 +++++++++++++++++++++++++
 1 file changed, 250 insertions(+)

diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py
index fa31d835d..c2084b5d1 100644
--- a/checkbox-ng/plainbox/impl/test_ctrl.py
+++ b/checkbox-ng/plainbox/impl/test_ctrl.py
@@ -26,12 +26,14 @@
 
 from subprocess import CalledProcessError
 from unittest import TestCase
+import json
 import os
 import shutil
 
 from plainbox.abc import IJobResult
 from plainbox.abc import IProvider1
 from plainbox.abc import IProviderBackend1
+from plainbox.configuration import Suspend
 from plainbox.impl.ctrl import CheckBoxSessionStateController
 from plainbox.impl.ctrl import SymLinkNest
 from plainbox.impl.ctrl import gen_rfc822_records_from_io_log
@@ -102,6 +104,66 @@ def test_get_dependency_set(self):
         self.assertEqual(
             self.ctrl.get_dependency_set(job_f),
             {('direct', 'j6'), ('resource', 'j6')})
+        # Job with an "also-after-suspend" flag, meaning this job should be
+        # set to run before the suspend job
+        job_g = JobDefinition({
+            "id": "j7",
+            "flags": Suspend.AUTO_FLAG
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.AUTO_JOB_ID
+        })
+        self.assertEqual(
+            self.ctrl.get_dependency_set(suspend_job, [job_g]),
+            {("ordering", "j7")}
+        )
+
+    def test_get_before_suspend_dependency_set__not_suspend_job_id(self):
+        self.assertEqual(
+            self.ctrl._get_before_suspend_dependency_set("not-suspend", []),
+            set()
+        )
+
+    def test_get_before_suspend_dependency_set__manual_suspend(self):
+        job1 = JobDefinition({
+            "id": "job1",
+            "flags": Suspend.MANUAL_FLAG
+        })
+        job2 = JobDefinition({
+            "id": "job2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.MANUAL_JOB_ID
+        })
+        self.assertEqual(
+            self.ctrl._get_before_suspend_dependency_set(
+                suspend_job.id,
+                [job1, job2]
+            ),
+            {"job1"}
+        )
+
+    def test_get_before_suspend_dependency_set__sibling(self):
+        job1 = JobDefinition({
+            "id": "job1",
+            "siblings": json.dumps([{
+                "id": "sibling-j1",
+                "depends": Suspend.AUTO_JOB_ID,
+            }])
+        })
+        job2 = JobDefinition({
+            "id": "job2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.AUTO_JOB_ID
+        })
+        self.assertEqual(
+            self.ctrl._get_before_suspend_dependency_set(
+                suspend_job.id,
+                [job1, job2]
+            ),
+            {"job1"}
+        )
 
     def test_get_inhibitor_list_PENDING_RESOURCE(self):
         # verify that jobs that require a resource that hasn't been
@@ -227,6 +289,30 @@ def test_get_inhibitor_list_FAILED_DEP(self):
             [JobReadinessInhibitor(
                 InhibitionCause.FAILED_DEP, j2, None)])
 
+    def test_get_inhibitor_list_NOT_FAILED_DEP(self):
+        # verify that jobs that depend on another job that ran but
+        # didn't result in OUTCOME_FAIL produce the NOT_FAILED_DEP
+        # inhibitor.
+        j1 = JobDefinition({
+            'id': 'j1',
+            'salvages': 'j2',
+        })
+        j2 = JobDefinition({
+            'id': 'j2'
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            'j1': mock.Mock(spec_set=JobState),
+            'j2': mock.Mock(spec_set=JobState),
+        }
+        jsm_j2 = session_state.job_state_map['j2']
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl.get_inhibitor_list(session_state, j1),
+            [JobReadinessInhibitor(
+                InhibitionCause.NOT_FAILED_DEP, j2, None)])
+
     def test_get_inhibitor_list_good_dep(self):
         # verify that jobs that depend on another job that ran and has outcome
         # equal to OUTCOME_PASS don't have any inhibitors
@@ -256,6 +342,170 @@ def test_get_inhibitor_list_good_dep(self):
         self.assertEqual(
             self.ctrl.get_inhibitor_list(session_state, j1), [])
 
+    def test_get_inhibitor_list__suspend_job(self):
+        j1 = JobDefinition({
+            "id": "j1",
+            "flags": Suspend.AUTO_FLAG,
+        })
+        j2 = JobDefinition({
+            "id": "j2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.AUTO_JOB_ID
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            "j1": mock.Mock(spec_set=JobState),
+            "j2": mock.Mock(spec_set=JobState),
+            Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState),
+        }
+        jsm_j1 = session_state.job_state_map["j1"]
+        jsm_j1.job = j1
+        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2 = session_state.job_state_map["j2"]
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
+        jsm_suspend.job = suspend_job
+        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl.get_inhibitor_list(session_state, suspend_job),
+            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+
+    def test_get_suspend_inhibitor_list__not_suspend_job_id(self):
+        j1 = JobDefinition({
+            "id": "not-a-suspend-job"
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        self.assertEqual(
+            self.ctrl._get_suspend_inhibitor_list(session_state, j1), [])
+
+    def test_get_suspend_inhibitor_list__auto_suspend(self):
+        j1 = JobDefinition({
+            "id": "j1",
+            "flags": Suspend.AUTO_FLAG,
+        })
+        j2 = JobDefinition({
+            "id": "j2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.AUTO_JOB_ID
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            "j1": mock.Mock(spec_set=JobState),
+            "j2": mock.Mock(spec_set=JobState),
+            Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState),
+        }
+        jsm_j1 = session_state.job_state_map["j1"]
+        jsm_j1.job = j1
+        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2 = session_state.job_state_map["j2"]
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
+        jsm_suspend.job = suspend_job
+        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
+            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+
+    def test_get_suspend_inhibitor_list__manual_suspend(self):
+        j1 = JobDefinition({
+            "id": "j1",
+            "flags": Suspend.MANUAL_FLAG,
+        })
+        j2 = JobDefinition({
+            "id": "j2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.MANUAL_JOB_ID
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            "j1": mock.Mock(spec_set=JobState),
+            "j2": mock.Mock(spec_set=JobState),
+            Suspend.MANUAL_JOB_ID: mock.Mock(spec_set=JobState),
+        }
+        jsm_j1 = session_state.job_state_map["j1"]
+        jsm_j1.job = j1
+        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2 = session_state.job_state_map["j2"]
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend = session_state.job_state_map[Suspend.MANUAL_JOB_ID]
+        jsm_suspend.job = suspend_job
+        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
+            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+
+    def test_get_suspend_inhibitor_list__auto_sibling(self):
+        j1 = JobDefinition({
+            "id": "j1",
+            "siblings": json.dumps([{
+                "id": "sibling-j1",
+                "depends": Suspend.AUTO_JOB_ID,
+            }])
+        })
+        j2 = JobDefinition({
+            "id": "j2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.AUTO_JOB_ID
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            "j1": mock.Mock(spec_set=JobState),
+            "j2": mock.Mock(spec_set=JobState),
+            Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState),
+        }
+        jsm_j1 = session_state.job_state_map["j1"]
+        jsm_j1.job = j1
+        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2 = session_state.job_state_map["j2"]
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
+        jsm_suspend.job = suspend_job
+        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
+            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+
+    def test_get_suspend_inhibitor_list__manual_sibling(self):
+        j1 = JobDefinition({
+            "id": "j1",
+            "siblings": json.dumps([{
+                "id": "sibling-j1",
+                "depends": Suspend.MANUAL_JOB_ID,
+            }])
+        })
+        j2 = JobDefinition({
+            "id": "j2",
+        })
+        suspend_job = JobDefinition({
+            "id": Suspend.MANUAL_JOB_ID
+        })
+        session_state = mock.MagicMock(spec=SessionState)
+        session_state.job_state_map = {
+            "j1": mock.Mock(spec_set=JobState),
+            "j2": mock.Mock(spec_set=JobState),
+            Suspend.MANUAL_JOB_ID: mock.Mock(spec_set=JobState),
+        }
+        jsm_j1 = session_state.job_state_map["j1"]
+        jsm_j1.job = j1
+        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2 = session_state.job_state_map["j2"]
+        jsm_j2.job = j2
+        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend = session_state.job_state_map[Suspend.MANUAL_JOB_ID]
+        jsm_suspend.job = suspend_job
+        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        self.assertEqual(
+            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
+            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+
     def test_observe_result__normal(self):
         job = mock.Mock(spec=JobDefinition)
         result = mock.Mock(spec=IJobResult)

From 1a06bdf6bed5d6a84c986bf4b0ab1772b03db200 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Tue, 5 Mar 2024 21:38:32 +0800
Subject: [PATCH 03/10] Rename module with Suspend constants

---
 checkbox-ng/plainbox/impl/ctrl.py                            | 3 +--
 checkbox-ng/plainbox/impl/test_ctrl.py                       | 3 +--
 checkbox-ng/plainbox/{configuration.py => suspend_consts.py} | 5 ++---
 3 files changed, 4 insertions(+), 7 deletions(-)
 rename checkbox-ng/plainbox/{configuration.py => suspend_consts.py} (88%)

diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py
index 01752c8bf..6bf6ed5b8 100644
--- a/checkbox-ng/plainbox/impl/ctrl.py
+++ b/checkbox-ng/plainbox/impl/ctrl.py
@@ -7,7 +7,6 @@
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
 # as published by the Free Software Foundation.
-
 #
 # Checkbox is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ -53,7 +52,6 @@ class to select the best method to execute a command of a particular job.  This
 
 from plainbox.abc import IJobResult
 from plainbox.abc import ISessionStateController
-from plainbox.configuration import Suspend
 from plainbox.i18n import gettext as _
 from plainbox.impl import get_plainbox_dir
 from plainbox.impl.depmgr import DependencyDuplicateError
@@ -72,6 +70,7 @@ class to select the best method to execute a command of a particular job.  This
 from plainbox.impl.unit.template import TemplateUnit
 from plainbox.impl.unit.unit import MissingParam
 from plainbox.impl.validation import Severity
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import morris
 from plainbox.vendor import extcmd
 
diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py
index c2084b5d1..ba4078eab 100644
--- a/checkbox-ng/plainbox/impl/test_ctrl.py
+++ b/checkbox-ng/plainbox/impl/test_ctrl.py
@@ -7,7 +7,6 @@
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
 # as published by the Free Software Foundation.
-
 #
 # Checkbox is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ -33,7 +32,7 @@
 from plainbox.abc import IJobResult
 from plainbox.abc import IProvider1
 from plainbox.abc import IProviderBackend1
-from plainbox.configuration import Suspend
+from plainbox.suspend_consts import Suspend
 from plainbox.impl.ctrl import CheckBoxSessionStateController
 from plainbox.impl.ctrl import SymLinkNest
 from plainbox.impl.ctrl import gen_rfc822_records_from_io_log
diff --git a/checkbox-ng/plainbox/configuration.py b/checkbox-ng/plainbox/suspend_consts.py
similarity index 88%
rename from checkbox-ng/plainbox/configuration.py
rename to checkbox-ng/plainbox/suspend_consts.py
index 2bb0dec1b..0424da00f 100644
--- a/checkbox-ng/plainbox/configuration.py
+++ b/checkbox-ng/plainbox/suspend_consts.py
@@ -7,7 +7,6 @@
 # Checkbox is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3,
 # as published by the Free Software Foundation.
-
 #
 # Checkbox is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ -18,8 +17,8 @@
 # along with Checkbox.  If not, see <http://www.gnu.org/licenses/>.
 
 """
-:mod:`plainbox.impl.configuration` -- Configuration variables
-=============================================================
+:mod:`plainbox.impl.suspend_consts` -- Suspend-related constants
+================================================================
 """
 
 

From 7708bdfdbe7b6062d70f94459e2741b569aa332a Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Tue, 5 Mar 2024 21:40:21 +0800
Subject: [PATCH 04/10] Use Suspend constants in plainbox.impl.session.state
 module

---
 checkbox-ng/plainbox/impl/session/state.py    | 23 ++++++++-----------
 .../plainbox/impl/session/test_state.py       | 18 ++++++++-------
 2 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/checkbox-ng/plainbox/impl/session/state.py b/checkbox-ng/plainbox/impl/session/state.py
index ef10d7c4b..46eb54fbc 100644
--- a/checkbox-ng/plainbox/impl/session/state.py
+++ b/checkbox-ng/plainbox/impl/session/state.py
@@ -41,6 +41,7 @@
 from plainbox.impl.unit.job import JobDefinition
 from plainbox.impl.unit.unit_with_id import UnitWithId
 from plainbox.impl.unit.testplan import TestPlanUnitSupport
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import morris
 
 
@@ -1091,24 +1092,21 @@ def _add_job_siblings_unit(self, new_job, recompute, via):
                         field_offset_map=new_job.field_offset_map),
                     recompute,
                     via)
-        if 'also-after-suspend' in new_job.get_flag_set():
+        if Suspend.AUTO_FLAG in new_job.get_flag_set():
             data = {
                 key: value for key, value in new_job._data.items()
                 if not key.endswith('siblings')
             }
-            data['flags'] = data['flags'].replace('also-after-suspend', '')
-            data['flags'] = data['flags'].replace(
-                'also-after-suspend-manual', '')
+            data['flags'] = data['flags'].replace(Suspend.AUTO_FLAG, '')
+            data['flags'] = data['flags'].replace(Suspend.MANUAL_FLAG, '')
             data['id'] = "after-suspend-{}".format(new_job.partial_id)
             data['_summary'] = "{} after suspend (S3)".format(
                 new_job.summary)
-            provider_id = "com.canonical.certification"
-            suspend_test_id = "suspend/suspend_advanced_auto"
             if new_job.depends:
                 data['depends'] += " {}".format(new_job.id)
             else:
                 data['depends'] = "{}".format(new_job.id)
-            data['depends'] += " {}::{}".format(provider_id, suspend_test_id)
+            data['depends'] += " {}".format(Suspend.AUTO_JOB_ID)
             self._add_job_unit(
                 JobDefinition(
                     data,
@@ -1119,24 +1117,21 @@ def _add_job_siblings_unit(self, new_job, recompute, via):
                     field_offset_map=new_job.field_offset_map),
                 recompute,
                 via)
-        if 'also-after-suspend-manual' in new_job.get_flag_set():
+        if Suspend.MANUAL_FLAG in new_job.get_flag_set():
             data = {
                 key: value for key, value in new_job._data.items()
                 if not key.endswith('siblings')
             }
-            data['flags'] = data['flags'].replace('also-after-suspend', '')
-            data['flags'] = data['flags'].replace(
-                'also-after-suspend-manual', '')
+            data['flags'] = data['flags'].replace(Suspend.AUTO_FLAG, '')
+            data['flags'] = data['flags'].replace(Suspend.MANUAL_FLAG, '')
             data['id'] = "after-suspend-manual-{}".format(new_job.partial_id)
             data['_summary'] = "{} after suspend (S3)".format(
                 new_job.summary)
-            provider_id = "com.canonical.certification"
-            suspend_test_id = "suspend/suspend_advanced"
             if new_job.depends:
                 data['depends'] += " {}".format(new_job.id)
             else:
                 data['depends'] = "{}".format(new_job.id)
-            data['depends'] += " {}::{}".format(provider_id, suspend_test_id)
+            data['depends'] += " {}".format(Suspend.MANUAL_JOB_ID)
             self._add_job_unit(
                 JobDefinition(
                     data,
diff --git a/checkbox-ng/plainbox/impl/session/test_state.py b/checkbox-ng/plainbox/impl/session/test_state.py
index 2adafd976..7f1dec63f 100644
--- a/checkbox-ng/plainbox/impl/session/test_state.py
+++ b/checkbox-ng/plainbox/impl/session/test_state.py
@@ -43,8 +43,8 @@
 from plainbox.impl.testing_utils import make_job
 from plainbox.impl.unit.job import JobDefinition
 from plainbox.impl.unit.category import CategoryUnit
-
 from plainbox.impl.unit.unit_with_id import UnitWithId
+from plainbox.suspend_consts import Suspend
 from plainbox.vendor import mock
 from plainbox.vendor.morris import SignalTestCase
 
@@ -309,7 +309,7 @@ def test_add_sibling_unit(self):
 
     def test_also_after_suspend_flag(self):
         # Define a job
-        job = make_job("A", summary="foo", flags="also-after-suspend")
+        job = make_job("A", summary="foo", flags=Suspend.AUTO_FLAG)
         # Define an empty session
         session = SessionState([])
         # Add the job to the session
@@ -319,12 +319,13 @@ def test_also_after_suspend_flag(self):
         self.assertIn(job, session.job_list)
         self.assertEqual(session.job_list[1].id, "after-suspend-A")
         self.assertEqual(session.job_list[1].summary, "foo after suspend (S3)")
+        expected_depends = "A {}".format(Suspend.AUTO_JOB_ID)
         self.assertEqual(
             session.job_list[1].depends,
-            ("A com.canonical.certification::suspend/suspend_advanced_auto"),
+            (expected_depends),
         )
         sibling = session.job_list[1]
-        self.assertNotIn("also-after-suspend", sibling.get_flag_set())
+        self.assertNotIn(Suspend.AUTO_FLAG, sibling.get_flag_set())
         # Both jobs got added to job state map
         self.assertIs(session.job_state_map[job.id].job, job)
         self.assertIs(session.job_state_map[sibling.id].job, sibling)
@@ -346,7 +347,7 @@ def test_also_after_suspend_flag(self):
 
     def test_also_after_suspend_manual_flag(self):
         # Define a job
-        job = make_job("A", summary="foo", flags="also-after-suspend-manual")
+        job = make_job("A", summary="foo", flags=Suspend.MANUAL_FLAG)
         # Define an empty session
         session = SessionState([])
         # Add the job to the session
@@ -356,12 +357,13 @@ def test_also_after_suspend_manual_flag(self):
         self.assertIn(job, session.job_list)
         self.assertEqual(session.job_list[1].id, "after-suspend-manual-A")
         self.assertEqual(session.job_list[1].summary, "foo after suspend (S3)")
+        expected_depends = "A {}".format(Suspend.MANUAL_JOB_ID)
         self.assertEqual(
             session.job_list[1].depends,
-            "A com.canonical.certification::suspend/suspend_advanced",
+            expected_depends,
         )
         sibling = session.job_list[1]
-        self.assertNotIn("also-after-suspend-manual", sibling.get_flag_set())
+        self.assertNotIn(Suspend.MANUAL_FLAG, sibling.get_flag_set())
         # Both jobs got added to job state map
         self.assertIs(session.job_state_map[job.id].job, job)
         self.assertIs(session.job_state_map[sibling.id].job, sibling)
@@ -1215,4 +1217,4 @@ def test_on_job_removed__via_state(self):
         sig1 = self.assertSignalFired(self.ctx.on_unit_removed, self.job)
         sig2 = self.assertSignalFired(self.ctx.state.on_unit_removed, self.job)
         sig3 = self.assertSignalFired(self.ctx.state.on_job_removed, self.job)
-        self.assertSignalOrdering(sig1, sig2, sig3)
\ No newline at end of file
+        self.assertSignalOrdering(sig1, sig2, sig3)

From 5d3b46bb4b63b40e9ff17191be1d787c36ea9763 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 17:26:58 +0800
Subject: [PATCH 05/10] Add helper function to prevent code duplication

_get_suspend_inhibitor_list() and _get_before_suspend_dependency_set()
work very similarly.

This patch introduces a new helper function,
_is_job_impacting_suspend(), which helps identifying jobs that should
run before the manual and automated suspend jobs.

Both _get_suspend_inhibitor_list() and
_get_before_suspend_dependency_set() make use of it to prevent code
duplication.
---
 checkbox-ng/plainbox/impl/ctrl.py | 78 +++++++++++++------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py
index 6bf6ed5b8..4999513ca 100644
--- a/checkbox-ng/plainbox/impl/ctrl.py
+++ b/checkbox-ng/plainbox/impl/ctrl.py
@@ -48,6 +48,7 @@ class to select the best method to execute a command of a particular job.  This
 import subprocess
 import sys
 import threading
+from functools import partial
 from subprocess import check_output, CalledProcessError, STDOUT
 
 from plainbox.abc import IJobResult
@@ -165,26 +166,30 @@ def _get_before_suspend_dependency_set(self, suspend_job_id, job_list):
         :returns:
             A set of job ids that need to be run before the suspend job
         """
-        suspend_deps = set()
-        expected_flag = ""
-        if suspend_job_id == Suspend.AUTO_JOB_ID:
-            expected_flag = Suspend.AUTO_FLAG
-        elif suspend_job_id == Suspend.MANUAL_JOB_ID:
-            expected_flag = Suspend.MANUAL_FLAG
-        if not expected_flag:
-            return suspend_deps
-        for dep_job in job_list:
-            if dep_job.flags and expected_flag in dep_job.flags:
-                suspend_deps.add(dep_job.id)
-            if dep_job.siblings:
-                for sibling_data in json.loads(dep_job.tr_siblings()):
-                    if (
-                        sibling_data.get("depends")
-                        and suspend_job_id in sibling_data["depends"]
-                    ):
-                        suspend_deps.add(dep_job.id)
+        p_suspend_job_id = partial(self._is_job_impacting_suspend, suspend_job_id)
+        suspend_deps_jobs = filter(p_suspend_job_id, job_list)
+        suspend_deps = set(job.id for job in suspend_deps_jobs)
         return suspend_deps
 
+    def _is_job_impacting_suspend(self, suspend_job_id, job):
+        """
+        Check if the ``suspend_job_id`` job needs to be run after a given
+        ``job``. This is the case if the ``job`` has a "also after suspend"
+        flag, or if it defines a sibling that has a dependency on the suspend
+        job.
+        """
+        expected_flag = {
+            Suspend.AUTO_JOB_ID: Suspend.AUTO_FLAG,
+            Suspend.MANUAL_JOB_ID: Suspend.MANUAL_FLAG,
+        }.get(suspend_job_id)
+        if job.flags and expected_flag in job.flags:
+            return True
+        if job.siblings:
+            for sibling_data in json.loads(job.tr_siblings()):
+                if suspend_job_id in sibling_data.get("depends", []):
+                    return True
+        return False
+
     def get_inhibitor_list(self, session_state, job):
         """
         Get a list of readiness inhibitors that inhibit a particular job.
@@ -315,33 +320,16 @@ def _get_suspend_inhibitor_list(self, session_state, suspend_job):
             List of JobReadinessInhibitor
         """
         suspend_inhibitors = []
-        expected_flag = ""
-        if suspend_job.id == Suspend.AUTO_JOB_ID:
-            expected_flag = Suspend.AUTO_FLAG
-        elif suspend_job.id == Suspend.MANUAL_JOB_ID:
-            expected_flag = Suspend.MANUAL_FLAG
-        if not expected_flag:
-            return suspend_inhibitors
-        for job_id, job_state in session_state.job_state_map.items():
-            if job_state.job.flags and expected_flag in job_state.job.flags:
-                if job_state.result.outcome == IJobResult.OUTCOME_NONE:
-                    inhibitor = JobReadinessInhibitor(
-                        cause=InhibitionCause.PENDING_DEP,
-                        related_job=job_state.job,
-                    )
-                    suspend_inhibitors.append(inhibitor)
-            if job_state.job.siblings:
-                for sibling_data in json.loads(job_state.job.tr_siblings()):
-                    if (
-                        sibling_data.get("depends")
-                        and suspend_job.id in sibling_data["depends"]
-                    ):
-                        if job_state.result.outcome == IJobResult.OUTCOME_NONE:
-                            inhibitor = JobReadinessInhibitor(
-                                cause=InhibitionCause.PENDING_DEP,
-                                related_job=job_state.job,
-                            )
-                        suspend_inhibitors.append(inhibitor)
+        job_list = [state.job for state in session_state.job_state_map.values()]
+        p_suspend_job_id = partial(self._is_job_impacting_suspend, suspend_job.id)
+        suspend_inhibitors_jobs = filter(p_suspend_job_id, job_list)
+        for job in suspend_inhibitors_jobs:
+            if session_state.job_state_map[job.id].result.outcome == IJobResult.OUTCOME_NONE:
+                inhibitor = JobReadinessInhibitor(
+                    cause=InhibitionCause.PENDING_DEP,
+                    related_job=job,
+                )
+                suspend_inhibitors.append(inhibitor)
         return suspend_inhibitors
 
     def observe_result(self, session_state, job, result, fake_resources=False):

From f6919e704260f080be27e60595d0d33b2a1c24f0 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 17:27:46 +0800
Subject: [PATCH 06/10] Refactor CheckboxController tests

With the new helper function, less tests are required.
---
 checkbox-ng/plainbox/impl/test_ctrl.py | 193 ++++---------------------
 1 file changed, 26 insertions(+), 167 deletions(-)

diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py
index ba4078eab..f2712229c 100644
--- a/checkbox-ng/plainbox/impl/test_ctrl.py
+++ b/checkbox-ng/plainbox/impl/test_ctrl.py
@@ -117,53 +117,6 @@ def test_get_dependency_set(self):
             {("ordering", "j7")}
         )
 
-    def test_get_before_suspend_dependency_set__not_suspend_job_id(self):
-        self.assertEqual(
-            self.ctrl._get_before_suspend_dependency_set("not-suspend", []),
-            set()
-        )
-
-    def test_get_before_suspend_dependency_set__manual_suspend(self):
-        job1 = JobDefinition({
-            "id": "job1",
-            "flags": Suspend.MANUAL_FLAG
-        })
-        job2 = JobDefinition({
-            "id": "job2",
-        })
-        suspend_job = JobDefinition({
-            "id": Suspend.MANUAL_JOB_ID
-        })
-        self.assertEqual(
-            self.ctrl._get_before_suspend_dependency_set(
-                suspend_job.id,
-                [job1, job2]
-            ),
-            {"job1"}
-        )
-
-    def test_get_before_suspend_dependency_set__sibling(self):
-        job1 = JobDefinition({
-            "id": "job1",
-            "siblings": json.dumps([{
-                "id": "sibling-j1",
-                "depends": Suspend.AUTO_JOB_ID,
-            }])
-        })
-        job2 = JobDefinition({
-            "id": "job2",
-        })
-        suspend_job = JobDefinition({
-            "id": Suspend.AUTO_JOB_ID
-        })
-        self.assertEqual(
-            self.ctrl._get_before_suspend_dependency_set(
-                suspend_job.id,
-                [job1, job2]
-            ),
-            {"job1"}
-        )
-
     def test_get_inhibitor_list_PENDING_RESOURCE(self):
         # verify that jobs that require a resource that hasn't been
         # invoked yet produce the PENDING_RESOURCE inhibitor
@@ -371,139 +324,45 @@ def test_get_inhibitor_list__suspend_job(self):
             self.ctrl.get_inhibitor_list(session_state, suspend_job),
             [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
 
-    def test_get_suspend_inhibitor_list__not_suspend_job_id(self):
-        j1 = JobDefinition({
-            "id": "not-a-suspend-job"
-        })
-        session_state = mock.MagicMock(spec=SessionState)
-        self.assertEqual(
-            self.ctrl._get_suspend_inhibitor_list(session_state, j1), [])
-
-    def test_get_suspend_inhibitor_list__auto_suspend(self):
-        j1 = JobDefinition({
-            "id": "j1",
-            "flags": Suspend.AUTO_FLAG,
-        })
-        j2 = JobDefinition({
-            "id": "j2",
+    def test_is_job_impacting_suspend__wrong_suspend_job(self):
+        job = JobDefinition({
+            "id": "job",
         })
-        suspend_job = JobDefinition({
-            "id": Suspend.AUTO_JOB_ID
-        })
-        session_state = mock.MagicMock(spec=SessionState)
-        session_state.job_state_map = {
-            "j1": mock.Mock(spec_set=JobState),
-            "j2": mock.Mock(spec_set=JobState),
-            Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState),
-        }
-        jsm_j1 = session_state.job_state_map["j1"]
-        jsm_j1.job = j1
-        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_j2 = session_state.job_state_map["j2"]
-        jsm_j2.job = j2
-        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
-        jsm_suspend.job = suspend_job
-        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
         self.assertEqual(
-            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
-            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+            self.ctrl._is_job_impacting_suspend("wrong-suspend-job-id", job),
+            False
+        )
 
-    def test_get_suspend_inhibitor_list__manual_suspend(self):
-        j1 = JobDefinition({
-            "id": "j1",
-            "flags": Suspend.MANUAL_FLAG,
-        })
-        j2 = JobDefinition({
-            "id": "j2",
-        })
-        suspend_job = JobDefinition({
-            "id": Suspend.MANUAL_JOB_ID
+    def test_is_job_impacting_suspend__flag(self):
+        job = JobDefinition({
+            "id": "job",
+            "flags": "also-after-suspend",
         })
-        session_state = mock.MagicMock(spec=SessionState)
-        session_state.job_state_map = {
-            "j1": mock.Mock(spec_set=JobState),
-            "j2": mock.Mock(spec_set=JobState),
-            Suspend.MANUAL_JOB_ID: mock.Mock(spec_set=JobState),
-        }
-        jsm_j1 = session_state.job_state_map["j1"]
-        jsm_j1.job = j1
-        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_j2 = session_state.job_state_map["j2"]
-        jsm_j2.job = j2
-        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_suspend = session_state.job_state_map[Suspend.MANUAL_JOB_ID]
-        jsm_suspend.job = suspend_job
-        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
         self.assertEqual(
-            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
-            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
-
-    def test_get_suspend_inhibitor_list__auto_sibling(self):
-        j1 = JobDefinition({
-            "id": "j1",
-            "siblings": json.dumps([{
-                "id": "sibling-j1",
-                "depends": Suspend.AUTO_JOB_ID,
-            }])
-        })
-        j2 = JobDefinition({
-            "id": "j2",
-        })
-        suspend_job = JobDefinition({
-            "id": Suspend.AUTO_JOB_ID
-        })
-        session_state = mock.MagicMock(spec=SessionState)
-        session_state.job_state_map = {
-            "j1": mock.Mock(spec_set=JobState),
-            "j2": mock.Mock(spec_set=JobState),
-            Suspend.AUTO_JOB_ID: mock.Mock(spec_set=JobState),
-        }
-        jsm_j1 = session_state.job_state_map["j1"]
-        jsm_j1.job = j1
-        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_j2 = session_state.job_state_map["j2"]
-        jsm_j2.job = j2
-        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
-        jsm_suspend.job = suspend_job
-        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+            self.ctrl._is_job_impacting_suspend(Suspend.AUTO_JOB_ID, job),
+            True
+        )
         self.assertEqual(
-            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
-            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+            self.ctrl._is_job_impacting_suspend(Suspend.MANUAL_JOB_ID, job),
+            False
+        )
 
-    def test_get_suspend_inhibitor_list__manual_sibling(self):
-        j1 = JobDefinition({
-            "id": "j1",
+    def test_is_job_impacting_suspend__siblings(self):
+        job = JobDefinition({
+            "id": "job",
             "siblings": json.dumps([{
                 "id": "sibling-j1",
                 "depends": Suspend.MANUAL_JOB_ID,
             }])
         })
-        j2 = JobDefinition({
-            "id": "j2",
-        })
-        suspend_job = JobDefinition({
-            "id": Suspend.MANUAL_JOB_ID
-        })
-        session_state = mock.MagicMock(spec=SessionState)
-        session_state.job_state_map = {
-            "j1": mock.Mock(spec_set=JobState),
-            "j2": mock.Mock(spec_set=JobState),
-            Suspend.MANUAL_JOB_ID: mock.Mock(spec_set=JobState),
-        }
-        jsm_j1 = session_state.job_state_map["j1"]
-        jsm_j1.job = j1
-        jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_j2 = session_state.job_state_map["j2"]
-        jsm_j2.job = j2
-        jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
-        jsm_suspend = session_state.job_state_map[Suspend.MANUAL_JOB_ID]
-        jsm_suspend.job = suspend_job
-        jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
         self.assertEqual(
-            self.ctrl._get_suspend_inhibitor_list(session_state, suspend_job),
-            [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])
+            self.ctrl._is_job_impacting_suspend(Suspend.AUTO_JOB_ID, job),
+            False
+        )
+        self.assertEqual(
+            self.ctrl._is_job_impacting_suspend(Suspend.MANUAL_JOB_ID, job),
+            True
+        )
 
     def test_observe_result__normal(self):
         job = mock.Mock(spec=JobDefinition)

From 9da78983cca074b97d0ed1ba6c4e3c66a6f4851a Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 20:46:13 +0800
Subject: [PATCH 07/10] Ensure suspend job only depends on jobs in the run list

If the filtering is done on the whole job_list, the suspend job will
depend on jobs that are not required (undesired) for the current session
run.

As a result, the suspend job is skipped with reasons like:

    required dependency 'com.canonical.certification::audio/
    playback_auto' did not run yet

and all of the after-suspend jobs are skipped!

the job_list therefore needs to be narrowed down to remove the
"undesired" jobs and only keep the ones that are actually going to run.
---
 checkbox-ng/plainbox/impl/ctrl.py | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py
index 4999513ca..180d3362e 100644
--- a/checkbox-ng/plainbox/impl/ctrl.py
+++ b/checkbox-ng/plainbox/impl/ctrl.py
@@ -320,9 +320,19 @@ def _get_suspend_inhibitor_list(self, session_state, suspend_job):
             List of JobReadinessInhibitor
         """
         suspend_inhibitors = []
-        job_list = [state.job for state in session_state.job_state_map.values()]
-        p_suspend_job_id = partial(self._is_job_impacting_suspend, suspend_job.id)
-        suspend_inhibitors_jobs = filter(p_suspend_job_id, job_list)
+        undesired_inhibitor = JobReadinessInhibitor(
+            cause=InhibitionCause.UNDESIRED
+        )
+        # We are only interested in jobs that are actually going to run
+        run_list = [
+            state.job
+            for state in session_state.job_state_map.values()
+            if undesired_inhibitor not in state.readiness_inhibitor_list
+        ]
+        p_suspend_job_id = partial(
+            self._is_job_impacting_suspend, suspend_job.id
+        )
+        suspend_inhibitors_jobs = filter(p_suspend_job_id, run_list)
         for job in suspend_inhibitors_jobs:
             if session_state.job_state_map[job.id].result.outcome == IJobResult.OUTCOME_NONE:
                 inhibitor = JobReadinessInhibitor(

From a28e64e351e80372666e7af57dcf77bd5f88f030 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 20:46:21 +0800
Subject: [PATCH 08/10] Formatting

---
 checkbox-ng/plainbox/impl/ctrl.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/checkbox-ng/plainbox/impl/ctrl.py b/checkbox-ng/plainbox/impl/ctrl.py
index 180d3362e..b02dc7e7d 100644
--- a/checkbox-ng/plainbox/impl/ctrl.py
+++ b/checkbox-ng/plainbox/impl/ctrl.py
@@ -166,7 +166,9 @@ def _get_before_suspend_dependency_set(self, suspend_job_id, job_list):
         :returns:
             A set of job ids that need to be run before the suspend job
         """
-        p_suspend_job_id = partial(self._is_job_impacting_suspend, suspend_job_id)
+        p_suspend_job_id = partial(
+            self._is_job_impacting_suspend, suspend_job_id
+        )
         suspend_deps_jobs = filter(p_suspend_job_id, job_list)
         suspend_deps = set(job.id for job in suspend_deps_jobs)
         return suspend_deps
@@ -334,7 +336,10 @@ def _get_suspend_inhibitor_list(self, session_state, suspend_job):
         )
         suspend_inhibitors_jobs = filter(p_suspend_job_id, run_list)
         for job in suspend_inhibitors_jobs:
-            if session_state.job_state_map[job.id].result.outcome == IJobResult.OUTCOME_NONE:
+            if (
+                session_state.job_state_map[job.id].result.outcome
+                == IJobResult.OUTCOME_NONE
+            ):
                 inhibitor = JobReadinessInhibitor(
                     cause=InhibitionCause.PENDING_DEP,
                     related_job=job,

From 264138726deade981b399a952c2d8d69d5b86ed9 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 20:47:03 +0800
Subject: [PATCH 09/10] Mention guarantee of job running before suspend in Job
 Unit documentation

---
 docs/reference/units/job.rst | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/docs/reference/units/job.rst b/docs/reference/units/job.rst
index e57687350..f8a0e7fcb 100644
--- a/docs/reference/units/job.rst
+++ b/docs/reference/units/job.rst
@@ -315,11 +315,17 @@ Following fields may be used by the job unit:
 
     .. _also-after-suspend flag:
 
-    ``also-after-suspend``: See :ref:`Job siblings field` below.
+    ``also-after-suspend``:
+        Ensure the test will be run before **and** after suspend by creating
+        a :ref:`sibling<Job siblings field>` that will depend on the automated
+        suspend job. The current job is guaranteed to run before suspend.
 
     .. _also-after-suspend-manual flag:
 
-    ``also-after-suspend-manual``: See :ref:`Job siblings field` below.
+    ``also-after-suspend-manual``:
+        Ensure the test will be run before **and** after suspend by creating
+        a :ref:`sibling<Job siblings field>` that will depend on the manual
+        suspend job. The current job is guaranteed to run before suspend.
 
     Additional flags may be present in job definition; they are ignored.
 
@@ -399,16 +405,20 @@ Following fields may be used by the job unit:
           com.canonical.certification::suspend/suspend_advanced
           foo
 
-.. warning::
-    The curly braces used in this field have to be escaped when used in a
-    template job (python format, Jinja2 templates do not have this issue).
-    The syntax for templates is::
+    .. note::
+        If the sibling definition depends on one of the suspend jobs, Checkbox
+        will make sure the original job runs **before** the suspend job.
 
-            _siblings: [
-                {{ "id": "bar-after-suspend_{interface}",
-                  "_summary": "bar after suspend",
-                  "depends": "suspend/advanced"}}
-                ]
+    .. warning::
+        The curly braces used in this field have to be escaped when used in a
+        template job (python format, Jinja2 templates do not have this issue).
+        The syntax for templates is::
+
+                _siblings: [
+                    {{ "id": "bar-after-suspend_{interface}",
+                      "_summary": "bar after suspend",
+                      "depends": "suspend/advanced"}}
+                    ]
 
 .. _Job imports field:
 

From f33a07b3ecf45c2d820f9d1150383c800d9fa2d0 Mon Sep 17 00:00:00 2001
From: Pierre Equoy <pierre.equoy@canonical.com>
Date: Wed, 6 Mar 2024 20:53:26 +0800
Subject: [PATCH 10/10] Fix unit test

---
 checkbox-ng/plainbox/impl/test_ctrl.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/checkbox-ng/plainbox/impl/test_ctrl.py b/checkbox-ng/plainbox/impl/test_ctrl.py
index f2712229c..c032dd5e5 100644
--- a/checkbox-ng/plainbox/impl/test_ctrl.py
+++ b/checkbox-ng/plainbox/impl/test_ctrl.py
@@ -314,12 +314,15 @@ def test_get_inhibitor_list__suspend_job(self):
         jsm_j1 = session_state.job_state_map["j1"]
         jsm_j1.job = j1
         jsm_j1.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j1.readiness_inhibitor_list = []
         jsm_j2 = session_state.job_state_map["j2"]
         jsm_j2.job = j2
         jsm_j2.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_j2.readiness_inhibitor_list = []
         jsm_suspend = session_state.job_state_map[Suspend.AUTO_JOB_ID]
         jsm_suspend.job = suspend_job
         jsm_suspend.result.outcome = IJobResult.OUTCOME_NONE
+        jsm_suspend.readiness_inhibitor_list = []
         self.assertEqual(
             self.ctrl.get_inhibitor_list(session_state, suspend_job),
             [JobReadinessInhibitor(InhibitionCause.PENDING_DEP, j1, None)])