Repository navigation
fix(tests): order-dependent parser-test isolation — the bare-name import-state leak (#627) - #662
Merged
Merged
Conversation
…ort-state leak (#627) pytest tests/parsers failed 12 tests on a clean master (4 in the python callgraph-constructs file, 8 in the MRO import-bases file) while every file alone, the pair together, and the full suite passed. Bisected to tests/parsers/c/test_empty_seed_keep_all.py: it execs parsers/c/test_pipeline.py, which (like parsers/python/parse_repository.py and the PHP and Ruby pipeline scripts) imports its machinery under BARE module names via a sys.path entry — the exec leaves the C versions in sys.modules, and a later python parse_repository then gets the C FunctionExtractor (first-import wins), whose statistics lack standalone_functions — the KeyError behind all 12 failures. The full suite passes only because some earlier test imports the python bare names first: order decides the winner. The fix is loader isolation, not a test reorder: snapshot sys.path and sys.modules at ENTRY and restore around the exec (new keys removed, replaced keys restored) — applied to the loaders that exec bare-importing pipelines (c, zig, php, ruby; Go/JavaScript/Swift use qualified imports and do not poison the bare-name cache). The self-contained regression test reproduces the poisoning WITHIN one test (no invocation order needed) and pins the no-leak invariant.
gadievron
requested review from
dgeyshis,
shahar-davidson and
sounil
as code owners
September 21, 2026 03:18
The regression test's docstring claimed every parser's pipeline script uses bare-name imports and that the fix covers two exec sites. Neither is accurate: Go, JavaScript, and Swift use qualified imports (their loaders leak sys.path state but do not poison the bare-name cache), and the fix covers c, zig, php, ruby — the loaders whose pipelines import bare names.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
pytest tests/parsersfailed 12 tests on a clean master (4 intest_issue309_py_callgraph_constructs.py, 8 intest_issue440_mro_import_bases.py) while every file alone, the two together, and the full suite passed — the exact receipt from the issue. Closes #627.Root cause (bisected)
The poisoner is
tests/parsers/c/test_empty_seed_keep_all.py. It execsparsers/c/test_pipeline.py, which — likeparsers/python/parse_repository.pyand the PHP and Ruby pipeline scripts — imports its parser machinery under bare module names (function_extractor,call_graph_builder,repository_scanner,unit_generator) resolved via asys.pathentry. The exec leaves the C versions insys.modules; a laterparse_repository(language="python")then gets the CFunctionExtractor(first-import wins), whose statistics lackstandalone_functions— theKeyErrorbehind all 12 failures. The full suite passes only because some earlier test imports the python bare names first: order decides the winner, which is exactly the flakiness class.The bisection: directory halving with a failing file as the detector →
tests/parsers/c→ single-file scan →test_empty_seed_keep_all.py; the pair (poisoner + both failing files) reproduces 12/12 deterministically.The fix (isolation, not a reorder)
Snapshot
sys.pathandsys.modulesat loader ENTRY and restore around the exec — new keys removed, replaced keys restored. Applied to the loaders that exec bare-importing pipelines: c, zig, php, ruby (Go, JavaScript, and Swift use qualified imports and do not poison the bare-name cache; their loaders still leaksys.pathstate — a broader sweep is noted as follow-up).Testing
test_issue627_import_state_isolation.py) reproduces the poisoning within one test — no invocation order needed: exec the C loader, then run a pythonparse_repositoryresolution; it asserts the pkg-defined re-export resolves and that the loader leaves no import state.pytest tests/parsers→ 918 passed, 1 skipped, 0 failed (was 12 failed, 805 passed).test_llm_sdk_contract_floorpins, a local-environment artifact (installed anthropic 1.3.0 vs therequirements.txtpin 1.7.0; CI installs the pin), not master failures. Corrected 2026-09-21.Coordination