diff --git a/samcli/cli/hidden_imports.py b/samcli/cli/hidden_imports.py index f247a34b4ae..e1afcc214e0 100644 --- a/samcli/cli/hidden_imports.py +++ b/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", diff --git a/tests/integration/logs/test_logs_command.py b/tests/integration/logs/test_logs_command.py index a56b884b8ee..941288080ae 100644 --- a/tests/integration/logs/test_logs_command.py +++ b/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 = "" diff --git a/tests/integration/sync/test_sync_code.py b/tests/integration/sync/test_sync_code.py index fd6876ecd85..85b36314d86 100644 --- a/tests/integration/sync/test_sync_code.py +++ b/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 = "" diff --git a/tests/integration/traces/test_traces_command.py b/tests/integration/traces/test_traces_command.py index a59528ee521..0ab84eed6a3 100644 --- a/tests/integration/traces/test_traces_command.py +++ b/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] = [] diff --git a/tests/unit/cli/test_pyinstaller_imports.py b/tests/unit/cli/test_pyinstaller_imports.py index c01a11753ac..16d69ec8129 100644 --- a/tests/unit/cli/test_pyinstaller_imports.py +++ b/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") diff --git a/tests/unit/local/docker/test_lambda_container.py b/tests/unit/local/docker/test_lambda_container.py index 2e50ec7d73c..b5aa907e36b 100644 --- a/tests/unit/local/docker/test_lambda_container.py +++ b/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