Added try...finally blocks to _run_test_phase()

Turned gather_tasks() into tasks() iterator
Moved child function docstrings for tasks() into the function body
Moved the extra discover phase to full.sh test to save time

Author: tcornell-bus

tests/execute/upgrade/data/plan.fmf

@@ -14,7 +14,11 @@ execute:
 /no-path:
     summary: Basic upgrade test with no upgrade path
 /path:
-    summary: Basic upgrade test with upgrade path
+    summary: Basic upgrade test with multiple discover phases
+    discover+:
+        - name: extra-phase
+          how: fmf
+          filter: tag:extra
     execute+:
         upgrade-path: $@{upgrade-path}
 /skip-tests:
@@ -27,11 +31,3 @@ execute:
         execute+:
             upgrade-path: $@{upgrade-path}
             skip-tests-after: true
-/multi-phase-discover:
-    summary: Upgrade test with multiple discover phases
-    discover+:
-        - name: extra-phase
-          how: fmf
-          filter: tag:extra
-    execute+:
-        upgrade-path: $@{upgrade-path}

tests/execute/upgrade/full.sh

@@ -16,12 +16,22 @@ rlJournalStart
             run --id $run --scratch --rm -vvv --before finish \
             plan --name /plan/path" 0 "Run the upgrade test"
 
-        # 1 test before + 3 upgrade tasks + 1 test after
-        rlAssertGrep "5 tests passed" $rlRun_LOG
+        # 2 test before + 3 upgrade tasks + 2 test after
+        rlAssertGrep "7 tests passed" $rlRun_LOG
+
+        # There should only be 1 execute task
+        rlAssertGrep "execute task #1:" $rlRun_LOG
+        rlAssertNotGrep "execute task #2:" $rlRun_LOG
+
+        rlAssertEquals "system upgrade should happen only once" "$(grep -o 'upgrade: perform the system upgrade' $rlRun_LOG | wc -l)" 1
 
         # Check that the IN_PLACE_UPGRADE variable was set
-        rlAssertGrep "IN_PLACE_UPGRADE=old" "$run/plan/path/execute/data/guest/default-0/old/test-1/output.txt"
-        rlAssertGrep "IN_PLACE_UPGRADE=new" "$run/plan/path/execute/data/guest/default-0/new/test-1/output.txt"
+        rlAssertGrep "IN_PLACE_UPGRADE=old" "$run/plan/path/execute/data/guest/default-0/old/default-0/test-1/output.txt"
+        rlAssertGrep "IN_PLACE_UPGRADE=new" "$run/plan/path/execute/data/guest/default-0/new/default-0/test-1/output.txt"
+
+        # Check that the extra discover phase test was run before and after the upgrade
+        rlAssertExists "$run/plan/path/execute/data/guest/default-0/old/extra-phase/extra-test-2/output.txt"
+        rlAssertExists "$run/plan/path/execute/data/guest/default-0/new/extra-phase/extra-test-2/output.txt"
     rlPhaseEnd
 
     rlPhaseStartCleanup

tests/execute/upgrade/main.fmf

@@ -2,7 +2,7 @@ duration: 10m
 tier: 4
 
 /full:
-    summary: Perform a full distro upgrade
+    summary: Perform a full distro upgrade with multiple discover phases
     test: ./full.sh
     # dnf upgrade is run, this may take quite long
     duration: 1h
@@ -34,12 +34,3 @@ tier: 4
     tag+:
       - provision-only
       - provision-virtual
-
-/multi-phase-discover:
-    summary: Perform a full distro upgrade with multiple discover phases
-    test: ./multi-phase-discover.sh
-    # dnf upgrade is run, this may take quite long
-    duration: 1h
-    tag+:
-      - provision-only
-      - provision-virtual

tests/execute/upgrade/multi-phase-discover.sh

@@ -4,7 +4,7 @@
 import signal as _signal
 import subprocess
 import threading
