From f5857aed329807cfe9a6723fed7b9e1cc085962a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pl=C3=ADchal?= Date: Fri, 7 Oct 2022 11:14:21 +0200 Subject: [PATCH] Adjust the support for importing remote plans Extend the test coverage to cover shallow/verbose combinations. Include a full plan using test scripts to verify execution. Directly use `FmfId` for storing the remote plan reference. Prevent worktree clash, fix extra default plan injection. Show import plan info for shallow verbose as well. Plus a bunch of naming, docstring and style adjustments. --- spec/plans/import.fmf | 2 + tests/plan/import/data/plans.fmf | 22 ++++ .../import/data/plans/import-ref-n-dir.fmf | 8 -- .../plan/import/data/plans/minimal-import.fmf | 6 - tests/plan/import/test.sh | 73 ++++++++++--- tmt/base.py | 103 +++++++++++++----- tmt/steps/discover/fmf.py | 11 +- tmt/templates.py | 5 +- 8 files changed, 162 insertions(+), 68 deletions(-) create mode 100644 tests/plan/import/data/plans.fmf delete mode 100644 tests/plan/import/data/plans/import-ref-n-dir.fmf delete mode 100644 tests/plan/import/data/plans/minimal-import.fmf diff --git a/spec/plans/import.fmf b/spec/plans/import.fmf index 4ff300da9b..b6decfd3b1 100644 --- a/spec/plans/import.fmf +++ b/spec/plans/import.fmf @@ -53,3 +53,5 @@ example: link: - relates: https://github.com/teemtee/tmt/issues/975 + - verified-by: /tests/plan/import + - implemented-by: /tmp/base.py diff --git a/tests/plan/import/data/plans.fmf b/tests/plan/import/data/plans.fmf new file mode 100644 index 0000000000..2b0745627e --- /dev/null +++ b/tests/plan/import/data/plans.fmf @@ -0,0 +1,22 @@ +/minimal: + summary: Just url and name + plan: + import: + url: https://github.com/teemtee/tmt + name: /plans/sanity/lint + +/ref-and-path: + summary: Git ref and custom path + plan: + import: + url: https://github.com/teemtee/tmt + path: /tests/run/worktree/data/prepare + name: /plan + ref: 1.16.0 + +/full: + summary: Full plan with test scripts + plan: + import: + url: https://github.com/teemtee/fmf + name: /plans/features diff --git a/tests/plan/import/data/plans/import-ref-n-dir.fmf b/tests/plan/import/data/plans/import-ref-n-dir.fmf deleted file mode 100644 index 9aeb8162f5..0000000000 --- a/tests/plan/import/data/plans/import-ref-n-dir.fmf +++ /dev/null @@ -1,8 +0,0 @@ -summary: Import plan with git ref and custom working dir - -plan: - import: - url: https://github.com/teemtee/tmt - name: /plan - ref: 1.16.0 - path: tests/run/worktree/data/prepare diff --git a/tests/plan/import/data/plans/minimal-import.fmf b/tests/plan/import/data/plans/minimal-import.fmf deleted file mode 100644 index 70dc7456dd..0000000000 --- a/tests/plan/import/data/plans/minimal-import.fmf +++ /dev/null @@ -1,6 +0,0 @@ -summary: Minimal import plan test - -plan: - import: - url: https://github.com/teemtee/tmt - name: /plans/sanity/lint diff --git a/tests/plan/import/test.sh b/tests/plan/import/test.sh index 2da9cecc03..663b8d239b 100755 --- a/tests/plan/import/test.sh +++ b/tests/plan/import/test.sh @@ -6,28 +6,73 @@ rlJournalStart rlRun "pushd data" rlPhaseEnd - rlPhaseStartTest "Import Plan Show" + rlPhaseStartTest "Explore Plans" rlRun -s "tmt plan" - rlAssertNotGrep "warn" $rlRun_LOG - rlAssertGrep "Found 2 plans" $rlRun_LOG + rlAssertNotGrep "warn" $rlRun_LOG + rlAssertGrep "Found 3 plans" $rlRun_LOG + rlPhaseEnd + + rlPhaseStartTest "Show Plans (deep)" + rlRun -s "tmt plan show" + rlAssertGrep "/plans/minimal" $rlRun_LOG + rlAssertNotGrep "summary Just url and name" $rlRun_LOG + rlAssertGrep "summary Metadata used by tmt itself are valid" $rlRun_LOG + rlAssertNotGrep "import" $rlRun_LOG + rlAssertNotGrep "ref 1.16.0" $rlRun_LOG + rlAssertNotGrep "warn" $rlRun_LOG + rlPhaseEnd + + rlPhaseStartTest "Show Plans (shallow)" + rlRun -s "tmt plan show --shallow" + rlAssertGrep "/plans/minimal" $rlRun_LOG + rlAssertGrep "summary Just url and name" $rlRun_LOG + rlAssertNotGrep "summary Metadata used by tmt itself are valid" $rlRun_LOG + rlAssertNotGrep "import" $rlRun_LOG + rlAssertNotGrep "ref 1.16.0" $rlRun_LOG + rlAssertNotGrep "warn" $rlRun_LOG + rlPhaseEnd + + rlPhaseStartTest "Show Plans (verbose, deep)" + rlRun -s "tmt plan show --verbose" + rlAssertGrep "/plans/minimal" $rlRun_LOG + rlAssertNotGrep "summary Just url and name" $rlRun_LOG + rlAssertGrep "summary Metadata used by tmt itself are valid" $rlRun_LOG + rlAssertGrep "import" $rlRun_LOG + rlAssertGrep "url https://github.com/teemtee/tmt" $rlRun_LOG + rlAssertGrep "path /tests/run/worktree/data/prepare" $rlRun_LOG + rlAssertGrep "name /plan" $rlRun_LOG + rlAssertGrep "ref 1.16.0" $rlRun_LOG + rlAssertNotGrep "warn" $rlRun_LOG rlPhaseEnd - rlPhaseStartTest "Import Plan Show detailed" - rlRun -s "tmt plan show -v" + rlPhaseStartTest "Show Plans (verbose, shallow)" + rlRun -s "tmt plan show --verbose --shallow" + rlAssertGrep "/plans/minimal" $rlRun_LOG + rlAssertGrep "summary Just url and name" $rlRun_LOG + rlAssertNotGrep "summary Metadata used by tmt itself are valid" $rlRun_LOG + rlAssertGrep "import" $rlRun_LOG + rlAssertGrep "url https://github.com/teemtee/tmt" $rlRun_LOG + rlAssertGrep "path /tests/run/worktree/data/prepare" $rlRun_LOG + rlAssertGrep "name /plan" $rlRun_LOG + rlAssertGrep "ref 1.16.0" $rlRun_LOG rlAssertNotGrep "warn" $rlRun_LOG - rlAssertGrep "imported plan" $rlRun_LOG - rlAssertGrep "/plans/minimal-import" $rlRun_LOG rlPhaseEnd - rlPhaseStartTest "Import Plan Run Discover" - rlRun -s "tmt run discover" - rlAssertGrep "/plans/minimal-import" $rlRun_LOG + rlPhaseStartTest "Discover Tests" + rlRun -s "tmt run discover -v" + rlAssertGrep "/plans/full" $rlRun_LOG + rlAssertGrep "/tests/basic/ls" $rlRun_LOG + rlAssertGrep "/tests/basic/show" $rlRun_LOG + rlAssertGrep "/plans/minimal" $rlRun_LOG + rlAssertGrep "/lint/tests" $rlRun_LOG + rlAssertGrep "/lint/plans" $rlRun_LOG + rlAssertNotGrep "/plans/default" $rlRun_LOG rlPhaseEnd - rlPhaseStartTest "Import Plan Run" - rlRun -s "tmt run -a" - rlAssertGrep "/plans/minimal-import" $rlRun_LOG - rlAssertGrep "summary: 1 test passed" $rlRun_LOG + rlPhaseStartTest "Run Tests" + rlRun -s "tmt run -v" + rlAssertGrep "pass /tests/basic/ls" $rlRun_LOG + rlAssertGrep "pass /lint/tests" $rlRun_LOG rlPhaseEnd rlPhaseStartCleanup diff --git a/tmt/base.py b/tmt/base.py index 46a74b421f..2f9ca59ccb 100644 --- a/tmt/base.py +++ b/tmt/base.py @@ -906,8 +906,11 @@ class Plan(Core): _normalize_context = tmt.utils.LoadFmfKeysMixin._normalize_environment _normalize_gate = tmt.utils.LoadFmfKeysMixin._normalize_string_list + # When fetching remote plans we store links between the original + # plan with the fmf id and the imported plan with the content. _imported_plan: Optional['Plan'] = None - _origin_plan: Optional['Plan'] = None + _original_plan: Optional['Plan'] = None + _remote_plan_fmf_id: Optional[FmfId] = None extra_L2_keys = [ 'context', @@ -934,6 +937,11 @@ def __init__( raise_on_validation_error=raise_on_validation_error, **kwargs) + # Check for possible remote plan reference first + reference = self.node.get(['plan', 'import']) + if reference is not None: + self._remote_plan_fmf_id = FmfId(**reference) + # Save the run, prepare worktree and plan data directory self.my_run = run if self.my_run: @@ -965,8 +973,6 @@ def __init__( self.finish = tmt.steps.finish.Finish( plan=self, data=self.node.get('finish')) - self._import_plan_node = self.node.get(['plan', 'import']) - # Test execution context defined in the plan self._plan_context: FmfContextType = self.node.get('context', dict()) @@ -1025,11 +1031,15 @@ def _initialize_worktree(self) -> None: Used as cwd in prepare, execute and finish steps. """ - assert self.my_run is not None # narrow type + + # Do nothing for remote plan reference + if self.is_remote_plan_reference: + return + + # Prepare worktree path and detect the source tree root assert self.workdir is not None # narrow type self.worktree = os.path.join(self.workdir, 'tree') - assert self.my_run.tree is not None # narrow type - tree_root = self.my_run.tree.root + tree_root = self.node.root # Create an empty directory if there's no metadata tree if not tree_root: @@ -1233,12 +1243,19 @@ def show(self) -> None: if self.opt('verbose'): self._show_additional_keys() - if self._origin_plan and self.opt('verbose'): - echo(tmt.utils.format( - 'imported plan', '', key_color='blue')) - for k, v in self._origin_plan._import_plan_node.items(): - echo(tmt.utils.format( - k, v, key_color='green')) + # Show fmf id of the remote plan in verbose mode + if (self._original_plan or self._remote_plan_fmf_id) and self.opt('verbose'): + # Pick fmf id from the original plan by default, use the + # current plan in shallow mode when no plans are fetched. + if self._original_plan is not None: + fmf_id = self._original_plan._remote_plan_fmf_id + else: + fmf_id = self._remote_plan_fmf_id + + echo(tmt.utils.format('import', '', key_color='blue')) + assert fmf_id is not None # narrow type + for key, value in fmf_id.items(): + echo(tmt.utils.format(key, value, key_color='green')) def _lint_execute(self) -> Optional[bool]: """ Lint execute step """ @@ -1469,27 +1486,48 @@ def export(self, *, format_: ExportFormat = ExportFormat.YAML, @property def is_remote_plan_reference(self) -> bool: - return self._import_plan_node is not None + """ Check whether the plan is a remote plan reference """ + return self._remote_plan_fmf_id is not None - @property - def imported_plan(self) -> Optional['Plan']: + def import_plan(self) -> Optional['Plan']: + """ Import plan from a remote repository, return a Plan instance """ if not self.is_remote_plan_reference: return None + if not self._imported_plan: - if self.parent: # check whether the run obj is set - plan_id = tmt.base.FmfId(**self._import_plan_node) + assert self._remote_plan_fmf_id is not None # narrow type + plan_id = self._remote_plan_fmf_id + self.debug(f"Import remote plan '{plan_id.name}' from '{plan_id.url}'.", level=3) + + # Clone the whole git repository if executing tests (run is attached) + if self.my_run: + assert self.parent is not None # narrow type + assert self.parent.workdir is not None # narrow type destination = os.path.join(self.parent.workdir, "import", self.name.lstrip("/")) - tmt.utils.git_clone(plan_id.url, destination, self) + if plan_id.url is None: + raise tmt.utils.SpecificationError( + f"No url provided for remote plan '{self.name}'.") + if os.path.exists(destination): + self.debug(f"Seems that '{destination}' has been already cloned.", level=3) + else: + tmt.utils.git_clone(plan_id.url, destination, self) if plan_id.ref: self.run(['git', 'checkout', plan_id.ref], cwd=destination) if plan_id.path: - destination = os.path.join(destination, plan_id.path) - node = fmf.Tree(destination).find(self._import_plan_node['name']) + destination = os.path.join(destination, plan_id.path.lstrip("/")) + node = fmf.Tree(destination).find(plan_id.name) + + # Use fmf cache for exploring plans (the whole git repo is not needed) else: - node = fmf.Tree.node(self._import_plan_node) - self._imported_plan = Plan(node=node, run=self.parent, logger=self) - self._imported_plan.name = self.name - self._imported_plan._origin_plan = self + node = fmf.Tree.node( + {key: value for key, value in plan_id.items() if value is not None}) + + # Override the plan name with the local one to ensure unique names + node.name = self.name + # Create the plan object, save links between both plans + self._imported_plan = Plan(node=node, run=self.my_run, logger=self) + self._imported_plan._original_plan = self + return self._imported_plan @@ -1974,7 +2012,7 @@ def plans( if Plan._opt('shallow'): return plans else: - return [plan.imported_plan or plan for plan in plans] + return [plan.import_plan() or plan for plan in plans] def stories( self, @@ -2125,7 +2163,7 @@ def _use_default_plan(self) -> None: """ Prepare metadata tree with only the default plan """ default_plan = tmt.utils.yaml_to_dict(tmt.templates.DEFAULT_PLAN) # The default discover method for this case is 'shell' - default_plan['/plans/default']['discover']['how'] = 'shell' + default_plan[tmt.templates.DEFAULT_PLAN_NAME]['discover']['how'] = 'shell' self.tree = tmt.Tree(tree=fmf.Tree(default_plan)) self.debug("No metadata found, using the default plan.") @@ -2138,11 +2176,16 @@ def _save_tree(self, tree: Optional[Tree]) -> None: # Clear the tree and insert default plan if requested if Plan._opt("default"): new_tree = fmf.Tree(default_plan) - new_tree.root = self.tree.root + # Make sure the fmf root is set for the default plan + new_tree.find(tmt.templates.DEFAULT_PLAN_NAME).root = self.tree.root self.tree.tree = new_tree - self.debug("Enforcing use of default plan") - # Insert default plan if no plan detected - if not list(self.tree.tree.prune(keys=['execute'])): + self.debug("Enforcing use of the default plan.") + # Insert default plan if no plan detected. Check using + # tree.prune() instead of self.tree.plans() to prevent + # creating plan objects which leads to wrong expansion of + # environment variables from the command line. + if not (list(self.tree.tree.prune(keys=['execute'])) or + list(self.tree.tree.prune(keys=['plan']))): self.tree.tree.update(default_plan) self.debug("No plan found, adding the default plan.") # Create an empty default plan if no fmf metadata found diff --git a/tmt/steps/discover/fmf.py b/tmt/steps/discover/fmf.py index f200426987..59ca27c8e4 100644 --- a/tmt/steps/discover/fmf.py +++ b/tmt/steps/discover/fmf.py @@ -298,9 +298,7 @@ def assert_git_url(plan_name: Optional[str] = None) -> None: if path is not None: fmf_root: Optional[str] = path else: - assert self.step.plan.my_run is not None # narrow type - assert self.step.plan.my_run.tree is not None # narrow type - fmf_root = self.step.plan.my_run.tree.root + fmf_root = self.step.plan.node.root requires_git = self.opt('sync-repo') or any( self.get(opt) for opt in self._REQUIRES_GIT) # Path for distgit sources cannot be checked until the @@ -311,15 +309,12 @@ def assert_git_url(plan_name: Optional[str] = None) -> None: if dist_git_source: # Ensure we're in a git repo when extracting dist-git sources try: - assert self.step.plan.my_run is not None # narrow type - assert self.step.plan.my_run.tree is not None # narrow type - assert self.step.plan.my_run.tree.root is not None # narrow type - git_root = get_git_root(self.step.plan.my_run.tree.root) + git_root = get_git_root(self.step.plan.node.root) except tmt.utils.RunError: assert self.step.plan.my_run is not None # narrow type assert self.step.plan.my_run.tree is not None # narrow type raise tmt.utils.DiscoverError( - f"{self.step.plan.my_run.tree.root} is not a git repo") + f"{self.step.plan.node.root} is not a git repo") else: if fmf_root is None: raise tmt.utils.DiscoverError( diff --git a/tmt/templates.py b/tmt/templates.py index 05cc6d05ea..f6c5b7e3d9 100644 --- a/tmt/templates.py +++ b/tmt/templates.py @@ -61,8 +61,9 @@ # Plan Templates # ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -DEFAULT_PLAN = """ -/plans/default: +DEFAULT_PLAN_NAME = "/plans/default" +DEFAULT_PLAN = f""" +{DEFAULT_PLAN_NAME}: discover: how: fmf execute: