Add opt-in parallel testing; fix registry and fixture isolation#539
Merged
Conversation
why: The pytest-optimizer pipeline writes durable analysis state under .pytest-optimizer/; it is tooling output, not source. what: - Ignore the .pytest-optimizer/ state directory
why: Enable optional parallel test execution for the subprocess-bound suite; pytest-xdist is the runner that provides it. what: - Add pytest-xdist to the dev and testing dependency groups - Regenerate uv.lock
why: Give contributors a one-word entry point for parallel runs without memorizing the xdist flags. what: - Add `test-parallel` recipe: uv run py.test -n auto
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #539 +/- ##
==========================================
- Coverage 61.19% 61.18% -0.01%
==========================================
Files 40 40
Lines 6563 6557 -6
Branches 1103 1103
==========================================
- Hits 4016 4012 -4
+ Misses 1950 1948 -2
Partials 597 597 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
Code reviewFound 1 issue:
Lines 23 to 42 in e918163 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
why: Document the new contributor-facing parallel test capability. what: - Add a Development entry describing pytest-xdist + test-parallel
why: parser_map was a class variable shared by every VCSRegistry, so constructing a second registry (as the registry docs example does) mutated the global `registry` singleton -- replacing its git parser. This surfaced as order-dependent test failures, but it is a real bug for any downstream caller that builds a custom registry. what: - Initialize parser_map as an instance attribute in __init__ - Drop the ClassVar declaration
why: The first consumer of git_repo/hg_repo got a live handle to the session-scoped master_copy, so its mutations (e.g. adding a remote) leaked into every later test that copied the cache. Surfaced as order-dependent failures; a real isolation bug for plugin consumers. what: - Build master_copy once as a pristine read-only cache - Always return an isolated copytree, including for the first consumer - Apply the same fix to git_repo and hg_repo
why: The RuleMap.register doctest added 'gl-prefix' to the global GitURL.rule_map without removing it, unlike the git/svn/hg examples which clean up. The leaked rule persisted into later tests. what: - Unregister 'gl-prefix' at the end of the example
e918163 to
e088abc
Compare
why: The svn_repo cache was dead. The cache-miss branch checked out to projects_path instead of master_copy, so master_copy was never written, the cache never hit, and new_checkout_path/unique_repo_name were unused; every svn test re-ran a full checkout. (Isolation was unaffected since projects_path is function-scoped.) what: - Build master_copy once via obtain(), then copytree to an isolated checkout for every consumer, mirroring git_repo/hg_repo
fe7ab57 to
f8104de
Compare
why: The workflow guide's Tests section listed only serial runs; the new opt-in xdist mode (just test-parallel) and the suite's order-independence expectation were undocumented. what: - Add a "Running tests in parallel" subsection (just test-parallel / -n auto) - Add an "Order independence" subsection with a shuffled-run check
why: The git_repo/hg_repo/svn_repo per-test isolation guarantee lived only in inline comments, so downstream consumers (e.g. vcspull) could not see it from the docstrings or rendered docs. A few neighboring fixture docs were also stale. what: - Document the per-test isolation contract in the three repo fixtures' docstrings and add a "Repository isolation" note to the plugin doc page - Correct the inaccurate "session-scoped" card in docs/api/index.md - Fix the git_remote_repo docstring and the "Emphemeral" typo
why: The custom-registry example never stated that building a custom VCSRegistry leaves the module-level registry untouched -- the exact property whose absence was a recently fixed bug. what: - Note that subclassing with a local RuleMap is isolated, vs registering on GitURL.rule_map which mutates shared class state - Add a doctest asserting the global registry still resolves git to GitURL after a custom registry is built (regression guard)
why: Record the decision behind the test-isolation fixes and the opt-in xdist mode so the rationale and rejected alternatives survive. what: - Add ADR 0002 (order-independence invariant + opt-in parallelism, with alternatives, consequences, and prior art) - Register it in the ADR index toctree
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.
Summary
just test-parallelrecipe (uv run py.test -n auto). The defaultuv run pytestis unchanged and still runs serially — parallelism is purely opt-in.VCSRegistrysharing parser state across instances. Its parser map was a class variable, so constructing any second registry mutated the sharedregistrysingleton (replacing its git/hg/svn parsers). Each registry now owns its own parser map.git_repo/hg_repopytest fixtures leaking state between tests. The first consumer of either fixture received a live handle to the session-cached master checkout, so mutating that checkout (adding a remote, switching branches) polluted the cache for every later test. The master copy is now a pristine, read-only cache and every consumer — including the first — gets its own copy.VCSRegistryand the pytest fixtures (e.g. vcspull), independent of parallelism.Changes by area
Parallel testing (opt-in)
pyproject.toml: addpytest-xdistto thedevandtestingdependency groups.justfile: add atest-parallelrecipe —uv run py.test -n auto..gitignore: ignore the.pytest-optimizer/profiling-state directory.Test-isolation fixes
src/libvcs/url/registry.py: initializeparser_mapas an instance attribute in__init__instead of aClassVar, so eachVCSRegistryis independent and the module-levelregistryis never mutated by constructing another.src/libvcs/pytest_plugin.py: build thegit_repo/hg_repomaster checkout once as a pristine read-only cache, then always return an isolatedcopytreeof it — the first consumer no longer receives a live handle to the cache.src/libvcs/url/base.py: unregister the demo rule at the end of theRuleMap.registerdoctest, matching the git/hg/svn examples, so it doesn't leak into the globalGitURLparser.Design decisions
uv run pyteststays serial and stable. The worker count is left to the operator (-n auto) rather than hardcoded inaddopts, because the optimal count is machine-specific (high-core machines oversubscribe a subprocess-bound suite).--distgrouping to paper over the coupling, the registry and fixtures were made genuinely isolated. This is the more robust fix and it also corrects real bugs for downstream callers.Before / after —
VCSRegistryBefore, a second registry clobbered the global one's parsers:
After, each registry is independent; the global
registryis untouched.Verification
The suite is order-independent (run shuffled across several seeds):
$ uv run --with pytest-randomly pytest -p randomly -p no:cacheproviderParallel execution is green:
$ uv run pytest -n autoTest plan
uv run ruff format .anduv run ruff check .— cleanuv run mypy— clean (strict, coverssrcandtests)uv run pytest— default serial suite green and unchangeduv run pytest -n auto— parallel run greenuv run --with pytest-randomly pytest -p randomly— green across multiple seeds (suite is order-independent)