From dc843a089d74e2c3ff284fc511e4ab9a916e33b7 Mon Sep 17 00:00:00 2001
From: Petr Matyas <pmatyas@redhat.com>
Date: Wed, 22 May 2024 11:48:37 +0200
Subject: [PATCH] Rewrite under test with again and failed-only

Skip Discover.load() for runs executed with again to reload data
---
 docs/guide.rst                                |   6 +-
 docs/releases.rst                             |   7 ++
 tests/rerun/main.fmf                          |   2 -
 .../again/failed-only}/data/.fmf/version      |   0
 .../again/failed-only}/data/plan.fmf          |   0
 .../again/failed-only}/data/test.fmf          |   0
 tests/run/again/failed-only/main.fmf          |   2 +
 .../{rerun => run/again/failed-only}/test.sh  |  14 +--
 tmt/base.py                                   |  37 +------
 tmt/cli.py                                    |   6 +-
 tmt/steps/__init__.py                         |   6 +-
 tmt/steps/discover/__init__.py                | 103 +++++-------------
 tmt/steps/discover/shell.py                   |   6 +-
 tmt/steps/execute/__init__.py                 |  35 +++---
 tmt/utils.py                                  |   6 -
 15 files changed, 68 insertions(+), 162 deletions(-)
 delete mode 100644 tests/rerun/main.fmf
 rename tests/{rerun => run/again/failed-only}/data/.fmf/version (100%)
 rename tests/{rerun => run/again/failed-only}/data/plan.fmf (100%)
 rename tests/{rerun => run/again/failed-only}/data/test.fmf (100%)
 create mode 100644 tests/run/again/failed-only/main.fmf
 rename tests/{rerun => run/again/failed-only}/test.sh (63%)

diff --git a/docs/guide.rst b/docs/guide.rst
index 34b8c97854..8a6f9c8b93 100644
--- a/docs/guide.rst
+++ b/docs/guide.rst
@@ -913,10 +913,10 @@ Rerunning failed tests of previously executed runs
 ------------------------------------------------------------------
 
 Executing failed tests again after fixing them is now possible
-with `tmt run` argument `--rerun`.
+with `tmt run --again test --failed-only`.
 
 This is only possible when you have the run directory available
-and `--id` argument provided (or use ) as it needs the data from execute step
+and `--id` argument provided (or use `--last`) as it needs the data from execute step
 to select only failed test cases. After new execute step, tmt will
 again merge the results from the previous run with the new ones
 to keep all the data for full report.
@@ -926,7 +926,7 @@ to keep all the data for full report.
     $ tmt run --all
     # Some tests fail, some pass
 
-    $ tmt run --last --rerun
+    $ tmt run --last --again tests --failed-only
     # Failed tests are rerun
 
 
diff --git a/docs/releases.rst b/docs/releases.rst
index fe5aa42f83..093a2979cf 100644
--- a/docs/releases.rst
+++ b/docs/releases.rst
@@ -4,6 +4,13 @@
     Releases
 ======================
 
+tmt-1.34
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Added option ``--failed-only`` to the test attributes,
+enabling rerunning failed tests from previous runs.
+
+
 tmt-1.33
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/tests/rerun/main.fmf b/tests/rerun/main.fmf
deleted file mode 100644
index 4eae9c6f16..0000000000
--- a/tests/rerun/main.fmf
+++ /dev/null
@@ -1,2 +0,0 @@
-summary: Check rerun argument is working correctly
-tier: 3
diff --git a/tests/rerun/data/.fmf/version b/tests/run/again/failed-only/data/.fmf/version
similarity index 100%
rename from tests/rerun/data/.fmf/version
rename to tests/run/again/failed-only/data/.fmf/version
diff --git a/tests/rerun/data/plan.fmf b/tests/run/again/failed-only/data/plan.fmf
similarity index 100%
rename from tests/rerun/data/plan.fmf
rename to tests/run/again/failed-only/data/plan.fmf
diff --git a/tests/rerun/data/test.fmf b/tests/run/again/failed-only/data/test.fmf
similarity index 100%
rename from tests/rerun/data/test.fmf
rename to tests/run/again/failed-only/data/test.fmf
diff --git a/tests/run/again/failed-only/main.fmf b/tests/run/again/failed-only/main.fmf
new file mode 100644
index 0000000000..b7ba02aa55
--- /dev/null
+++ b/tests/run/again/failed-only/main.fmf
@@ -0,0 +1,2 @@
+summary: Check rerun is working correctly
+tier: 3
diff --git a/tests/rerun/test.sh b/tests/run/again/failed-only/test.sh
similarity index 63%
rename from tests/rerun/test.sh
rename to tests/run/again/failed-only/test.sh
index 6c134b0be7..5bd5742ba9 100755
--- a/tests/rerun/test.sh
+++ b/tests/run/again/failed-only/test.sh
@@ -10,25 +10,21 @@ rlJournalStart
         rlRun "tmt init"
     rlPhaseEnd
 
