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

[roger-zhangg]

Which issue(s) does this change fix?

N/A

Why is this change necessary?

Nightly integration tests have failed in sync-code and sync-watch on every run since pytest was bumped 9.0.3 → 9.1.1 (#9095, 2026-07-31).

Run	pytest	sync-code
2026-07-30 (last pre-bump)	9.0.3	success — 89 passed, 1 rerun
2026-08-14	9.1.1	failure — 37 passed, 52 errors, 156 rerun

The logs show only pytest's own internal assertion, assert not self._finalizers (_pytest/fixtures.py:1221), with no indication of the real cause. Reproducible in ten lines, no AWS needed:

class TestFixt:
    @pytest.fixture(scope="class")
    def fixt(self): yield
    def test_1(self, fixt): pass
    def test_2(self, fixt): pass

pytest 9.0.3           -> 2 passed
pytest 9.1.1 -W error  -> 2 errors

Four links:

1. pytest 9.1 deprecated class-scoped fixtures declared as instance methods (PytestRemovedIn10Warning). TestSyncCodeBase.execute_infra_sync / sync_code_base are exactly this shape (test_sync_code.py:49,84).
2. pytest.ini sets filterwarnings = error, turning that warning into a setup failure on the first test of every affected class.
3. pytest then raises its own internal AssertionError for the remaining tests in the class — upstream pytest-dev/pytest#14775, still open. Root cause is fixtures.py:1147: finish() early-returns when cached_result is None without clearing _finalizers, on an assumption that no longer holds.
4. --reruns 3 hides the cause. pytest-rerunfailures reports only the final attempt, so the real warning is discarded and only the internal AssertionError survives — which is why the CI logs contain zero occurrences of the actual error.

Confirmed link 4 directly, same test with and without --reruns:

without --reruns:  test_1 => PytestRemovedIn10Warning   <- real cause
                   test_2 => AssertionError
with --reruns 3:   test_1 => AssertionError             <- cause gone

The counts line up exactly: 52 errors × 3 reruns = 156 reruns (sync-watch: 12 × 3 = 36).

Blast radius is one shared base class — TestSyncCodeBase, inherited by test_sync_code.py, test_sync_build_in_source.py and test_sync_adl.py, which is why a single cause took out both sync-code and sync-watch. The same fixture pattern also exists in test_logs_command.py:41,58 and test_traces_command.py:44,61.

How does it address the issue?

Marks the warning as ignored on the three classes that declare these fixtures — TestSyncCodeBase, LogsIntegTestCases, TestTracesCommand.

This is safe here specifically: those fixtures assign to the class (TestSyncCodeBase.stack_name = ...), not to self, so the hazard the deprecation exists to catch — attributes set on self being invisible to tests — does not apply to them. They are only using a pattern pytest wants gone.

A class mark rather than a repo-wide pytest.ini entry, so the rest of the suite still fails on this warning (verified: a new unmarked class-scoped instance-method fixture added elsewhere still errors). And a class mark rather than a module-level pytestmark, because pytestmark is module-scoped while TestSyncCodeBase is subclassed from other modules — which is precisely why one base class took out both sync-code and sync-watch. Verified both directions:

	subclass in another module
module-level pytestmark on the base's module	still errors
class-level mark on the base	passes

Confirmed at runtime that all six affected classes inherit the mark, including TestSyncCode_BuildInSource_Esbuild and TestSyncAdlCasesWithCodeParameter.

Also fixes two unrelated sources of nondeterministic test collection, found while verifying the above. Both come from iterating a set of strings: hash randomization makes that order differ per process, so each pytest-xdist worker generated a different parameterized case list and the run aborted with "Different tests were collected between workers" (13 errors).

• test_lambda_container.py wrapped an already-duplicate-free 15-item list in set() — verified 15 items, 15 unique, so the set() was a no-op.

• hidden_imports.py built SAM_CLI_HIDDEN_IMPORTS via list(set(...)). This also made pyinstaller's hidden-import list unstable between builds. Now collected into a list, which is deterministic on its own — the set was redundant, since walk_modules already dedups via if pkg.name in visited.

  No sorted(): pkgutil.walk_packages already walks children in sorted order (_iter_file_finder_modules calls os.listdir(...).sort()). That sort is per-importer rather than a language guarantee — iter_importer_modules is a simplegeneric dispatch — so instead of re-sorting at import time, the invariant is asserted by two new tests: one over the fixture tree (same order twice, and sorted) and one over the real samcli walk. SAM_CLI_HIDDEN_IMPORTS is byte-identical across three separate interpreters, and identical to the previously sorted output.

  Measured before switching, since a list makes the membership check O(n): 658 modules, in costs 0.80 ms vs 0.008 ms for a set — once, at import.

What side effects does this change have?

Deferred, not avoided: converting those fixtures is required before pytest 10 removes the behaviour. I left it out deliberately — it is not mechanical. execute_infra_sync derives its stack name from self._method_to_stack_name(self.id()), and unittest's id() needs an instance, so any conversion changes live CloudFormation stack names. All three autouse fixtures also assert via self.assertEqual, which under @classmethod would silently bind the first argument as self rather than fail loudly. That belongs in its own PR, verified against real AWS. The pytest.ini comment records this.

Nondeterminism fixes change no CI behaviour. CI runs unit tests serially via make pr, so the 13 collection errors were local-only; this makes pytest -n auto usable locally, where it previously aborted.

Known follow-up: with collection fixed, -n auto surfaces pre-existing test-isolation failures that the collection abort previously made unreachable. The count and the affected tests vary between runs — I observed 4, 5, 6 and 10 across repeated runs, spread over three files:

tests/unit/local/lambda_service/test_local_lambda_http_service.py   (RuntimeError: Working outside of request context)
tests/unit/commands/deploy/test_guided_config.py
tests/unit/lib/sync/flows/test_rest_api_sync_flow.py

That variance is itself the diagnosis: which tests fail depends on how xdist distributes them across workers, i.e. order-dependent cross-test pollution. They pass serially (9386 passed) and pass when each file runs alone under xdist, so this is not a regression from this change, and it is not reachable in CI, which runs unit tests serially. Leading suspect is test_import_module_proxy.py reassigning importlib.import_module globally in setUpClass. Left for a separate PR — worth fixing, but it is a distinct problem from the pytest 9.1 breakage this PR unblocks.

Testing

• Verified against the repo's real pytest.ini: repro goes 6 errors → 6 passed, and reverting only pytest.ini restores the 6 errors
• tests/integration/{sync,logs,traces} collect: 266 tests
• Unit tests serial (what CI runs): 9388 passed, 25 skipped, 28 subtests passed
• Unit tests -n auto: 13 collection errors → 0; suite now completes
• black, ruff, mypy (1377 files) clean; make schema produces no diff

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Review the generative AI contribution guidelines
• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..851ec58
Files: 3
Comments: 1

[valerena]

how many modules are there from walk_modules? Another solution would be to just use a list instead of a set. Just samcli_modules = ["samcli"].

walk_modules has a if pkg.name in visited: continue, so it won't add a package more than once.

It's probably not too important, but it feels like a more natural solution unless there a reason to use a set before converting into a list.

[roger-zhangg]

converted to list and removed sorted.

[roger-zhangg]

But the bot's comment below actually make sense, liner scan is O(n^2/2) where sort is just nlogn, so adding a set and a list together is the best. But to be honest, this part is a super small contributor, (<40ms diff in a ~5 second import). But still, as I already touched it, changing it to set+list make sense

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..01bb427
Files: 4
Comments: 1

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..5880a6b
Files: 6
Comments: 2

[aws-sam-tooling-bot]

[PERFORMANCE] Switching visited from a set to a List[str] fixes the ordering problem, but it also turns the dedup check on the next line into a linear scan, and this function runs at import time.

The amplification matters here: import(pkg.name) returns the top-level module (import("samcli.cli") is samcli), so each recursive call re-walks the entire tree from the root. With ~670 modules and ~150 packages under samcli/, the pkg.name in visited check goes from ~100k O(1) set lookups to tens of millions of string comparisons. That cost lands on samdev startup (via samcli/cli/import_module_proxy.py, imported from main.py:148) and on every pyinstaller build.

Order stability and O(1) membership aren't mutually exclusive — keep the list for order and a parallel set for lookups:

def walk_modules(module: ModuleType, visited: List[str], seen: Optional[Set[str]] = None) -> None:
   """Recursively find all modules from a parent module"""
   if seen is None:
       seen = set(visited)
   for pkg in pkgutil.walk_packages(module.__path__, module.__name__ + "."):
       if pkg.name in seen:
           continue
       seen.add(pkg.name)
       visited.append(pkg.name)
       if pkg.ispkg:
           submodule = import(pkg.name)
           walk_modules(submodule, visited, seen)

Existing two-argument callers (including the new tests) keep working, and test_walk_modules_order_is_deterministic_and_sorted still holds.

[roger-zhangg]

Adopted in ceb03b0 — visited keeps order, parallel seen set for dedup, seen optional so the two-argument callers still work.

Your premise checks out. __import__("samcli.cli") does return top-level samcli, so each recursive call re-walks from the root; I instrumented it at 108,405 membership checks for 658 modules.

Two measurements worth adding, though:

The win is smaller than the analysis implies — ~4970 ms with the set vs ~5000 ms with the list, i.e. inside noise. I measured in separate fresh processes with sys.modules pre-warmed, so imports are not confounding it. The cost is not the string comparisons; it is the 108k redundant walk_packages iterations themselves (filesystem work).

Which points at the actual problem. pkgutil.walk_packages is already recursive, so walk_modules' own recursion is redundant. A single walk_packages(samcli.__path__, "samcli.") pass yields the identical 658 modules in 29 ms — about 170x faster, and it makes the list-vs-set question moot (658 checks instead of 108,405).

I did not make that change here, because it is not equivalent on failure: walk_packages swallows ImportError unless given an onerror callback, whereas the explicit __import__ propagates. Silently dropping a module would mean a missing import in the frozen binary instead of a loud build failure — worse than slow. That belongs in its own change, ideally with onerror wired to raise.

One scope note on the impact: import_module_proxy is imported from main.py:148 under command_path == "samdev", so this lands on samdev and pyinstaller builds, not on end-user sam startup.

[aws-sam-tooling-bot]

[GENERAL] The comment block was copy-pasted from test_sync_code.py and its central justification is wrong in this file and in tests/integration/traces/test_traces_command.py:38. It says a module-level pytestmark was rejected "because subclasses live in other modules (test_sync_adl.py, test_sync_build_in_source.py)". That is true only for TestSyncCodeBase. I verified:

• LogsIntegTestCases subclasses are both in this same module — TestLogsCommandWithRegularStack (line 244) and TestLogsCommandWithNestedStack (line 319).
• TestTracesCommand has no subclasses at all.

So the stated reason points a future maintainer at two unrelated sync test files. Since this comment is the entire rationale for the suppression and carries the removal instruction for the pytest 10 migration, a wrong rationale is likely to be acted on incorrectly. Either drop the pytestmark paragraph in these two files or replace it with the reason that actually applies (e.g. the mark belongs on the base class so the subclasses inherit it).

[roger-zhangg]

You are right, and this was my copy-paste — fixed in ceb03b0.

Verified your counts:

class	subclasses	modules
TestSyncCodeBase	32	test_sync_code (26), test_sync_build_in_source (4), test_sync_adl (2)
LogsIntegTestCases	2	test_logs_command only
TestTracesCommand	0	—

So the cross-module reasoning holds only for sync, exactly as you said. Each file now states only what applies to it: sync keeps the cross-module justification, logs says the two subclasses in the same module inherit it, and traces just says it is marked on the class rather than in pytest.ini so the rest of the suite still fails on the warning.

Your point about why this matters is the right one — that comment carries the pytest 10 removal instruction, so a wrong reason is the kind of thing someone acts on later.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: e1f4bf6..ceb03b0
Files: 6
Comments: 1

[aws-sam-tooling-bot]

[PERFORMANCE] The seen set makes the dedup check O(1), but the amplification it was added to compensate for is itself removable — the recursion is redundant.

pkgutil.walk_packages already yields the entire tree: for every entry with ispkg it imports the package and does yield from walk_packages(path, info.name + "."). So by the time the loop reaches walk_modules(submodule, visited, seen), every name that recursive call can produce is already in seen and gets skipped. The recursion contributes no names — only cost.

That cost is what the new docstring describes: because __import__("samcli.cli") returns the top-level samcli, each of the ~150 packages triggers another full-tree walk from the root. The set removes the ~108k membership checks, but the ~150 redundant walks remain, and their real expense is the filesystem work (_iter_file_finder_modules does an os.listdir + sort per directory, so ~150 × ~150 directory listings) plus the importlib machinery. This runs at import time of samcli.cli.hidden_imports, which is hit by the pyinstaller hook, samdev startup, and two unit test modules.

Dropping the recursive branch keeps the original two-argument signature, so the seen parameter does not need to leak into the API where a caller could pass a set that is out of sync with visited:

def walk_modules(module: ModuleType, visited: List[str]) -> 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 = set(visited)
   for pkg in pkgutil.walk_packages(module.__path__, module.__name__ + "."):
       if pkg.name in seen:
           continue
       seen.add(pkg.name)
       visited.append(pkg.name)

This preserves discovery order and dedup, so all four tests in tests/unit/cli/test_pyinstaller_imports.py still hold — including test_walk_modules_does_not_add_duplicates, since seen is rebuilt from visited on each call.