Merge pull request #3 from SmithChart/cfi/review-feedback

Quality improvements and best current practices

conftest.py

@@ -1,3 +1,4 @@
+import json
 import traceback
 
 import pytest
@@ -25,35 +26,70 @@ def shell(strategy):
     return strategy.shell
 
 
-@pytest.fixture(scope="function")
-def online(strategy):
-    try:
-        strategy.transition("network")
-    except Exception as e:
-        traceback.print_exc()
-        pytest.exit(f"Transition into online state failed: {e}", returncode=3)
+@pytest.fixture
+def default_bootstate(strategy):
+    """Set default state values as setup/teardown"""
+    strategy.transition("barebox")
+    strategy.barebox.run_check("bootchooser -a default -p default")
 
+    yield
 
-@pytest.fixture(scope="function")
-def system0_shell(strategy):
-    try:
-        strategy.transition("system0")
-    except Exception as e:
-        traceback.print_exc()
-        pytest.exit(f"Transition into system0 shell failed: {e}", returncode=3)
+    strategy.transition("barebox")
+    strategy.barebox.run_check("bootchooser -a default -p default")
 
-    return strategy.shell
 
+@pytest.fixture
+def booted_slot(shell):
+    """Returns booted slot."""
 
-@pytest.fixture(scope="function")
-def system1_shell(strategy):
-    try:
-        strategy.transition("system1")
-    except Exception as e:
-        traceback.print_exc()
-        pytest.exit(f"Transition into system1 shell failed: {e}", returncode=3)
+    def _booted_slot():
+        [stdout] = shell.run_check("rauc status --output-format=json", timeout=60)
+        rauc_status = json.loads(stdout)
 
-    return strategy.shell
+        assert "booted" in rauc_status, 'No "booted" key in rauc status json found'
+
+        return rauc_status["booted"]
+
+    yield _booted_slot
+
+
+@pytest.fixture
+def set_bootstate_in_bootloader(strategy, default_bootstate):
+    """Sets the given bootchooser parameters."""
+
+    def _set_bootstate(system0_prio, system0_attempts, system1_prio, system1_attempts):
+        strategy.transition("barebox")
+        barebox = strategy.barebox
+
+        barebox.run_check(f"state.bootstate.system0.priority={system0_prio}")
+        barebox.run_check(f"state.bootstate.system0.remaining_attempts={system0_attempts}")
+
+        barebox.run_check(f"state.bootstate.system1.priority={system1_prio}")
+        barebox.run_check(f"state.bootstate.system1.remaining_attempts={system1_attempts}")
+
+        barebox.run_check("state -s")
+
+    yield _set_bootstate
+
+
+@pytest.fixture
+def rauc_bundle(target, strategy, env, shell):
+    """Makes the RAUC bundle target-accessible at the returned location."""
+    bundle = env.config.get_image_path("rauc_bundle")
+
+    def _rauc_bundle():
+        target.activate(strategy.httpprovider)
+        return strategy.httpprovider.stage(bundle)
+
+    yield _rauc_bundle
+
+
+@pytest.fixture
+def eet(strategy):
+    eet = strategy.eet
+    yield eet
+    if eet:
+        eet.link("")
 
 
 def pytest_configure(config):

lxatac-eet.py

@@ -1,10 +1,9 @@
 import os.path
 
 import attr
-from labgrid.driver.common import Driver
-from labgrid.factory import target_factory
+from labgrid import step, target_factory
+from labgrid.driver import Driver
 from labgrid.resource.common import NetworkResource, Resource
-from labgrid.step import step
 from labgrid.util.agentwrapper import AgentWrapper
 
 

lxatac-ptx.yaml

@@ -52,7 +52,7 @@ targets:
         - /ptx/work/WORK_XUNGI
         - /ptx/work/WORK_JALLU
         - /ptx/work/WORK_MOHCH
-      usbpath: 1-1.1:1.0
+      usb_storage: /dev/disk/by-path/platform-5800d000.usb-usbv2-0:1.1:1.0-scsi-0:0:0:0
     features:
       - ptx-flavor
       - ethmux

lxatac-vanilla-eet.yaml

@@ -41,7 +41,7 @@ targets:
           ethmux: "ethmux"
     options:
       ptx-works-available: []
