From 6606bbd4c8ab867cb25ea15c4d147b2b7f9118cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Prchl=C3=ADk?= Date: Mon, 4 Dec 2023 16:43:24 +0100 Subject: [PATCH] Clear test invocation data path use and derived paths There was a "data path" tied to a test invocation, then there was a "test data path" which was the "data path" plus one more `/data`. The patch turns `TestInvocation.data_path()` into `TestInvocation.path`, adds caching and defines the test data path. Together, they should provide clearer view on paths related to a test and its invocations, simplifying the related code. --- tmt/frameworks/beakerlib.py | 10 ++-- tmt/frameworks/shell.py | 2 +- tmt/result.py | 2 +- tmt/steps/execute/__init__.py | 91 +++++++++++++++++++---------------- tmt/steps/execute/internal.py | 14 +++--- 5 files changed, 62 insertions(+), 57 deletions(-) diff --git a/tmt/frameworks/beakerlib.py b/tmt/frameworks/beakerlib.py index 6fddc649c4..577dea9845 100644 --- a/tmt/frameworks/beakerlib.py +++ b/tmt/frameworks/beakerlib.py @@ -23,7 +23,7 @@ def get_environment_variables( logger: tmt.log.Logger) -> tmt.utils.EnvironmentType: return { - 'BEAKERLIB_DIR': str(invocation.data_path(full=True)), + 'BEAKERLIB_DIR': str(invocation.path), 'BEAKERLIB_COMMAND_SUBMIT_LOG': f'bash {tmt.steps.execute.TMT_FILE_SUBMIT_SCRIPT.path}' } @@ -34,7 +34,7 @@ def get_pull_options( logger: tmt.log.Logger) -> list[str]: return [ '--exclude', - str(invocation.data_path("backup*", full=True)) + str(invocation.path / "backup*") ] @classmethod @@ -47,11 +47,11 @@ def extract_results( note: Optional[str] = None log: list[Path] = [] for filename in [tmt.steps.execute.TEST_OUTPUT_FILENAME, 'journal.txt']: - if invocation.data_path(filename, full=True).is_file(): - log.append(invocation.data_path(filename)) + if (invocation.path / filename).is_file(): + log.append(invocation.path / filename) # Check beakerlib log for the result - beakerlib_results_filepath = invocation.data_path('TestResults', full=True) + beakerlib_results_filepath = invocation.path / 'TestResults' try: results = invocation.phase.read(beakerlib_results_filepath, level=3) diff --git a/tmt/frameworks/shell.py b/tmt/frameworks/shell.py index 942995c178..2dd22fa3da 100644 --- a/tmt/frameworks/shell.py +++ b/tmt/frameworks/shell.py @@ -48,5 +48,5 @@ def extract_results( return [tmt.Result.from_test_invocation( invocation=invocation, result=result, - log=[invocation.data_path(tmt.steps.execute.TEST_OUTPUT_FILENAME)], + log=[invocation.relative_path / tmt.steps.execute.TEST_OUTPUT_FILENAME], note=note)] diff --git a/tmt/result.py b/tmt/result.py index fb95c20582..c31cbd14ae 100644 --- a/tmt/result.py +++ b/tmt/result.py @@ -232,7 +232,7 @@ def from_test_invocation( ids=ids, log=log or [], guest=guest_data, - data_path=invocation.data_path('data')) + data_path=invocation.relative_test_data_path) return _result.interpret_result(ResultInterpret( invocation.test.result) if invocation.test.result else ResultInterpret.RESPECT) diff --git a/tmt/steps/execute/__init__.py b/tmt/steps/execute/__init__.py index 64ee27147e..04830bb1a2 100644 --- a/tmt/steps/execute/__init__.py +++ b/tmt/steps/execute/__init__.py @@ -23,7 +23,7 @@ from tmt.steps import Action, PhaseQueue, QueuedPhase, Step from tmt.steps.discover import Discover, DiscoverPlugin, DiscoverStepData from tmt.steps.provision import Guest -from tmt.utils import Path, ShellScript, Stopwatch, field +from tmt.utils import Path, ShellScript, Stopwatch, cached_property, field if TYPE_CHECKING: import tmt.cli @@ -159,44 +159,58 @@ class TestInvocation: _reboot_count: int = 0 - def data_path( - self, - filename: Optional[str] = None, - full: bool = False, - create: bool = False) -> Path: - """ - Prepare full/relative test data directory/file path. + @cached_property + def path(self) -> Path: + """ Absolute path to invocation directory """ - Construct test data directory path for given test, create it - if requested and return the full or relative path to it (if - filename not provided) or to the given data file otherwise. - """ - # Prepare directory path, create if requested assert self.phase.step.workdir is not None # narrow type - directory = self.phase.step.workdir \ + + path = self.phase.step.workdir \ / TEST_DATA \ / 'guest' \ / self.guest.safe_name \ / f'{self.test.safe_name.lstrip("/") or "default"}-{self.test.serial_number}' - if create and not directory.is_dir(): - directory.joinpath(TEST_DATA).mkdir(parents=True) - if not filename: - return directory - path = directory / filename - return path if full else path.relative_to(self.phase.step.workdir) + + path.mkdir(parents=True, exist_ok=True) + + # Pre-create also the test data path - cannot use `self.test_data_path`, + # that would be an endless recursion. + (path / TEST_DATA).mkdir(parents=True, exist_ok=True) + + return path + + @cached_property + def relative_path(self) -> Path: + """ Invocation directory path relative to step workdir """ + + assert self.phase.step.workdir is not None # narrow type + + return self.path.relative_to(self.phase.step.workdir) + + @cached_property + def test_data_path(self) -> Path: + """ Absolute path to test data directory """ + + return self.path / TEST_DATA + + @cached_property + def relative_test_data_path(self) -> Path: + """ Test data path relative to step workdir """ + + return self.relative_path / TEST_DATA @tmt.utils.cached_property def check_files_path(self) -> Path: """ Construct a directory path for check files needed by tmt """ - path = self.data_path(create=True, full=True) / "checks" + + path = self.path / "checks" path.mkdir(exist_ok=True) return path @tmt.utils.cached_property def reboot_request_path(self) -> Path: """ A path to the reboot request file """ - return self.data_path(full=True) \ - / TEST_DATA \ + return self.test_data_path \ / TMT_REBOOT_SCRIPT.created_file @property @@ -224,8 +238,6 @@ def handle_reboot(self) -> bool: self.logger.debug( f"Reboot during test '{self.test}' with reboot count {self._reboot_count}.") - test_data = self.data_path(full=True) / TEST_DATA - with open(self.reboot_request_path) as reboot_file: reboot_data = json.loads(reboot_file.read()) @@ -241,7 +253,7 @@ def handle_reboot(self) -> bool: # Reset the file os.remove(self.reboot_request_path) - self.guest.push(test_data) + self.guest.push(self.test_data_path) rebooted = False @@ -355,10 +367,9 @@ def prepare_tests(self, guest: Guest, logger: tmt.log.Logger) -> list[TestInvoca invocation = TestInvocation(phase=self, test=test, guest=guest, logger=logger) invocations.append(invocation) - metadata_filename = invocation.data_path( - filename=TEST_METADATA_FILENAME, full=True, create=True) self.write( - metadata_filename, tmt.utils.dict_to_yaml(test._metadata)) + invocation.path / TEST_METADATA_FILENAME, + tmt.utils.dict_to_yaml(test._metadata)) return invocations @@ -380,8 +391,7 @@ def prepare_scripts(self, guest: "tmt.steps.provision.Guest") -> None: def _tmt_report_results_filepath(self, invocation: TestInvocation) -> Path: """ Create path to test's ``tmt-report-result`` file """ - return invocation.data_path(full=True) \ - / TEST_DATA \ + return invocation.test_data_path \ / TMT_REPORT_RESULT_SCRIPT.created_file def load_tmt_report_results(self, invocation: TestInvocation) -> list["tmt.Result"]: @@ -424,7 +434,7 @@ def load_tmt_report_results(self, invocation: TestInvocation) -> list["tmt.Resul return [tmt.Result.from_test_invocation( invocation=invocation, result=actual_result, - log=[invocation.data_path(TEST_OUTPUT_FILENAME)], + log=[invocation.relative_path / TEST_OUTPUT_FILENAME], note=note)] def load_custom_results(self, invocation: TestInvocation) -> list["tmt.Result"]: @@ -433,10 +443,8 @@ def load_custom_results(self, invocation: TestInvocation) -> list["tmt.Result"]: """ test, guest = invocation.test, invocation.guest - test_data_path = invocation.data_path(full=True) / TEST_DATA - - custom_results_path_yaml = test_data_path / 'results.yaml' - custom_results_path_json = test_data_path / 'results.json' + custom_results_path_yaml = invocation.test_data_path / 'results.yaml' + custom_results_path_json = invocation.test_data_path / 'results.json' if custom_results_path_yaml.exists(): with open(custom_results_path_yaml) as results_file: @@ -449,7 +457,7 @@ def load_custom_results(self, invocation: TestInvocation) -> list["tmt.Result"]: else: return [tmt.Result.from_test_invocation( invocation=invocation, - note=f"custom results file not found in '{test_data_path}'", + note=f"custom results file not found in '{invocation.test_data_path}'", result=ResultOutcome.ERROR)] if not results: @@ -477,8 +485,8 @@ def load_custom_results(self, invocation: TestInvocation) -> list["tmt.Result"]: # Fix log paths as user provides relative path to TMT_TEST_DATA # but Result has to point relative to the execute workdir - log_path_base = invocation.data_path(full=False, filename=TEST_DATA) - partial_result.log = [log_path_base / log for log in partial_result.log] + partial_result.log = [ + invocation.relative_test_data_path / log for log in partial_result.log] # TODO: this might need more care: the test has been assigned a serial # number, which is now part of its data directory path. Now, the test @@ -532,8 +540,7 @@ def check_abort_file(self, invocation: TestInvocation) -> bool: Returns whether an abort file is present (i.e. abort occurred). """ - return (invocation.data_path(full=True) - / TEST_DATA + return (invocation.test_data_path / TMT_ABORT_SCRIPT.created_file).exists() @staticmethod @@ -556,7 +563,7 @@ def format_duration(duration: datetime.timedelta) -> str: def timeout_hint(self, invocation: TestInvocation) -> None: """ Append a duration increase hint to the test output """ - output = invocation.data_path(TEST_OUTPUT_FILENAME, full=True) + output = invocation.path / TEST_OUTPUT_FILENAME self.write( output, f"\nMaximum test time '{invocation.test.duration}' exceeded.\n" diff --git a/tmt/steps/execute/internal.py b/tmt/steps/execute/internal.py index 74b8e47f7e..adbf6f7458 100644 --- a/tmt/steps/execute/internal.py +++ b/tmt/steps/execute/internal.py @@ -223,8 +223,6 @@ def _test_environment( extra_environment = extra_environment or {} - data_directory = invocation.data_path(full=True, create=True) - environment = extra_environment.copy() environment.update(invocation.test.environment) assert self.parent is not None @@ -235,12 +233,12 @@ def _test_environment( environment['TMT_TEST_PIDFILE_LOCK'] = str( effective_pidfile_root() / TEST_PIDFILE_LOCK_FILENAME) environment["TMT_TEST_NAME"] = invocation.test.name - environment["TMT_TEST_DATA"] = str(data_directory / tmt.steps.execute.TEST_DATA) + environment["TMT_TEST_DATA"] = str(invocation.test_data_path) environment['TMT_TEST_SERIAL_NUMBER'] = str(invocation.test.serial_number) environment["TMT_TEST_METADATA"] = str( - data_directory / tmt.steps.execute.TEST_METADATA_FILENAME) + invocation.path / tmt.steps.execute.TEST_METADATA_FILENAME) environment["TMT_REBOOT_REQUEST"] = str( - data_directory / tmt.steps.execute.TEST_DATA / TMT_REBOOT_SCRIPT.created_file) + invocation.test_data_path / TMT_REBOOT_SCRIPT.created_file) # Set all supported reboot variables for reboot_variable in TMT_REBOOT_SCRIPT.related_variables: environment[reboot_variable] = str(invocation._reboot_count) @@ -315,7 +313,7 @@ def execute( topology.guest = tmt.steps.GuestTopology(guest) environment.update(topology.push( - dirpath=invocation.data_path(full=True), + dirpath=invocation.path, guest=guest, logger=logger)) @@ -386,7 +384,7 @@ def _test_output_logger( invocation.real_duration = self.format_duration(timer.duration) self.write( - invocation.data_path(TEST_OUTPUT_FILENAME, full=True), + invocation.path / TEST_OUTPUT_FILENAME, stdout or '', mode='a', level=3) # TODO: do we want timestamps? Yes, we do, leaving that for refactoring later, @@ -452,7 +450,7 @@ def _run_tests( logger=logger) guest.pull( - source=invocation.data_path(full=True), + source=invocation.path, extend_options=test.test_framework.get_pull_options(invocation, logger)) results = self.extract_results(invocation, logger) # Produce list of results