-from collections.abc import Sequence
+from collections.abc import Iterator, Sequence
 from typing import TYPE_CHECKING, Any, Optional, TypeVar, Union, cast
 
 import click
@@ -978,16 +978,15 @@ def timeout_hint(self, invocation: TestInvocation) -> None:
         )
 
     @abc.abstractmethod
-    def gather_tasks(
+    def tasks(
         self,
-    ) -> list[tuple['ExecutePlugin[ExecuteStepData]', list['Guest']]]:
+    ) -> Iterator[tuple['ExecutePlugin[ExecuteStepData]', list['Guest']]]:
         """
-        Return tasks to be enqueued for execution.
+        Iterate over tasks to be enqueued for execution.
 
-        This method returns a list of tuples, each containing a plugin phase
-        copy and the list of guests it should run on.
+        :yields: tuple of two items, a plugin phase copy
+            and the list of guests it should run on.
         """
-
         raise NotImplementedError
 
     @abc.abstractmethod
@@ -1218,7 +1217,7 @@ def go(self, force: bool = False) -> None:
                 queue.enqueue_action(phase=phase)
 
             elif isinstance(phase, ExecutePlugin):
-                for phase_copy, guests in phase.gather_tasks():
+                for phase_copy, guests in phase.tasks():
                     queue.enqueue_plugin(phase=phase_copy, guests=guests)
 
         failed_tasks: list[Union[ActionTask, PluginTask[ExecuteStepData, None]]] = []

tmt/steps/execute/__init__.py

@@ -1,4 +1,5 @@
 import copy
+from collections.abc import Iterator
 from typing import Any, Optional, cast
 
 import tmt.base.core
@@ -272,39 +273,31 @@ def __init__(self, **kwargs: Any):
         super().__init__(**kwargs)
         self._previous_progress_message = ""
 
-    def gather_tasks(
+    def tasks(
         self,
-    ) -> list[tuple['ExecutePlugin[ExecuteStepData]', list['Guest']]]:
-        """
-        Return tasks to be enqueued for execution.
-
-        A single execute plugin is expected to process (potentially)
-        multiple discover phases. There must be a way to tell the execute
-        plugin which discover phase to focus on. Unfortunately, the
-        current way is the execute plugin checking its `discover`
-        attribute. For each discover phase, we need a copy of the execute
-        plugin, so we could point it to that discover phase rather than
-        let it "see" all tests, or test in different discover phase.
-        """
-        tasks: list[tuple[ExecutePlugin[ExecuteStepData], list[Guest]]] = []
+    ) -> Iterator[tuple['ExecutePlugin[ExecuteStepData]', list['Guest']]]:
+        # A single execute plugin is expected to process (potentially)
+        # multiple discover phases. There must be a way to tell the execute
+        # plugin which discover phase to focus on. Unfortunately, the
+        # current way is the execute plugin checking its `discover`
+        # attribute. For each discover phase, we need a copy of the execute
+        # plugin, so we could point it to that discover phase rather than
+        # let it "see" all tests, or test in different discover phase.
         for discover in self.step.plan.discover.phases(classes=(DiscoverPlugin,)):
             if not discover.enabled_by_when:
                 continue
 
             phase_copy = cast(ExecutePlugin[ExecuteStepData], copy.copy(self))
             phase_copy.discover_phase = discover.name
 