-    rlPhaseStartTest "Initial run with empty run directory and rerun argument"
-        rlRun -s "tmt run --all --rerun --id $run" 2 "Rerun tests with empty directory"
-        rlAssertGrep "Run id has to be specified" $rlRun_LOG
-    rlPhaseEnd
-
-    rlPhaseStartTest "Test basic rerun scenario is working"
+    rlPhaseStartTest "Test basic run failed tests again scenario is working"
         rlRun -s "tmt run --all --scratch --id $run" 1 "Run tests, some fail"
         rlAssertGrep "total: 2 tests passed and 2 tests failed" $rlRun_LOG
 
         rlRun "sed -i 's/false/true/g' *" 0 "Fix the test"
 
-        rlRun -s "tmt run --all --rerun --id $run" 0 "Rerun failed tests"
+        rlRun -s "tmt run --all --again --id $run tests --failed-only" 0 "Rerun failed tests"
         rlAssertGrep "1 test selected" $rlRun_LOG
         rlAssertGrep "total: 4 tests passed" $rlRun_LOG
     rlPhaseEnd
 
-    rlPhaseStartTest "Test another rerun does not execute anything"
-        rlRun -s "tmt run --all --rerun --id $run" 0 "Rerun failed tests again"
+    rlPhaseStartTest "Test another run with again does not execute anything"
+        rlRun -s "tmt run --all --again --id $run tests --failed-only" 0 "Rerun failed tests again"
         rlAssertGrep "0 tests selected" $rlRun_LOG
+        rlAssertNotGrep "1 tests selected" $rlRun_LOG
         rlAssertGrep "total: 4 tests passed" $rlRun_LOG
     rlPhaseEnd
 
diff --git a/tmt/base.py b/tmt/base.py
index 8f0b0a37ed..138c72f401 100644
--- a/tmt/base.py
+++ b/tmt/base.py
@@ -1669,21 +1669,6 @@ def __init__(
 
         self._plan_environment = Environment()
 
-        # Set directory for last run execute data in case of a rerun
-        if self.is_rerun:
-            assert self.workdir is not None  # narrow type
-            self.last_run_execute: Path = self.workdir / 'last_run_execute'
-
-        # Store 'environment' and 'environment-file' keys content
-        self._environment = tmt.utils.environment_from_spec(
-            raw_fmf_environment_files=node.get("environment-file") or [],
-            raw_fmf_environment=node.get('environment', {}),
-            raw_cli_environment_files=self.opt('environment-file') or [],
-            raw_cli_environment=self.opt('environment'),
-            file_root=Path(node.root) if node.root else None,
-            key_address=node.name,
-            logger=self._logger)
-
         # Expand all environment and context variables in the node
         with self.environment.as_environ():
             expand_node_data(node.data, self._fmf_context)
@@ -2248,23 +2233,6 @@ def _lint_step(step: str) -> LinterReturn:
     def wake(self) -> None:
         """ Wake up all steps """
 
-        # Additional debug info like plan environment
-        self.debug('info', color='cyan', shift=0, level=3)
-        # TODO: something better than str()?
-        self.debug('environment', format_value(self.environment), 'magenta', level=3)
-        self.debug('context', format_value(self._fmf_context), 'magenta', level=3)
-
-        # Save last run execute step if called with rerun
-        if self.is_rerun:
-            assert self.workdir is not None  # narrow type
-            if not (self.workdir / 'execute').exists():
-                raise tmt.utils.GeneralError(
-                    "Run id has to be specified and "
-                    "execute directory has to exist in order to use --rerun.")
-            self.debug(f"Saving last run execute into {self.last_run_execute}.")
-            shutil.copytree(self.workdir / 'execute', self.last_run_execute, dirs_exist_ok=True)
-
-        # Wake up all steps
         self.debug('wake', color='cyan', shift=0, level=2)
         for step in self.steps(enabled_only=False):
             self.debug(str(step), color='blue', level=2)
@@ -2335,10 +2303,9 @@ def go(self) -> None:
         try:
             for step in self.steps(skip=['finish']):
                 step.go()
-                # Finish plan if no tests found (except dry mode and rerun)
+                # Finish plan if no tests found (except dry mode)
                 if (isinstance(step, tmt.steps.discover.Discover) and not step.tests()
-                        and not self.is_dry_run and not step.extract_tests_later
-                        and not self.is_rerun):
+                        and not self.is_dry_run and not step.extract_tests_later):
                     step.info(
                         'warning', 'No tests found, finishing plan.',
                         color='yellow', shift=1)
