tests: unblock nightly integ tests on pytest 9.1 and make collection deterministic

samcli/cli/hidden_imports.py

@@ -4,24 +4,44 @@
 
 import pkgutil
 from types import ModuleType
+from typing import List, Optional, Set
 
 
-def walk_modules(module: ModuleType, visited: set) -> None:
-    """Recursively find all modules from a parent module"""
+def walk_modules(module: ModuleType, visited: List[str], seen: Optional[Set[str]] = None) -> None:
+    """Recursively find all modules from a parent module.
+
+    `visited` keeps discovery order, which callers rely on: it is what pyinstaller
+    bundles, and an unstable order both makes builds non-reproducible and makes the
+    parameterized test over it collect differently in each pytest-xdist worker.
+
+    `seen` is a parallel set used only for the dedup check, so ordering does not cost
+    O(n) lookups. That matters because `__import__("samcli.cli")` returns the top-level
+    `samcli`, so each recursive call re-walks the whole tree from the root -- 108k
+    membership checks for 658 modules. Optional so existing two-argument callers work.
+    """
+    if seen is None:
+        seen = set(visited)
     for pkg in pkgutil.walk_packages(module.__path__, module.__name__ + "."):
-        if pkg.name in visited:
+        if pkg.name in seen:
             continue
-        visited.add(pkg.name)
+        seen.add(pkg.name)
+        visited.append(pkg.name)
         if pkg.ispkg:
             submodule = __import__(pkg.name)
-            walk_modules(submodule, visited)
+            walk_modules(submodule, visited, seen)
 
 
-samcli_modules = set(["samcli"])
+samcli_modules = ["samcli"]
 samcli = __import__("samcli")
 walk_modules(samcli, samcli_modules)
 
-SAM_CLI_HIDDEN_IMPORTS = list(samcli_modules) + [
+# Collected in discovery order. This used to be list(set(...)), whose order varied per
+# process because string hashing is randomized -- that made pyinstaller's bundle list
+# unstable between builds, and made the parameterized test over it collect differently in
+# each pytest-xdist worker ("Different tests were collected between workers"). Walking
+# into a list is deterministic on its own, so no sort is needed here; the ordering is
+# pinned by test_walk_modules_order_is_deterministic_and_sorted.
+SAM_CLI_HIDDEN_IMPORTS = samcli_modules + [
     "cookiecutter.extensions",
     "text_unidecode",
     "samtranslator",

tests/integration/logs/test_logs_command.py

@@ -26,6 +26,18 @@
 APIGW_REQUESTS_TO_WARM_UP = 20
 
 
+# pytest 9.1 deprecated class-scoped fixtures declared as instance methods, because
+# attributes they set on `self` are invisible to tests (each test gets a fresh instance
+# while the fixture runs once per class). The fixtures below assign to the class instead,
+# so the hazard the warning exists to catch does not apply. Marked on the class rather
+# than in pytest.ini so the rest of the suite still fails on this warning; the two
+# subclasses below inherit it.
+#
+# Without this, `filterwarnings = error` turns the warning into a setup failure on the
+# first test of the class, and pytest then raises its own internal AssertionError
+# ("assert not self._finalizers") for the remaining tests (pytest-dev/pytest#14775).
+# Remove once these fixtures are converted -- required before pytest 10 drops this.
+@pytest.mark.filterwarnings("ignore:.*Class-scoped fixture defined as instance method.*")
 class LogsIntegTestCases(LogsIntegBase):
     test_template_folder = ""
 

tests/integration/sync/test_sync_code.py

@@ -39,6 +39,20 @@
 LOG = logging.getLogger(__name__)
 
 
+# pytest 9.1 deprecated class-scoped fixtures declared as instance methods, because
+# attributes they set on `self` are invisible to tests (each test gets a fresh instance
+# while the fixture runs once per class). The fixtures below assign to the class instead,
+# so the hazard the warning exists to catch does not apply. Marked on the class rather
+# than in pytest.ini so the rest of the suite still fails on it, and rather than with a
+# module-level `pytestmark` because subclasses live in other modules (test_sync_adl.py,
+# test_sync_build_in_source.py) and a module mark would not reach them -- a class mark is
+# inherited.
+#
+# Without this, `filterwarnings = error` turns the warning into a setup failure on the
+# first test of the class, and pytest then raises its own internal AssertionError
+# ("assert not self._finalizers") for the remaining tests (pytest-dev/pytest#14775).
+# Remove once these fixtures are converted -- required before pytest 10 drops this.
+@pytest.mark.filterwarnings("ignore:.*Class-scoped fixture defined as instance method.*")
 class TestSyncCodeBase(SyncIntegBase):
     stack_name = ""
     template_path = ""

tests/integration/traces/test_traces_command.py

@@ -30,6 +30,17 @@
 
 
 @skipIf(SKIP_TRACES_TESTS, "Skip traces tests in CI/CD only")
+# pytest 9.1 deprecated class-scoped fixtures declared as instance methods, because
+# attributes they set on `self` are invisible to tests (each test gets a fresh instance
+# while the fixture runs once per class). The fixtures below assign to the class instead,
+# so the hazard the warning exists to catch does not apply. Marked on the class rather
+# than in pytest.ini so the rest of the suite still fails on this warning.
+#
+# Without this, `filterwarnings = error` turns the warning into a setup failure on the
+# first test of the class, and pytest then raises its own internal AssertionError
+# ("assert not self._finalizers") for the remaining tests (pytest-dev/pytest#14775).
+# Remove once these fixtures are converted -- required before pytest 10 drops this.
+@pytest.mark.filterwarnings("ignore:.*Class-scoped fixture defined as instance method.*")
 @pytest.mark.xdist_group(name="sam_traces")
 class TestTracesCommand(TracesIntegBase):
     stack_resources: List[Any] = []

tests/unit/cli/test_pyinstaller_imports.py

@@ -48,16 +48,59 @@ def tearDown(self):
 
     def test_walk_modules_contains_all_modules(self):
         my_test_module = __import__("my_test_module")
-        modules = set(["my_test_module"])
+        modules = ["my_test_module"]
         hidden_imports.walk_modules(my_test_module, modules)
         self.assertIn("my_test_module", modules)
         self.assertIn("my_test_module.my_submodule", modules)
         self.assertIn("my_test_module.another_submodule", modules)
         del sys.modules["my_test_module"]
 
+    def test_walk_modules_does_not_add_duplicates(self):
+        """walk_modules dedups via `if pkg.name in visited`, which is why it can collect
+        into a list -- the set it used to take was redundant."""
+        my_test_module = __import__("my_test_module")
+        modules = ["my_test_module"]
+        hidden_imports.walk_modules(my_test_module, modules)
+        hidden_imports.walk_modules(my_test_module, modules)
+        self.assertEqual(len(modules), len(set(modules)))
+        del sys.modules["my_test_module"]
+
     def test_walk_modules_not_contain_nonexistent_module(self):
         my_test_module = __import__("my_test_module")
-        modules = set("my_test_module")
+        # Was `set("my_test_module")`, which built a set of individual characters rather
+        # than a one-element collection holding the module name.
+        modules = ["my_test_module"]
         hidden_imports.walk_modules(my_test_module, modules)
         self.assertNotIn("my_non_existant_module", modules)
         del sys.modules["my_test_module"]
+
+    def test_walk_modules_order_is_deterministic_and_sorted(self):
+        """SAM_CLI_HIDDEN_IMPORTS relies on walk_modules being order-stable.
+
+        It decides what pyinstaller bundles, so an unstable order makes builds
+        non-reproducible; it is also the parameter source for the tests above, and
+        pytest-xdist aborts the run if workers collect in different orders. That used
+        to happen because the modules were collected into a set.
+
+        Collecting into a list is deterministic, and sorted because
+        `pkgutil.walk_packages` walks children in sorted order
+        (`_iter_file_finder_modules` calls `os.listdir(...).sort()`). That sort is
+        per-importer rather than a language guarantee, so assert it here instead of
+        re-sorting at import time.
+        """
+        my_test_module = __import__("my_test_module")
+        first = ["my_test_module"]
+        hidden_imports.walk_modules(my_test_module, first)
+        second = ["my_test_module"]
+        hidden_imports.walk_modules(my_test_module, second)
+
+        self.assertEqual(first, second, "walk_modules returned a different order for the same tree")
+        self.assertEqual(first, sorted(first), f"walk_modules order is not sorted: {first}")
+        del sys.modules["my_test_module"]
+
+    def test_hidden_imports_discovered_modules_are_sorted(self):
+        """The real samcli walk, not the fixture tree -- this is what gets bundled."""
+        discovered = hidden_imports.samcli_modules
+
+        self.assertEqual(discovered, sorted(discovered))
+        self.assertEqual(len(discovered), len(set(discovered)), "duplicate modules collected")

tests/unit/local/docker/test_lambda_container.py

@@ -678,7 +678,11 @@ def test_must_provide_container_env_vars(self, runtime):
 
         self.assertIsNotNone(container_env_vars)
 
-    @parameterized.expand([param(r) for r in set(RUNTIMES_WITH_BOOTSTRAP_ENTRYPOINT)])
+    # Iterate the list directly rather than a set: set iteration order for strings varies
+    # between processes (hash randomization), so each pytest-xdist worker generated the
+    # parameterized cases in a different order and the run aborted with "Different tests
+    # were collected between workers". The list has no duplicates, so the set was a no-op.
+    @parameterized.expand([param(r) for r in RUNTIMES_WITH_BOOTSTRAP_ENTRYPOINT])
     def test_debug_arg_must_be_split_by_spaces_and_appended_to_bootstrap_based_entrypoint(self, runtime):
         """
         Debug args list is appended as arguments to bootstrap-args, which is past the fourth position in the array