-            tasks.append(
-                (
-                    phase_copy,
-                    [
-                        guest
-                        for guest in self.step.plan.provision.ready_guests
-                        if discover.enabled_on_guest(guest)
-                    ],
-                )
+            yield (
+                phase_copy,
+                [
+                    guest
+                    for guest in self.step.plan.provision.ready_guests
+                    if discover.enabled_on_guest(guest)
+                ],
             )
-        return tasks
 
     def _test_output_logger(
         self,

tmt/steps/execute/internal.py

@@ -1,4 +1,5 @@
 import copy
+from collections.abc import Iterator
 from typing import Any, Optional, Union, cast
 
 import fmf.utils
@@ -228,29 +229,20 @@ def discover(self) -> Union[Discover, DiscoverFmf]:
     def discover(self, plugin: Optional[DiscoverPlugin[DiscoverStepData]]) -> None:
         self._discover = plugin
 
-    def gather_tasks(
+    def tasks(
         self,
-    ) -> list[tuple['ExecutePlugin[ExecuteStepData]', list['tmt.guest.Guest']]]:
-        """
-        Return tasks to be enqueued for execution.
-
-        upgrade plugin is expected to (potentially) execute multiple
-        discover phases on old, perform the upgrade, then execute
-        those same discover phases again on new. All of this should occur
-        in a single task.
-        """
+    ) -> Iterator[tuple['ExecutePlugin[ExecuteStepData]', list['tmt.guest.Guest']]]:
+        # upgrade plugin is expected to (potentially) execute multiple
+        # discover phases on old, perform the upgrade, then execute
+        # those same discover phases again on new. All of this should occur
+        # in a single task.
         phase_copy = cast(ExecutePlugin[ExecuteStepData], copy.copy(self))
         # We want to run all discover phases on old and new, so set discover_phase to None.
         # This comes with the responsibility to later only run discover phases that are enabled
         # for that guest, based on discover.enabled_by_when and discover.enabled_on_guest(guest).
         phase_copy.discover_phase = None
 
-        return [
-            (
-                phase_copy,
-                self.step.plan.provision.ready_guests,
-            )
-        ]
+        yield (phase_copy, self.step.plan.provision.ready_guests)
 
     def go(
         self,
@@ -497,31 +489,36 @@ def _run_test_phase(self, guest: tmt.guest.Guest, prefix: str, logger: tmt.log.L
         # Backup the original discover phase, it should be None
         original_discover_phase = self.discover_phase
 
-        # Run discover phases one at a time,
-        # only running phases that are enabled for this guest.
-        for discover in self.step.plan.discover.phases(classes=(DiscoverPlugin,)):
-            if discover.enabled_by_when and discover.enabled_on_guest(guest):
-                self.discover_phase = discover.name
-
-                # Backup and modify test names for this discover phase
-                test_name_backups: list[tuple[tmt.base.core.Test, str]] = []
-                for test_origin in self.discover.tests(
-                    phase_name=self.discover_phase, enabled=True
-                ):
-                    test_name_backups.append((test_origin.test, test_origin.test.name))
-                    test_origin.test.name = f'/{prefix}/{test_origin.test.name.lstrip("/")}'
-
-                self._run_tests(
-                    guest=guest,
-                    extra_environment=Environment({STATUS_VARIABLE: EnvVarValue(prefix)}),
-                    logger=logger,
-                )
-
-                # Restore test names immediately after this phase
-                for test, original_name in test_name_backups:
-                    test.name = original_name
-
-        self.discover_phase = original_discover_phase
+        try:
+            # Run discover phases one at a time,
+            # only running phases that are enabled for this guest.
+            for discover in self.step.plan.discover.phases(classes=(DiscoverPlugin,)):
+                if discover.enabled_by_when and discover.enabled_on_guest(guest):
+                    self.discover_phase = discover.name
+
+                    test_name_backups: list[tuple[tmt.base.core.Test, str]] = []
+                    try:
+                        # Backup and modify test names for this discover phase
+                        for test_origin in self.discover.tests(
+                            phase_name=self.discover_phase, enabled=True
+                        ):
+                            test_name_backups.append((test_origin.test, test_origin.test.name))
+                            test_origin.test.name = (
+                                f'/{prefix}/{test_origin.test.name.lstrip("/")}'
+                            )
+
+                        self._run_tests(
+                            guest=guest,
+                            extra_environment=Environment({STATUS_VARIABLE: EnvVarValue(prefix)}),
+                            logger=logger,
+                        )
+
+                    finally:
+                        # Restore test names immediately after this phase
+                        for test, original_name in test_name_backups:
+                            test.name = original_name
+        finally:
+            self.discover_phase = original_discover_phase
 
         self._remove_old_results(prefix)