diff --git a/tmt/cli.py b/tmt/cli.py
index 85383cb7b5..15f1f94388 100644
--- a/tmt/cli.py
+++ b/tmt/cli.py
@@ -383,9 +383,6 @@ def main(
 @option(
     '--scratch', is_flag=True,
     help='Remove the run workdir before executing to start from scratch.')
-@option(
-    '--rerun', is_flag=True,
-    help='Rerun failed tests and update existing results.')
 @option(
     '--follow', is_flag=True,
     help='Output the logfile as it grows.')
@@ -485,6 +482,9 @@ def run_plans(context: Context, **kwargs: Any) -> None:
     help="""
          Filter by linked objects (regular expressions are supported for both relation and target).
          """)
+@option(
+    '--failed-only', is_flag=True, default=False,
+    help="Filter failed tests from a previous run to run again")
 @verbosity_options
 def run_tests(context: Context, **kwargs: Any) -> None:
     """
diff --git a/tmt/steps/__init__.py b/tmt/steps/__init__.py
index 1a63a2e58f..30892f25e0 100644
--- a/tmt/steps/__init__.py
+++ b/tmt/steps/__init__.py
@@ -628,6 +628,10 @@ def summary(self) -> None:
 
     def load(self) -> None:
         """ Load status and step data from the workdir """
+        if self.should_run_again and isinstance(self, tmt.steps.discover.Discover):
+            self.debug('Run discover again when reexecuting to capture changes in plans')
+            return
+
         try:
             raw_step_data: dict[Any, Any] = tmt.utils.yaml_to_dict(self.read(Path('step.yaml')))
 
@@ -664,7 +668,7 @@ def wake(self) -> None:
         """ Wake up the step (process workdir and command line) """
         # Cleanup possible old workdir if called with --force, but not
         # if running the step --again which should reuse saved step data
-        if (self.is_forced_run or self.is_rerun) and not self.should_run_again:
+        if self.is_forced_run and not self.should_run_again:
             self._workdir_cleanup()
 
         # Load stored data
diff --git a/tmt/steps/discover/__init__.py b/tmt/steps/discover/__init__.py
index 2afde882ae..efcbd8df0a 100644
--- a/tmt/steps/discover/__init__.py
+++ b/tmt/steps/discover/__init__.py
@@ -17,7 +17,6 @@
 import tmt.utils
 from tmt.options import option
 from tmt.plugins import PluginRegistry
-from tmt.result import Result
 from tmt.steps import Action
 from tmt.utils import GeneralError, Path, field, key_to_option
 
@@ -157,37 +156,6 @@ def post_dist_git(self, created_content: list[Path]) -> None:
         """ Discover tests after dist-git applied patches """
         pass
 
-    def filter_for_rerun(self) -> None:
-        """ Filter out passed tests from previous run data """
-        assert isinstance(self.step.parent, tmt.base.Plan)  # narrow type
-        old_results: Path = self.step.parent.last_run_execute / 'results.yaml'
-        results = [
-            Result.from_serialized(data) for data in
-            tmt.utils.yaml_to_list(self.read(old_results))]
-        results_failed: list[str] = []
-        results_passed: list[Result] = []
-        for result in results:
-            if (
-                    result.result is not tmt.result.ResultOutcome.PASS and
-                    result.result is not tmt.result.ResultOutcome.INFO):
-                results_failed.append(result.name)
-            else:
-                results_passed.append(result)
-
-        # Overwrite previous run results to only include passed cases
-        self.debug(
-            f"Overwriting {old_results} to only include passed results: "
-            f"{', '.join([result.name for result in results_passed])}")
-        self.write(
-            old_results,
-            tmt.utils.dict_to_yaml([result.to_serialized() for result in results_passed]))
-
-        tests_to_execute: list[tmt.base.Test] = []
-        for test in self._tests:
-            if test.name in results_failed:
-                tests_to_execute.append(test)
-        self._tests: list[tmt.base.Test] = tests_to_execute
-
 
 class Discover(tmt.steps.Step):
     """ Gather information about test cases to be executed. """
@@ -206,6 +174,7 @@ def __init__(
 
         # Collection of discovered tests
         self._tests: dict[str, list[tmt.Test]] = {}
+        self._failed_tests: dict[str, list[tmt.Test]] = {}
 
         # Test will be (re)discovered in other phases/steps
         self.extract_tests_later: bool = False
@@ -268,44 +237,6 @@ def save(self) -> None:
 
         self.write(Path('tests.yaml'), tmt.utils.dict_to_yaml(raw_test_data))
 
-    def _filter_for_rerun(self) -> None:
-        """ Filter out passed tests from previous run data """
-        assert isinstance(self.parent, tmt.base.Plan)  # narrow type
-        old_results: Path = self.parent.last_run_execute / 'results.yaml'
-        results = [
-            Result.from_serialized(data) for data in
-            tmt.utils.yaml_to_list(self.read(old_results))]
-        results_failed: list[Result] = []
-        results_passed: list[Result] = []
-        for result in results:
-            if (
-                    result.result is not tmt.result.ResultOutcome.PASS and
-                    result.result is not tmt.result.ResultOutcome.INFO):
-                results_failed.append(result)
-            else:
-                results_passed.append(result)
-
-        # Save positive results to specific results.yaml
-        old_results_positive: Path = (
-            self.parent.last_run_execute / 'positive_results.yaml')
-        self.debug(
-            f"Save positive results from last run to {old_results_positive}, these are: "
-            f"{', '.join([result.name for result in results_passed])}")
-        self.write(
-            old_results_positive,
-            tmt.utils.dict_to_yaml([result.to_serialized() for result in results_passed]))
-
-        # Filter out failed tests based on test name and serial number
-        filtered_tests: dict[str, list[tmt.base.Test]] = {}
-        for phase in self._tests:
-            current_phase_filtered: list[tmt.base.Test] = []
-            for test in self._tests[phase]:
-                for result in results_failed:
-                    if test.name == result.name and test.serial_number == result.serial_number:
-                        current_phase_filtered.append(test)
-            filtered_tests[phase] = current_phase_filtered
-        self._tests = filtered_tests
-
     def _discover_from_execute(self) -> None:
         """ Check the execute step for possible shell script tests """
 
@@ -428,10 +359,6 @@ def go(self, force: bool = False) -> None:
         for test in self.tests():
             test.serial_number = self.plan.draw_test_serial_number(test)
 
-        # Filter selected tests if this is a rerun
-        if self.is_rerun:
-            self._filter_for_rerun()
-
         # Show fmf identifiers for tests discovered in plan
         # TODO: This part should go into the 'fmf.py' module
         if self.opt('fmf_id'):
@@ -454,6 +381,28 @@ def go(self, force: bool = False) -> None:
                 click.echo(''.join(export_fmf_ids), nl=False)
             return
 
+        if self.should_run_again and tmt.base.Test._opt('failed_only'):
+            failed_results: list[tmt.base.Result] = []
+            assert self.parent is not None  # narrow type
+            assert isinstance(self.parent, tmt.base.Plan)  # narrow type
+
+            # Get failed results from previous run execute
+            for result in self.parent.execute._results:
+                if (
+                        result.result is not tmt.result.ResultOutcome.PASS and
+                        result.result is not tmt.result.ResultOutcome.INFO):
+                    failed_results.append(result)
+
+            # Filter existing tests into another variable which is then used by tests() method
+            for test_phase in self._tests:
+                self._failed_tests[test_phase] = []
+                for test in self._tests[test_phase]:
+                    for result in failed_results:
+                        if (
+                                test.name == result.name and
+                                test.serial_number == result.serial_number):
+                            self._failed_tests[test_phase].append(test)
+
         # Give a summary, update status and save
         self.summary()
         self.status('done')
@@ -465,13 +414,15 @@ def tests(
             phase_name: Optional[str] = None,
             enabled: Optional[bool] = None) -> list['tmt.Test']:
         def _iter_all_tests() -> Iterator['tmt.Test']:
-            for phase_tests in self._tests.values():
+            tests = self._failed_tests if self._failed_tests else self._tests
+            for phase_tests in tests.values():
                 yield from phase_tests
 
         def _iter_phase_tests() -> Iterator['tmt.Test']:
             assert phase_name is not None
+            tests = self._failed_tests if self._failed_tests else self._tests
 
-            yield from self._tests[phase_name]
+            yield from tests[phase_name]
 
         iterator = _iter_all_tests if phase_name is None else _iter_phase_tests
 
diff --git a/tmt/steps/discover/shell.py b/tmt/steps/discover/shell.py
index fbb83faa3e..6fd76f8881 100644
--- a/tmt/steps/discover/shell.py
+++ b/tmt/steps/discover/shell.py
@@ -443,11 +443,7 @@ def go(self) -> None:
         self._tests = tmt.Tree(
             logger=self._logger,
             tree=tests).tests(
-            conditions=["manual is False"])
-
-        # Filter selected tests if this is a rerun
-        if self.is_rerun:
-            self.filter_for_rerun()
+                conditions=["manual is False"])
 
         # Propagate `where` key and TMT_SOURCE_DIR
         for test in self._tests:
diff --git a/tmt/steps/execute/__init__.py b/tmt/steps/execute/__init__.py
index b4f314cd74..660bae802f 100644
--- a/tmt/steps/execute/__init__.py
+++ b/tmt/steps/execute/__init__.py
@@ -6,7 +6,6 @@
 import signal as _signal
 import subprocess
 import threading
-import shutil
 from contextlib import suppress
 from dataclasses import dataclass
 from typing import TYPE_CHECKING, Any, Optional, TypeVar, Union, cast
@@ -527,6 +526,15 @@ def prepare_tests(self, guest: Guest, logger: tmt.log.Logger) -> list[TestInvoca
                 invocation.path / TEST_METADATA_FILENAME,
                 tmt.utils.dict_to_yaml(test_metadata))
 
+            # If we are running again then we need to clear the previous result
+            if self.should_run_again:
+                assert self.parent is not None  # narrow type
+                assert isinstance(self.parent, tmt.steps.execute.Execute)  # narrow type
+                self.parent._results = [
+                    result for result in self.parent._results
+                    if not (
+                        test.name == result.name and test.serial_number == result.serial_number)]
+
         return invocations
 
     def prepare_scripts(self, guest: "tmt.steps.provision.Guest") -> None:
@@ -811,29 +819,9 @@ def load(self) -> None:
         except tmt.utils.FileError:
             self.debug('Test results not found.', level=2)
 
-    def merge_results_rerun(self) -> None:
-        """ Merge new results with old ones for rerun """
-        assert isinstance(self.parent, tmt.base.Plan)  # narrow type
-        old_results: list[Result] = [
-            Result.from_serialized(data) for data in
-            tmt.utils.yaml_to_list(
-                self.read(self.parent.last_run_execute / 'positive_results.yaml'))]
-
-        for result in old_results:
-            # Add old results into new ones and copy log directories
-            self._results.append(result)
-            assert self.workdir is not None  # narrow type
-            assert result.data_path is not None  # narrow type
-            shutil.copytree(
-                self.parent.last_run_execute / result.data_path.parent,
-                self.workdir / result.data_path.parent,
-                dirs_exist_ok=True)
-
     def save(self) -> None:
         """ Save test results to the workdir """
         super().save()
-        if self.is_rerun and self.status() == 'done':
-            self.merge_results_rerun()
         results = [result.to_serialized() for result in self.results()]
         self.write(Path('results.yaml'), tmt.utils.dict_to_yaml(results))
 
@@ -882,9 +870,12 @@ def go(self, force: bool = False) -> None:
         super().go(force=force)
 
         # Clean up possible old results
-        if force or self.should_run_again:
+        if force:
             self._results.clear()
 
+        if self.should_run_again:
+            self.status('todo')
+
         # Nothing more to do if already done
         if self.status() == 'done':
             self.info('status', 'done', 'green', shift=1)
diff --git a/tmt/utils.py b/tmt/utils.py
index 561f148928..c589991596 100644
--- a/tmt/utils.py
+++ b/tmt/utils.py
@@ -1856,12 +1856,6 @@ def is_feeling_safe(self) -> bool:
 
         return self._get_cli_flag('is_feeling_safe', 'feeling_safe', False)
 
-    @property
-    def is_rerun(self) -> bool:
-        """ Whether the current run is a rerun and so allowed to overwrite files and data """
-
-        return self._get_cli_flag('is_rerun', 'rerun', False)
-
     def _level(self) -> int:
         """ Hierarchy level """
         if self.parent is None: