fix(tests): make isolated_home actually isolate, and apply it to every test - #64
Open
Hotragn wants to merge 2 commits into
Open
fix(tests): make isolated_home actually isolate, and apply it to every test#64Hotragn wants to merge 2 commits into
Hotragn wants to merge 2 commits into
Conversation
…he real registry `isolated_home` patched only $HOME. `core.paths.home_dir()` resolves the user home through `Path.home()`, and on Windows `ntpath.expanduser` reads %USERPROFILE% and ignores $HOME. Every test using the fixture therefore ran against the developer's real `~/.codealmanac` on Windows. `uv run pytest` on a clean Windows checkout created a real `~/.codealmanac/codealmanac.db` holding a repositories row pointing at a pytest temp directory, plus `~/.codealmanac/repos/<id>/index.db`. Because registry entries are never auto-dropped, that row survives the run and every later one. The fixture now patches %USERPROFILE% as well, and fails loudly if `Path.home()` does not land inside the sandbox, so an insufficient patch set can never silently degrade into using the real home again. This also fixes two pre-existing Windows failures that were masked by the escape: `test_default_user_state_paths_are_product_specific` and `test_cli_setup_and_uninstall_codex_instructions`.
… ask for it
45 tests build an app or invoke the CLI without requesting `isolated_home` and
without passing an explicit `database_path`, so `AppConfig()` falls back to
`~/.codealmanac` and they operate on real global user state. This is not
Windows-specific: `Path.home()` is the developer's real home on every platform.
The visible consequence is that `uv run pytest` is not repeatable.
`repositories.name` is `NOT NULL UNIQUE`, and `RepositoryStore.remember` upserts
`ON CONFLICT(repository_id)` only, so a second run registers a fresh temp path
under the already-taken name "repo" and raises:
tests/test_validate.py::test_cli_validate_returns_nonzero_for_issues
IntegrityError: UNIQUE constraint failed: repositories.name
Reproduced on main, from a clean `~/.codealmanac`:
uv run pytest tests/test_validate.py -q # 7 passed
uv run pytest tests/test_validate.py -q # 1 failed, 6 passed
CI does not see it because each job starts from a fresh container, so the second
run never happens.
Rather than add the fixture to 45 call sites, `isolated_home` becomes `autouse`.
Every test now gets a sandboxed home under its own `tmp_path`, the 390 existing
explicit requests keep resolving to the same object and are unchanged, and no
future test can opt out of the invariant by forgetting an argument. This mirrors
the existing autouse `disable_external_telemetry_during_tests` fixture.
Full suite is unchanged at 11 failed / 552 passed, and the same two runs above
now pass twice with `~/.codealmanac` never created.
Stacked on the %USERPROFILE% fix, which this needs in order to sandbox anything
at all on Windows.
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
tests/conftest.py'sisolated_homefixture does not deliver the guaranteeCONTRIBUTING.mdmakes for it. Two independent gaps, one commit each:$HOME, butntpath.expanduserreads%USERPROFILE%and ignores$HOME, so all 390 call sites ran against the real~/.codealmanac.database_path, so they hit real global state on every platform, macOS and Linux included.Net effect of (2):
uv run pytestis not repeatable. It passes once, then fails.Why
CONTRIBUTING.md: "Tests that touch user state should use theisolated_homefixture so they write under a temp~/.codealmanac/, not the real registry."Gap 1 — the fixture is a no-op on Windows.
core.paths.home_dir()resolves the home throughPath.home():After one
uv run pyteston a clean Windows checkout:A real registry row pointing at a pytest temp directory, plus a real
~/.codealmanac/repos/<id>/index.db. Since "registry entries are never auto-dropped", it persists.Gap 2 — the suite is not repeatable.
repositories.nameisNOT NULL UNIQUE, andRepositoryStore.rememberupsertsON CONFLICT(repository_id)only — nothing handles anameconflict. So the second run registers a fresh temp path under the already-taken name"repo". Reproduced onmainfrom a clean~/.codealmanac:CI never sees this because every job starts from a fresh container, so there is no second run. A developer running the suite twice hits it, and the only remedy is deleting
~/.codealmanac— which also destroys their real registry.Verification
Suite totals are identical across the autouse commit (11 failed / 552 passed both sides), so nothing depended on seeing the real home.
The two newly passing tests were fixed by the
%USERPROFILE%commit:test_default_user_state_paths_are_product_specificandtest_cli_setup_and_uninstall_codex_instructions. Both asserted correct behaviour and failed only because the fixture leaked.The remaining 11 failures are pre-existing and unrelated — 5 macOS
launchdtests plus non-portable fixtures (test_tagging.pywrites\r\nthrough translating text mode;test_transcript_discovery.pyinterpolates a Windows path into JSON). Happy to send those separately; I kept them out to hold this diff to the isolation invariant.Docs and wiki
Notes for reviewers
%USERPROFILE%fix is Windows-only; theautousechange affects all platforms. They touch the same six lines, so splitting them into separate PRs would just create a conflict — but say the word and I will drop either commit.src/change.Path.home()reading%USERPROFILE%is correct product behaviour on Windows; the bug is that the fixture never overrode it. I deliberately did not add aCODEALMANAC_HOMEoverride, since that adds public env surface to fix a test-only problem.%USERPROFILE%fixes today's platforms; assertingPath.home() == homemeans any future platform where that env set is insufficient fails loudly at setup instead of silently using the real home. A silent no-op is what let this survive.autouseover 45 signatures. Adding the fixture to each call site is a large mechanical diff that fixes today's tests and leaves the 46th to reintroduce the bug.autousecloses the class. A fixture can beautouseand still be requested by name, so the 390 explicit requests resolve to the same object and no assertion changes. Precedent:disable_external_telemetry_during_testsin this same file is alreadyautousefor the same reason.HOMEDRIVE/HOMEPATHleft alone on purpose —ntpath.expanduserconsults%USERPROFILE%first, so they are unreachable here, and clearing them would affect unrelated subprocess behaviour.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.