Migrate package tests to the main test suite

.github/workflows/test-packages-action-linux.yml

@@ -134,7 +134,7 @@ jobs:
         uses: actions/download-artifact@v3
         with:
           name: ${{ inputs.package-name }}-${{ inputs.salt-version }}-${{ inputs.arch }}-${{ inputs.pkg-type }}
-          path: pkg/artifacts/
+          path: artifacts/pkg/
 
       - name: Download Onedir Tarball as an Artifact
         uses: actions/download-artifact@v3
@@ -151,7 +151,7 @@ jobs:
 
       - name: List Packages
         run: |
-          tree pkg/artifacts
+          tree artifacts/pkg/
 
       - name: Download cached nox.linux.${{ inputs.arch }}.tar.* for session ${{ inputs.nox-session }}
         uses: actions/cache@v3.3.1

.github/workflows/test-packages-action-macos.yml

@@ -123,7 +123,7 @@ jobs:
         uses: actions/download-artifact@v3
         with:
           name: salt-${{ inputs.salt-version }}-${{ inputs.arch }}-${{ inputs.pkg-type }}
-          path: pkg/artifacts/
+          path: artifacts/pkg/
 
       - name: Install System Dependencies
         run: |
@@ -131,7 +131,7 @@ jobs:
 
       - name: List Packages
         run: |
-          tree pkg/artifacts
+          tree artifacts/pkg/
 
       - name: Download Onedir Tarball as an Artifact
         uses: actions/download-artifact@v3

.gitignore

@@ -117,7 +117,6 @@ kitchen.local.yml
 .bundle/
 Gemfile.lock
 /artifacts/
-/pkg/artifacts/
 requirements/static/*/py*/*.log
 
 # Vim's default session file

doc/topics/packaging/testing.rst

@@ -27,7 +27,7 @@ In order to run the package tests, the `relenv
 built packages need to be placed in the correct locations.
 
 * Place all salt packages for the applicable testing version in
-  ``<repo-root>/pkg/artifacts/``.
+  ``<repo-root>/artifacts/pkg/``.
 * The onedir must be located under ``<repo-root>/artifacts/``.
 * Additionally, to ensure complete parity with Salt's CI/CD suite, place the
   ``nox`` virtual environment in ``<repo-root>/.nox/test-pkgs-onedir``.
@@ -109,7 +109,7 @@ can be performed for artifacts from nightly builds.
 
 #. Place the artifacts in the correct location:
 
-    Unzip the packages and place them in ``<repo-root>/pkg/artifacts/``.
+    Unzip the packages and place them in ``<repo-root>/artifacts/pkg/``.
 
     You must unzip and untar the onedir packages and place them in
     ``<repo-root>/artifacts/``. Windows onedir requires an additional unzip

noxfile.py

@@ -463,7 +463,7 @@ def _report_coverage(
         xml_coverage_file = COVERAGE_OUTPUT_DIR.relative_to(REPO_ROOT) / "salt.xml"
         html_coverage_dir = COVERAGE_OUTPUT_DIR.relative_to(REPO_ROOT) / "html" / "salt"
         cmd_args = [
-            "--omit=tests/*,pkg/tests/*",
+            "--omit=tests/*,tests/pytests/pkg/*",
             "--include=salt/*",
         ]
 
@@ -475,7 +475,7 @@ def _report_coverage(
         )
         cmd_args = [
             "--omit=salt/*",
-            "--include=tests/*,pkg/tests/*",
+            "--include=tests/*,tests/pytests/pkg/*",
         ]
     else:
         json_coverage_file = (
@@ -484,7 +484,7 @@ def _report_coverage(
         xml_coverage_file = COVERAGE_OUTPUT_DIR.relative_to(REPO_ROOT) / "coverage.xml"
         html_coverage_dir = COVERAGE_OUTPUT_DIR.relative_to(REPO_ROOT) / "html" / "full"
         cmd_args = [
-            "--include=salt/*,tests/*,pkg/tests/*",
+            "--include=salt/*,tests/*,tests/pytests/pkg/*",
         ]
 
     if cli_report:
@@ -1837,30 +1837,30 @@ def ci_test_onedir_pkgs(session):
         )
 
     chunks = {
-        "install": ["pkg/tests/"],
+        "install": ["tests/pytests/pkg/"],
         "upgrade": [
             "--upgrade",
             "--no-uninstall",
-            "pkg/tests/upgrade/",
+            "tests/pytests/pkg/upgrade/",
         ],
         "upgrade-classic": [
             "--upgrade",
             "--no-uninstall",
-            "pkg/tests/upgrade/",
+            "tests/pytests/pkg/upgrade/",
         ],
         "downgrade": [
             "--downgrade",
             "--no-uninstall",
-            "pkg/tests/downgrade/",
+            "tests/pytests/pkg/downgrade/",
         ],
         "downgrade-classic": [
             "--downgrade",
             "--no-uninstall",
-            "pkg/tests/downgrade/",
+            "tests/pytests/pkg/downgrade/",
         ],
         "download-pkgs": [
             "--download-pkgs",
-            "pkg/tests/download/",
+            "tests/pytests/pkg/download/",
         ],
     }
 

tests/pytests/pkg/conftest.py

@@ -12,7 +12,7 @@ from saltfactories.utils import random_string
 from saltfactories.utils.tempfiles import SaltPillarTree, SaltStateTree
 
 import salt.config
-from tests.support.helpers import (
+from tests.pytests.pkg.support.helpers import (
     CODE_DIR,
     TESTS_DIR,
     ApiRequest,
@@ -209,7 +209,7 @@ def state_tree():
     envs = {
         "base": [
             str(file_root),
-            str(TESTS_DIR / "files"),
+            str(TESTS_DIR / "pytests" / "pkg" / "files"),
         ],
     }
     tree = SaltStateTree(envs=envs)
@@ -345,7 +345,7 @@ def salt_master(salt_factories, install_salt, state_tree, pillar_tree):
     test_user = False
     master_config = install_salt.config_path / "master"
     if master_config.exists():
-        with open(master_config) as fp:
+        with salt.utils.files.fopen(master_config) as fp:
             data = yaml.safe_load(fp)
             if data and "user" in data:
                 test_user = True
@@ -439,15 +439,18 @@ def salt_master(salt_factories, install_salt, state_tree, pillar_tree):
                 "-R",
                 "salt:salt",
                 str(pathlib.Path("/etc", "salt", "pki", "master")),
-            ]
+            ],
+            check=True,
         )
         # The engines_dirs is created in .nox path. We need to set correct perms
         # for the user running the Salt Master
-        subprocess.run(["chown", "-R", "salt:salt", str(CODE_DIR.parent / ".nox")])
+        subprocess.run(
+            ["chown", "-R", "salt:salt", str(CODE_DIR.parent / ".nox")], check=False
+        )
         file_roots = pathlib.Path("/srv/", "salt")
         pillar_roots = pathlib.Path("/srv/", "pillar")
         for _dir in [file_roots, pillar_roots]:
-            subprocess.run(["chown", "-R", "salt:salt", str(_dir)])
+            subprocess.run(["chown", "-R", "salt:salt", str(_dir)], check=False)
 
     with factory.started(start_timeout=start_timeout):
         yield factory
@@ -504,10 +507,10 @@ def salt_minion(salt_factories, salt_master, install_salt):
         file_roots = pathlib.Path("/srv/", "salt")
         pillar_roots = pathlib.Path("/srv/", "pillar")
         for _dir in [file_roots, pillar_roots]:
-            subprocess.run(["chown", "-R", "salt:salt", str(_dir)])
+            subprocess.run(["chown", "-R", "salt:salt", str(_dir)], check=True)
 
     factory.after_terminate(
-        pytest.helpers.remove_stale_minion_key, salt_master, factory.id
+        pytest.helpers.remove_stale_minion_key_pkg, salt_master, factory.id
     )
     with factory.started(start_timeout=start_timeout):
         yield factory

tests/pytests/pkg/downgrade/test_salt_downgrade.py

@@ -1,5 +1,3 @@
-import shutil
-
 import packaging.version
 import pytest
 from pytestskipmarkers.utils import platform

tests/pytests/pkg/download/test_pkg_download.py

@@ -267,7 +267,7 @@ def setup_redhat_family(
 
     try:
         pytest.helpers.download_file(gpg_file_url, downloads_path / gpg_key_name)
-    except Exception as exc:
+    except Exception as exc:  # pylint: disable=broad-except
         pytest.fail(f"Failed to download {gpg_file_url}: {exc}")
 
     ret = shell.run("rpm", "--import", str(downloads_path / gpg_key_name), check=False)
@@ -333,7 +333,7 @@ def setup_debian_family(
 
         try:
             pytest.helpers.download_file(gpg_file_url, downloads_path / gpg_key_name)
-        except Exception as exc:
+        except Exception as exc:  # pylint: disable=broad-except
             pytest.fail(f"Failed to download {gpg_file_url}: {exc}")
 
         salt_sources_path = downloads_path / "salt.list"
@@ -384,7 +384,7 @@ def setup_debian_family(
 
         try:
             pytest.helpers.download_file(onedir_url, onedir_location)
-        except Exception as exc:
+        except Exception as exc:  # pylint: disable=broad-except
             pytest.fail(f"Failed to download {onedir_url}: {exc}")
 
         shell.run("tar", "xvf", str(onedir_location), "-C", str(onedir_extracted))
@@ -439,7 +439,7 @@ def setup_macos(
 
         try:
             pytest.helpers.download_file(onedir_url, onedir_location)
-        except Exception as exc:
+        except Exception as exc:  # pylint: disable=broad-except
             pytest.fail(f"Failed to download {onedir_url}: {exc}")
 
         shell.run("tar", "xvf", str(onedir_location), "-C", str(onedir_extracted))
@@ -516,7 +516,7 @@ def setup_windows(
 
             try:
                 pytest.helpers.download_file(onedir_url, onedir_location)
-            except Exception as exc:
+            except Exception as exc:  # pylint: disable=broad-except
                 pytest.fail(f"Failed to download {onedir_url}: {exc}")
 
             shell.run("unzip", str(onedir_location), "-d", str(onedir_extracted))

tests/pytests/pkg/integration/test_enabled_disabled.py

@@ -1,6 +1,5 @@
 import pytest
 from pytestskipmarkers.utils import platform
-from saltfactories.utils.functional import MultiStateResult
 
 
 @pytest.mark.skip_on_windows(reason="Linux test only")

tests/pytests/pkg/integration/test_help.py

@@ -1,7 +1,5 @@
 import subprocess
 
-from pytestskipmarkers.utils import platform
-
 
 def test_help(install_salt):
     """

tests/pytests/pkg/integration/test_python.py

@@ -2,7 +2,7 @@ import subprocess
 
 import pytest
 
-from tests.support.helpers import TESTS_DIR
+from tests.pytests.pkg.support.helpers import TESTS_DIR
 
 
 @pytest.fixture
@@ -16,7 +16,13 @@ def python_script_bin(install_salt):
 @pytest.mark.parametrize("exp_ret,user_arg", [(1, "false"), (0, "true")])
 def test_python_script(install_salt, exp_ret, user_arg, python_script_bin):
     ret = install_salt.proc.run(
-        *(python_script_bin + [str(TESTS_DIR / "files" / "check_python.py"), user_arg]),
+        *(
+            python_script_bin
+            + [
+                str(TESTS_DIR / "pytests" / "pkg" / "files" / "check_python.py"),
+                user_arg,
+            ]
+        ),
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
         check=False,
@@ -28,7 +34,13 @@ def test_python_script(install_salt, exp_ret, user_arg, python_script_bin):
 
 def test_python_script_exception(install_salt, python_script_bin):
     ret = install_salt.proc.run(
-        *(python_script_bin + [str(TESTS_DIR / "files" / "check_python.py"), "raise"]),
+        *(
+            python_script_bin
+            + [
+                str(TESTS_DIR / "pytests" / "pkg" / "files" / "check_python.py"),
+                "raise",
+            ]
+        ),
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
         check=False,

tests/pytests/pkg/integration/test_salt_user.py

@@ -90,7 +90,7 @@ def test_salt_user_home(install_salt):
     home = ""
     try:
         home = proc.stdout.decode().split(":")[5]
-    except:
+    except Exception:  # pylint: disable=broad-except
         pass
     assert home == "/opt/saltstack/salt"
 
@@ -106,7 +106,7 @@ def test_salt_user_group(install_salt):
         for group in proc.stdout.decode().split(" "):
             if "salt" in group:
                 in_group = True
-    except:
+    except Exception:  # pylint: disable=broad-except
         pass
     assert in_group is True
 
@@ -124,7 +124,7 @@ def test_salt_user_shell(install_salt):
     try:
         shell = proc.stdout.decode().split(":")[6].strip()
         shell_exists = pathlib.Path(shell).exists()
-    except:
+    except Exception:  # pylint: disable=broad-except
         pass
     assert shell_exists is True
 

tests/pytests/pkg/integration/test_version.py

@@ -32,6 +32,7 @@ def test_salt_versions_report_master(install_salt):
     ret.stdout.matcher.fnmatch_lines(["*Salt Version:*"])
     py_version = subprocess.run(
         [str(python_bin), "--version"],
+        check=True,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
     ).stdout

tests/pytests/pkg/support/helpers.py

@@ -16,6 +16,7 @@ import packaging
 import psutil
 import pytest
 import requests
+import saltfactories.cli
 from pytestshellutils.shell import DaemonImpl, Subprocess
 from pytestshellutils.utils.processes import (
     ProcessResult,
@@ -24,10 +25,12 @@ from pytestshellutils.utils.processes import (
 )
 from pytestskipmarkers.utils import platform
 from saltfactories.bases import SystemdSaltDaemonImpl
-from saltfactories.cli import call, key, salt
+from saltfactories.cli import call, key
 from saltfactories.daemons import api, master, minion
 from saltfactories.utils import cli_scripts
 
+import salt.utils.files
+
 try:
     import crypt
 
@@ -41,9 +44,9 @@ try:
 except ImportError:
     HAS_PWD = False
 
-TESTS_DIR = pathlib.Path(__file__).resolve().parent.parent
+TESTS_DIR = pathlib.Path(__file__).resolve().parent.parent.parent.parent
 CODE_DIR = TESTS_DIR.parent
-ARTIFACTS_DIR = CODE_DIR / "artifacts"
+ARTIFACTS_DIR = CODE_DIR / "artifacts" / "pkg"
 
 log = logging.getLogger(__name__)
 
@@ -420,7 +423,7 @@ class SaltPkgInstall:
                 # expects unless we do it via a batch file
                 batch_file = pathlib.Path(pkg).parent / "install_msi.cmd"
                 batch_content = f'msiexec /qn /i "{str(pkg)}" START_MINION=""\n'
-                with open(batch_file, "w") as fp:
+                with salt.utils.files.fopen(batch_file, "w") as fp:
                     fp.write(batch_content)
                 # Now run the batch file
                 ret = self.proc.run("cmd.exe", "/c", str(batch_file))
@@ -618,7 +621,7 @@ class SaltPkgInstall:
                 f"https://repo.saltproject.io/{root_url}{distro_name}/{self.distro_version}/{arch}/{major_ver}/{gpg_key}",
                 f"/etc/apt/keyrings/{gpg_dest}",
             )
-            with open(
+            with salt.utils.files.fopen(
                 pathlib.Path("/etc", "apt", "sources.list.d", "salt.list"), "w"
             ) as fp:
                 fp.write(
@@ -665,8 +668,8 @@ class SaltPkgInstall:
             # Let's not check the returncode if this is the case
             if not (
                 downgrade
-                and not packaging.version.parse(self.prev_version)
-                >= packaging.version.parse("3006.0")
+                and packaging.version.parse(self.prev_version)
+                < packaging.version.parse("3006.0")
             ):
                 self._check_retcode(ret)
             if downgrade:
@@ -710,7 +713,7 @@ class SaltPkgInstall:
                 # expects unless we do it via a batch file
                 batch_file = pkg_path.parent / "install_msi.cmd"
                 batch_content = f'msiexec /qn /i {str(pkg_path)} START_MINION=""'
-                with open(batch_file, "w") as fp:
+                with salt.utils.files.fopen(batch_file, "w") as fp:
                     fp.write(batch_content)
                 # Now run the batch file
                 ret = self.proc.run("cmd.exe", "/c", str(batch_file))
@@ -814,40 +817,6 @@ class SaltPkgInstall:
             ret = self.proc.run(self.pkg_mngr, self.rm_pkg, "-y", *self.salt_pkgs)
             self._check_retcode(ret)
 
-    def assert_uninstalled(self):
-        """
-        Assert that the paths in /opt/saltstack/ were correctly
-        removed or not removed
-        """
-        return
-        if platform.is_windows():
-            # I'm not sure where the /opt/saltstack path is coming from
-            # This is the path we're using to test windows
-            opt_path = pathlib.Path(os.getenv("LocalAppData"), "salt", "pypath")
-        else:
-            opt_path = pathlib.Path(os.sep, "opt", "saltstack", "salt", "pypath")
-        if not opt_path.exists():
-            if platform.is_windows():
-                assert not opt_path.parent.exists()
-            else:
-                assert not opt_path.parent.parent.exists()
-        else:
-            opt_path_contents = list(opt_path.rglob("*"))
-            if not opt_path_contents:
-                pytest.fail(
-                    f"The path '{opt_path}' exists but there are no files in it."
-                )
-            else:
-                for path in list(opt_path_contents):
-                    if path.name in (".installs.json", "__pycache__"):
-                        opt_path_contents.remove(path)
-                if opt_path_contents:
-                    pytest.fail(
-                        "The test left some files behind: {}".format(
-                            ", ".join([str(p) for p in opt_path_contents])
-                        )
-                    )
-
     def write_launchd_conf(self, service):
         service_name = f"com.saltstack.salt.{service}"
         ret = self.proc.run("launchctl", "list", service_name)
@@ -953,7 +922,6 @@ class SaltPkgInstall:
     def __exit__(self, *_):
         if not self.no_uninstall:
             self.uninstall()
-            self.assert_uninstalled()
 
 
 class PkgSystemdSaltDaemonImpl(SystemdSaltDaemonImpl):
@@ -1335,17 +1303,21 @@ class SaltMaster(DaemonPkgMixin, master.SaltMaster):
             factory_class=SaltApi, salt_pkg_install=self.salt_pkg_install, **kwargs
         )
 
-    def salt_key_cli(self, **factory_class_kwargs):
+    def salt_key_cli(self, factory_class=None, **factory_class_kwargs):
+        if not factory_class:
+            factory_class = SaltKey
+        factory_class_kwargs["salt_pkg_install"] = self.salt_pkg_install
         return super().salt_key_cli(
-            factory_class=SaltKey,
-            salt_pkg_install=self.salt_pkg_install,
+            factory_class=factory_class,
             **factory_class_kwargs,
         )
 
-    def salt_cli(self, **factory_class_kwargs):
+    def salt_cli(self, factory_class=None, **factory_class_kwargs):
+        if not factory_class:
+            factory_class = SaltCli
+        factory_class_kwargs["salt_pkg_install"] = self.salt_pkg_install
         return super().salt_cli(
-            factory_class=SaltCli,
-            salt_pkg_install=self.salt_pkg_install,
+            factory_class=factory_class,
             **factory_class_kwargs,
         )
 
@@ -1403,10 +1375,12 @@ class SaltMinion(DaemonPkgMixin, minion.SaltMinion):
             "salt-minion", self.salt_pkg_install.binary_paths["minion"]
         )
 
-    def salt_call_cli(self, **factory_class_kwargs):
+    def salt_call_cli(self, factory_class=None, **factory_class_kwargs):
+        if not factory_class:
+            factory_class = SaltCall
+        factory_class_kwargs["salt_pkg_install"] = self.salt_pkg_install
         return super().salt_call_cli(
-            factory_class=SaltCall,
-            salt_pkg_install=self.salt_pkg_install,
+            factory_class=factory_class,
             **factory_class_kwargs,
         )
 
@@ -1453,14 +1427,14 @@ class SaltCall(PkgMixin, call.SaltCall):
 
 
 @attr.s(kw_only=True, slots=True)
-class SaltCli(PkgMixin, salt.SaltCli):
+class SaltCli(PkgMixin, saltfactories.cli.salt.SaltCli):
     """
     Subclassed just to tweak the binary paths if needed.
     """
 
     def __attrs_post_init__(self):
         self.script_name = "salt"
-        salt.SaltCli.__attrs_post_init__(self)
+        saltfactories.cli.salt.SaltCli.__attrs_post_init__(self)
 
 
 @attr.s(kw_only=True, slots=True)
@@ -1586,7 +1560,7 @@ class ApiRequest:
 
 
 @pytest.helpers.register
-def remove_stale_minion_key(master, minion_id):
+def remove_stale_minion_key_pkg(master, minion_id):
     key_path = os.path.join(master.config["pki_dir"], "minions", minion_id)
     if os.path.exists(key_path):
         os.unlink(key_path)
@@ -1624,7 +1598,7 @@ def download_file(url, dest, auth=None):
     # NOTE the stream=True parameter below
     with requests.get(url, stream=True, auth=auth) as r:
         r.raise_for_status()
-        with open(dest, "wb") as f:
+        with salt.utils.files.fopen(dest, "wb") as f:
             for chunk in r.iter_content(chunk_size=8192):
                 if chunk:
                     f.write(chunk)

tools/vm.py

@@ -1315,7 +1315,7 @@ class VM:
             "--include",
             "artifacts/salt",
             "--include",
-            "pkg/artifacts/*",
+            "artifacts/pkg/*",
             # But we also want to exclude all other entries under artifacts/
             "--exclude",
             "artifacts/*",