-      usbpath: 1-1.1:1.0
+      usb_storage: /dev/disk/by-path/platform-5800d000.usb-usbv2-0:1.1:1.0-scsi-0:0:0:0
     features:
       - eet
       - ethmux

lxatac-vanilla.yaml

@@ -33,7 +33,6 @@ targets:
           dfu_mode: "dfu_mode"
     options:
       ptx-works-available: []
-      usbpath: 1-1.1:1.0  # Connect a USB mass storage to USB port 1
 
 images:
   tfa: ../images/lxatac/tf-a-stm32mp157c-lxa-tac.stm32

lxatacstrategy.py

@@ -1,27 +1,17 @@
 import enum
-import json
 
 import attr
+from labgrid import step, target_factory
 from labgrid.driver import ExecutionError
-from labgrid.factory import target_factory
-from labgrid.step import step
 from labgrid.strategy import Strategy, StrategyError
 
 # Possible state transitions:
 #
-#            +---------------------------------------------------------+
-#            v                                                         |
-#            +--------+------------+----------+  +---------------------+
-#            v        v            v          |  v                     |
-# unknown -> off -1-> bootstrap -> barebox -> shell -> network --------+
-#                                                      | |             |
-#                                                      2 +-> system0 --+
-#                                                      v               |
-#                                                      rauc_installed -+
-#                                                      |               |
-#                                                      +---> system1 --+
+#            +--------+------------+----------+
+#            v        v            v          |
+# unknown -> off -1-> bootstrap -> barebox -> shell
+#
 # 1) Via bootstrap() but only once
-# 2) Via rauc_install() but only once
 
 
 class Status(enum.Enum):
@@ -30,10 +20,6 @@ class Status(enum.Enum):
     bootstrap = 2
     barebox = 3
     shell = 4
-    network = 5
-    system0 = 6
-    rauc_installed = 7
-    system1 = 8
 
 
 @target_factory.reg_driver
@@ -59,10 +45,6 @@ class LXATACStrategy(Strategy):
 
     status = attr.ib(default=Status.unknown)
     mmc_bootstrapped = attr.ib(default=False)
-    rauc_installed = attr.ib(default=False)
-
-    def __attrs_post_init__(self):
-        super().__attrs_post_init__()
 
     @property
     def target_hostname(self):
@@ -107,49 +89,20 @@ def bootstrap(self):
 
         self.mmc_bootstrapped = True
 
-    def rauc_install(self):
-        self.transition(Status.system0)
-
-        self.target.activate(self.httpprovider)
-
-        bundle = self.target.env.config.get_image_path("rauc_bundle")
-        bundle_url = self.httpprovider.stage(bundle)
+    def wait_online(self):
+        self.shell.poll_until_success("ping -c1 _gateway", timeout=60.0)
 
-        self.shell.run_check(f'curl -I "{bundle_url}"')
-
-        self.shell.run_check("rauc-enable-cert devel.cert.pem")
+        # Also make sure we have accurate time, so that TLS works.
+        self.shell.run_check("chronyc waitsync", timeout=120.0)
 
+    def wait_system_ready(self):
         try:
-            self.shell.run_check(f'rauc install "{bundle_url}"', timeout=600)
+            self.shell.run("systemctl is-system-running --wait", timeout=90)
         except ExecutionError:
-            self.shell.run("journalctl --boot --unit rauc.service")
+            # gather information about failed units
+            self.shell.run("systemctl list-units --failed --no-legend --plain --no-pager")
             raise
 
-        self.target.deactivate(self.httpprovider)
-
-        self.rauc_installed = True
-
-    def set_bootstate(self, system0_prio, system0_attempts, system1_prio, system1_attempts):
-        self.transition(Status.barebox)
-
-        self.barebox.run_check(f"state.bootstate.system0.priority={system0_prio}")
-        self.barebox.run_check(f"state.bootstate.system0.remaining_attempts={system0_attempts}")
-
-        self.barebox.run_check(f"state.bootstate.system1.priority={system1_prio}")
-        self.barebox.run_check(f"state.bootstate.system1.remaining_attempts={system1_attempts}")
-
-        self.barebox.run_check("state -s")
-
-    def get_booted_slot(self):
-        self.transition(Status.network)
-
-        stdout = self.shell.run_check("rauc status --output-format=json", timeout=60)
-        rauc_status = json.loads(stdout[0])
-
-        assert "booted" in rauc_status, 'No "booted" key in rauc status json found'
-
-        return rauc_status["booted"]
-
     @step(args=["status"])
     def transition(self, status, *, step):
         if not isinstance(status, Status):
@@ -163,7 +116,7 @@ def transition(self, status, *, step):
             return
 
         elif status == Status.off:
-            if self.status in [Status.shell, Status.network, Status.system0, Status.system1]:
+            if self.status == Status.shell:
                 # Cleanly shut down the labgrid exporter to help the
                 # coordinator clean up stale resources.
                 self.shell.run("systemctl stop labgrid-exporter", timeout=90)
@@ -199,52 +152,15 @@ def transition(self, status, *, step):
             self.barebox.run_check("global linux.bootargs.loglevel=loglevel=6")
 
         elif status == Status.shell:
-            # No need to reboot just because we checked for network connectivity
-            # or the slot we are running on.
-            if self.status not in [Status.network, Status.system0, Status.system1]:
-                # transition to barebox
-                self.transition(Status.barebox)
-
-                self.barebox.boot("")
-                self.barebox.await_boot()
-
-                self.target.activate(self.shell)
-
-            self.shell.run("systemctl is-system-running --wait", timeout=90)
-
-        elif status == Status.network:
-            # No need to reboot just because we checked which slot we are running on.
-            if self.status not in [Status.system0, Status.system1]:
-                self.transition(Status.shell)
-
-            self.shell.poll_until_success("ping -c1 _gateway", timeout=60.0)
-
-            # Also make sure we have accurate time, so that TLS works.
-            self.shell.run_check("chronyc waitsync", timeout=120.0)
+            # transition to barebox
+            self.transition(Status.barebox)
 
-        elif status == Status.system0:
-            self.transition(Status.network)
+            self.barebox.boot("")
+            self.barebox.await_boot()
 
-            if self.get_booted_slot() != "system0":
-                self.set_bootstate(20, 1, 10, 1)
-                self.transition(Status.network)
-
-                assert self.get_booted_slot() == "system0"
-
-        elif status == Status.rauc_installed:
-            self.transition(Status.network)
-
-            if not self.rauc_installed:
-                self.rauc_install()
-
-        elif status == Status.system1:
-            self.transition(Status.rauc_installed)
-
-            if self.get_booted_slot() != "system1":
-                self.set_bootstate(10, 1, 20, 1)
-                self.transition(Status.network)
-
-                assert self.get_booted_slot() == "system1"
+            self.target.activate(self.shell)
+            self.wait_system_ready()
+            self.wait_online()
 
         else:
             raise StrategyError(f"no transition found from {self.status} to {status}")

tests/helper.py

@@ -0,0 +1,38 @@
+import re
+
+import labgrid.protocol
+
+
+class SystemdRun:
+    """
+    Wrapper around `systemd-run` to run a command in the background.
+    This wrapper is intended to be used as context manager:
+
+    > with SystemdRun(command="sleep 30", shell=shell):
+    >     pass
+
+    The output of the command is not collected.
+    Neither is the exit code.
+
+    This is a workaround until something like https://github.com/labgrid-project/labgrid/pull/835 is merged.
+    """
+
+    _re_run = re.compile(r"^Running as unit: (run-\w+\.service);")
+
+    def __init__(self, command: str, shell: labgrid.protocol.ConsoleProtocol):
+        """
+        Run `command` in a transient unit using `systemd-run`.
+        """
+        self._command = command
+        self._shell = shell
+        self._unit = None
+
+    def __enter__(self) -> None:
+        stdout = self._shell.run_check(f"systemd-run {self._command}")
+        match = SystemdRun._re_run.match("".join(stdout))
+        if not match:
+            raise ValueError(f"systemd-run returned not parseable output: {stdout}")
+        self._unit = match[1]
+
+    def __exit__(self, _type, _value, _traceback):
+        self._shell.run(f"systemctl stop {self._unit}")