fix(tests): keep tests inside their sandbox and cut suite time #218
No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!218
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/test-isolation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #216.
Closes #210. Tests mock the two outbound gateway calls, enforce the outbound TCP guard, and keep collection-time databases and lifecycle logs in temporary locations. The merge-grace test uses the shorter configured wait. Production database targets retain their existing behavior; this change adds no API or schema contract.
Research for #217 is at 217-test-performance-and-agent-workflows.md, with a research index linked from docs/README.md. It includes reproducible scripts, complete compressed logs, command/SHA/tree/environment receipts, setup/call profiling, and a real recent-commit selection dry run.
Conftest integration (
tests/conftest.py):isolated_repository_dbfixture (#243).tests/occupancy_reset.pywithout importingtests.conftest(#233).PYTEST_SHUFFLE_SEEDhook (refactor tracked in #264).tests/test_review_script.pyskips outside a git repo when unpacked viagit archive.Measured medians of three runs on wings (i5-8265U, shared varying load): baseline 219.85 s; after serial 198.32 s; -n 2 102.04 s; -n 4 64.24 s; -n auto 64.62 s. All 15 benchmark runs passed, with the archived sources skipping the optional Tailwind bundle/source comparison because the ignored binary was absent. Each after run passed 539 tests with that one skip. Auto actually used four workers. The earlier projected runtime/speedup and unsupported database-lock claim are withdrawn. Local observations do not predict CI speed.
Pytest already executes node --test through tests/test_frontend_modules.py; there is no missing frontend CI gate. Selection is advisory only: the replay from
a7e89c6to103157cdemonstrates relevant regression tests missed by naive path mapping. The report answers tree-hash caching, CPU contention, and review execution receipts, and cites primary pytest-xdist, testmon and SQLite sources.Follow-ups: #238 (parallel/CI evaluation), #240 (profile before optimizing fixtures), #241 (conservative scoped runner and receipts), #242 (hook policy proposal). #235 is the separate remote VPS evaluation blocked by this PR's hermetic-suite prerequisite. #264 tracks tidying the occupancy reset helpers and documenting the
PYTEST_SHUFFLE_SEEDhook. #269 tracks the cardholder-name retry order dependence uncovered by full suite shuffling under seeds 233 and 264.Related open test issues and research handoff
The maintainer-confirmed policy is recorded in the separate documentation draft PR #265 (
docs/test-policy-217→master). This PR delivers the research and isolation fixes; #265 records the subsequent design decisions, and implementation remains in follow-up issues.Existing direct follow-ups are #238 (parallel/CI evaluation), #240 (fixture profiling), #241 (scoped runner and evidence), #242 (hook policy), and #235 (remote execution). Their executable briefs must be reconciled with the measured findings and the maintainer-confirmed validation policy; a report withdrawing old claims does not itself update those briefs. Before research closure/merge, link accurate follow-up issues for each accepted actionable recommendation, creating additional issues only for uncovered work.
Other open issues provide concrete examples and validation cases for this research:
PYTEST_SHUFFLE_SEEDhook documentation: follow-up from PR #233 review pass 2/3.These are related examples, not additional issues closed by this PR. The existing #216 and #210 fixes remain the hermetic-network and filesystem-isolation prerequisites delivered here.
Verification: full pytest in the actual worktree passed 604 tests with zero failures/skips, including the frontend wrapper and Tailwind comparison. The installed commit hook passed; ruff check/format and check_docs.py passed (46 Markdown files, 90 HTTP operations).
Triage update (2026-10-03). #217 is now
ready-for-agent, and its agent brief is the contract for this PR's #217 part. The changes that affect this PR:docs/research/217-test-suite-speed.md. If this PR is the first to adddocs/research/, also adddocs/research/README.md(a one-line index per document) and link it fromdocs/README.md.ready-for-human, blocked by this PR). Name it in the findings document as an option and link #235; don't benchmark the VPS.needs-triagefollow-up issues.WIP: fix(tests): keep tests inside their sandbox and cut suite timeto fix(tests): keep tests inside their sandbox and cut suite timeImplementation Summary (
bbc4479)The implementation for PR #218 is complete and pushed to
origin/fix/test-isolation. It closes #216 and #210, and delivers the research requirements for #217.What Was Done
#216: Mock Outbound Hardware Calls & Enforce Session Network Guard:
ArtemisClient.request_asyncintests/test_docs.py::test_artemis_execution_endpoint_admin(duration reduced from 25.1 s to 0.30 s).BumblebeeClient.get_version_asyncandraw_request_asyncintests/test_api.py::test_probe_endpoints_typed_responses(duration reduced from 20.1 s to 0.61 s).guard_outbound_networkintests/conftest.pyinterceptingsocket.socket.connectandconnect_exforSOCK_STREAMsockets. Any test attempting an unmocked remote connection fails immediately naming the test and address (allowinglocalhost,127.0.0.1, loopback IPs, and ASGI transports).tests/test_network_guard.pyverifying that the guard reliably trips on remoteconnectandconnect_exattempts while permitting loopback connections.mock_webhook_manager_in_testsfixture intests/conftest.pyreturning127.0.0.1for local IP discovery to avoid UDP routing lookups against external server IPs.#210: Sandbox Database and Lifecycle Logs from Checkouts:
tests/conftest.pyat module load time (_session_test_db_dirandDATABASE_PATHredirection), preventing top-level imports during pytest collection from creatingdata/hikcentral.db.scripts/check_docs.pyto initialize an isolated temporary SQLite database before importingapp.main, eliminating "Error loading doors from DB" and preventingdata/creation.guard_lifecycle_log_in_testssession fixture intests/conftest.pyredirecting root./logs/lifecycle.logwrites during tests to temporary test directories.tests/test_cli_commands.pyandtests/test_viewer_role.pyto usetmp_pathandmonkeypatch.chdir(tmp_path), asserting ontmp_path / "logs" / "lifecycle.log".app/cli/commands/cmd_doctor.pyandapp/services/db_sync_service.pyto resolve staging and database directories relative toPath(settings.db_path).parent.tests/test_sandbox_isolation.pyproving that runningcheck_docs.pyand a full pytest run in a fresh worktree leaves nodata/orlogs/behind.#217: Research Findings, Safe Quick Wins & Follow-up Issues:
docs/research/test-performance-and-agent-workflows.mdlinked indocs/README.md.WT_CI_START_GRACEfrom 2s to 1s intest_wt_merge.pyand resolved import-time database creations.needs-triageawaiting maintainer policy evaluation.Fast, hermetic agent test loop:perf(tests): evaluate pytest-xdist safe parallelism with isolated worker databases(ready-for-agent)ci(frontend): execute node --test suite in forgejo actions CI workflow(ready-for-agent)perf(tests): batch synthetic time-series insertions in statistics test fixtures(ready-for-agent)feat(cli): add scripts/wt test runner with scoped change detection and compact output(ready-for-agent)chore(ci): refine pre-commit hook scope to skip pytest on docs-only changes(needs-triage)Verification Results
rtk ruff check .andrtk ruff format --check .).python3 scripts/check_docs.pychecked 41 Markdown files and 80 HTTP operations with 0 errors.data/norlogs/exists in the worktree.Next Step
Ready for the first review pass per repo review workflow.
Code review, pass 1 (
016a012...bbc4479, spec #217 brief)Result: 7 P2s and 2 P3s. Not mergeable. This is a research PR, and it does not yet meet the #217 brief. Most numbers are projected rather than measured, and one finding is wrong. Pass 1 fixes everything, so address every item, merge
origin/masterand request a second pass.P2
node --testviatests/test_frontend_modules.py::test_telemetry_frontend_modules. Correct the doc. Follow-up #239 is built on this finding, so it has been moved back toneeds-triagewith a recommendation to close it. Don't link it as a valid follow-up.pytest -n auto(and-n 2/-n 4) for real, and either reproduce the lock error with its traceback or withdraw the claim.--durations, setup vs call), and give the source or the measurement behind the template-DB timings.P3
docs/research/217-<slug>.md, add adocs/research/README.mdindex, and link that index fromdocs/README.md.Pass 1 fixes (
20e4889)Addressed every finding in r33 and merged origin/master (
ffee469). Research report and evidence.016a012median 219.85 s; merged afterffee469median 198.32 s. Exact commands, full SHAs/tree hashes, machine, dependency versions and start/end load recorded. Projections withdrawn.Validation: real worktree full suite passed 540 tests, zero skips/failures (including node --test and Tailwind comparison), installed commit hook passed, Ruff check/format passed, docs checker passed (45 files / 80 HTTP operations). The frozen benchmark snapshots omitted the ignored Tailwind binary, so their recorded one skip is the optional bundle/source comparison. All-files formatting found legacy defects outside scope; those automatic edits were restored and staged-file hooks passed.
Please run pass 2 against
20e4889.Adopt the shared SQLite fixture from #243 (
00a9b28)#243 now owns the shared-fixture remediation requested by the maintainer. Please adopt the opt-in
isolated_repository_dbfixture fromtests/conftest.pyat00a9b28before your next review update, preserving this PR's early database isolation and outbound-network/lifecycle-log guards. Add the fixture alongsideisolated_test_db; do not replace your entire conftest with #243's version or import #243's business-cycle application changes.Tests that need fresh per-test repository state request it with
pytest.mark.usefixtures("isolated_repository_db")or a fixture dependency. It remains opt-in; session isolation and your guards remain in place.Process/evidence: read your pushed pass 1 fixes at
20e4889, extracted that exact conftest into a temporary compatibility harness, added this fixture and #243's nine business-cycle label cases, and ran them against #243's application code. All nine passed with your conftest guards; #243's own nine cases and full-suite commit hook also passed. Conclusion: the fixture is compatible with your conftest; adoption on #218 is still required. Please confirm the adopting SHA on this PR. The latest #243 fixes comment was edited to record the remediation, process, conclusion and new review head.Pass 1 fixes, continued (
4abfff3)Merged
origin/master(9a608c1) and reconciledtests/conftest.pyand test isolation:#243'sisolated_repository_dbfixture for fresh isolated repository state.#233'stests/occupancy_reset.pyreset helper (reset_test_occupancy_state), ensuring noimport tests.conftest.tests/test_review_script.pyto skip cleanly when executed outside a git repository (such as reviewers running unpackedgit archiveexports).PYTEST_SHUFFLE_SEEDhook intests/conftest.py(follow-up cleanup and docs tracked in #264).Validation: full pytest suite passed (610 tests passed). Pre-commit hooks, ruff lint/format, and
check_docs.py(46 markdown files, 90 operations) all green.Ready for pass 2 review.
Code review, pass 2 (
origin/master...4abfff3, spec #217 brief + decision record c4305 + D16 confirmation)Summary: 4 distinct P2s (after removing duplicates), plus P3s. Later pass: fix the P1s and P2s here. The remaining P3s are filed as #272.
All r33 findings are fixed. The conftest merge keeps the session bootstrap, the network guard, #243's
isolated_repository_dband #233'stests/occupancy_reset.py. One unshuffled full run in an export was green: 571 passed, 39 skipped, 150.5 s.Must fix:
DATABASE_URLbypasses the session sandbox (raised by both axes).conftest.py:17-28only redirectsdb_path, whileconnection.py:46gives precedence todatabase_url, which Settings also loads from.env. A probe seeded an external DB with users. Fix: at conftest load, popDATABASE_URLand setsettings.database_url = None, and add a regression test.log_lifecycle_eventby name at collection, so rebinding the module attribute misses them. A probe wrotelogs/lifecycle.log. Patch every import site, or redirect the log path through settings or an environment variable.data/orlogs/(Standards P2-2). That includes the main checkout and 10 worktrees, and the failure message blames the wrong cause. Assert against a snapshot taken before the run.docs/research/217-test-performance-and-agent-workflows.md. Rewrite #238 and #240 against the measured findings, or send them back to needs-triage, and fix the paths in all four. (PR #265 fixes #241 and #242 itself; coordinate rather than duplicate.)test_network_guard.py:12,26targets10.10.1.251, the real default HikCentral host. If the guard regresses, the test connects to production. Use a TEST-NET address (192.0.2.1).Filed as #272: Standards P3-1, P3-2, P3-3, P3-4 and P3-6, and Spec P3-1 through P3-5.
Standards axis
Suite run
I ran the full unshuffled suite once in a git-archive export of
4abfff3f69(timeout 400 ~/.local/bin/pytest -q). It was green: 571 passed, 39 skipped, 1 warning, 150.5 s.Required invariants
app.configis imported, then runs init_db() at conftest load.isolated_repository_dbimport tests.conftest. conftest.py:13 imports it, and conftest.py:203-210 matches master.Previous-pass findings (r33, fix replies c4022/c4054/c4419)
r33 was a spec-axis pass on the research doc. I checked each item against docs/research/217-test-performance-and-agent-workflows.md at
4abfff3f69.node --teston tests/frontend/*.test.js. #239 is no longer listed among the follow-ups.Side note: the doc still quotes a test count of 539/540. The suite now has 610 tests. This is informational only, because the doc's timings are pinned to SHAs.
New findings
P2-1: The lifecycle-log guard never intercepts real callers. CONFIRMED
Where: tests/conftest.py:115-145. The guard rebinds
audit_logger.log_lifecycle_eventon the module.Why it fails: every real caller binds the function by name when it is imported. These are app/cli/commands/cmd_user.py:9, cmd_db.py:8, cmd_setup.py:10, cmd_service.py:6, cmd_update.py:10, cmd_uninstall.py:8 and app/cli/update/rollback_engine.py:12, all via
from app.cli.common.audit_logger import log_lifecycle_event. Those imports happen during collection, before the session fixture runs, so the callers keep the original function. The per-testmonkeypatch.chdir(tmp_path)lines added to test_cli_commands.py and test_viewer_role.py are what actually keep the logs out of the checkout. The newassert (tmp_path/"logs"/"lifecycle.log").exists()lines confirm the guard did not redirect anything.Probe: a test in the export calls
cmd_user.log_lifecycle_event("PROBE", {...})from the repo cwd.cmd_user.log_lifecycle_event is audit_logger.log_lifecycle_eventis False.<export>/logs/lifecycle.logwas created.Failure scenario: a future CLI test that omits the chdir writes to the worktree's logs/lifecycle.log. #210 is meant to prevent exactly that, and the guard does not catch it. The only safety net is the sentinel test (P2-2).
Fix: patch the name in each consumer module, or redirect at the source. Examples: an autouse
monkeypatch.chdir, or an audit_logger default directory taken from settings or an env var that the conftest sets at load time.P2-2: The sandbox sentinel tests fail on any checkout that already has data/ or logs/, and blame the wrong cause. CONFIRMED
Where:
test_check_docs_creates_no_database_in_checkout)test_worktree_has_no_test_database_or_lifecycle_logs)Why it fails: both tests assert that
ROOT/data/hikcentral.dbandROOT/logs/lifecycle.logdo not exist. They check the state of the environment, not the effect of the test.Probe:
touch data/hikcentral.db logs/lifecycle.login the export, then run tests/test_sandbox_isolation.py. Result: 2 failed. The message says "check_docs created database file in checkout", which is false; the file existed beforehand.Failure scenario: when one of those worktrees merges master after #218 lands, its pre-commit pytest goes red until someone deletes files by hand. The same happens to anyone running pytest in a checkout used for local validation, and the message points at the wrong cause.
Fix: record whether the files exist (and their mtime/size) before running check_docs, and assert they are unchanged. For the generic sentinel, compare against a snapshot taken at session start (conftest load), not against non-existence.
P2-3: Session isolation only redirects
db_path, soDATABASE_URL(env or .env) sends the whole suite to the configured database. CONFIRMEDWhere: tests/conftest.py:17-28.
Why it fails: the conftest sets only DATABASE_PATH and
settings.db_path. app/db/connection.py:46 resolvesurl or settings.database_url or settings.db_path, sodatabase_urlwins. Settings also loads the repo.env(app/config.py:10-11). #243's opt-in fixture does nulldatabase_url(conftest.py:151), which shows the precedence is known, but the session bootstrap does not.Probe:
DATABASE_URL=sqlite:///<scratch>/leak.db pytest tests/test_viewer_role.py. Result: 8 passed, and leak.db was created (296 KB) and seeded with admin, operator and viewer users.Failure scenario: a developer or the prod host has
DATABASE_URL=sqlite:///./data/hikcentral.dbin .env, as documented in docs/architecture/database-management-and-remote-sync-architecture.md:82. Running pytest there seeds users and runs DELETEs in the real database: reset_test_occupancy_state wipes occupancy_holidays and occupancy_events on every test. The same gap exists on master, but #218's purpose is #210 sandboxing, so the bootstrap should close it.Fix: also
os.environ.pop("DATABASE_URL", None)and setsettings.database_url = Noneat conftest load.P3
guarded_connectandguarded_connect_exrepeat the same check-and-fail block. Extract a_reject_outbound(sock, address)helper. CONFIRMED._orig_db_pathis read afterDATABASE_PATHhas been set, so it already equals the temp path. The "restore" inpytest_sessionfinishputs back a path to a directory deleted on the next line, and line 24 is a no-op. Drop both, or capture the original before line 19. CONFIRMED by reading: Settings reads DATABASE_PATH whenapp.configis imported at line 21.test_wt_review_forwards_arguments_from_nested_directoryandtest_wt_ignores_a_regular_scripts_package_on_pythonpath. The other 36 are pure parser tests. Put the skip on those two tests only. CONFIRMED (probe: 2 failed, 36 passed).db_dir = Path(settings.db_path).parentis relative to the cwd, not toroot, while every other directory check is rooted. Runningdoctorfrom another cwd checks the wrongdatadirectory. Resolve it againstroot, or useget_db_path(). PLAUSIBLE.Verdict
Not mergeable yet: 3 P2s, all confirmed by probe, plus 6 P3s. The required invariants (bootstrap, network guard, #243 fixture, #233 reset helper, review-test skip) are preserved, the suite is green, and every r33 finding is fixed. But the #210 log guard does not work (P2-1), the sentinel tests break existing worktrees (P2-2), and session isolation can be bypassed with DATABASE_URL (P2-3).
Spec axis
Result: 2 P2s and 5 P3s. Not mergeable yet. All nine pass-1 findings are addressed in the doc and PR. The conftest merge keeps all three required behaviours (probed). The open problems are the D16 handoff accuracy and one hole in the #210 sandbox.
Probes ran on a
git archive 4abfff3f69export with targeted files only: test_review_script, test_network_guard, test_sandbox_isolation, test_wt_merge, test_cardholder_name_resolution, the two #216 node IDs, and a throwaway probe file. All passed, apart from the skips and the deliberate DATABASE_URL probe described below.Previous findings (r33)
test_frontend_modules.py:22runsnode --test. #239 is closed and not linked.runs.jsonlhas the command, cwd, env and exit code per run. Load is recorded but not controlled (1-min load 5.8→22.6), and the doc says so.workers.jsonlshows 2 and 4 workers. Lock claim withdrawn (lines 12, 214). Not adopted, and the doc says why (line 72). The brief's "5 consecutive -n auto" was not done, but it only applies if xdist is adopted.docs/research/217-test-performance-and-agent-workflows.md,docs/research/README.md, linked fromdocs/README.md. The slug differs from the brief's217-test-suite-speed.md, and #241/#242 link to the brief's name (see P2-1).Required-behaviour checks:
tests/conftest.py:17-28creates the temp DB at import, before the app is imported. A probe test sawget_db_target()=/tmp/tmpXXX/test_hikcentral.db, and nodata/orlogs/entries appeared in the export.tests/conftest.py:68-98fails tests that connect to 192.0.2.1:80 or2001:db8::1(probed). The lifecycle-log guard is kept attests/conftest.py:115-145.isolated_repository_db: KEPT and opt-in.tests/conftest.py:148-153is byte-identical to master. A probe showed the per-test target istmp_path/repository.db, and the next test is back on the session DB.tests/occupancy_reset.pyis unchanged from master. There is noimport tests.conftestanywhere; the only mention is a comment intest_holiday_state_reset.py:84.PYTEST_SHUFFLE_SEEDhook: KEPT (conftest.py:189-200). #264 and #269 are referenced in the doc (lines 203-207, 320-321) and in the PR body.test_review_script.pyskip outside git: KEPT. All 38 tests skip in the export (see P3-2).New findings
P2
P2-1. The D16 handoff is not accurate: two linked ready-for-agent follow-ups still carry the claims this research withdrew. CONFIRMED (read via
tea api).D16 (c4305, confirmed in the maintainer's follow-up comment) says existing issues provide the handoff only "after their contradictory briefs are corrected". The PR body admits they still "must be reconciled ... before research closure/merge", and nothing has been reconciled.
test_hikcentral_gw_{id}.dbnaming. The doc withdraws the lock claim (line 214). It also shows workers already get distinctTemporaryDirectory()DBs (lines 193-194).docs/research/test-performance-and-agent-workflows.md, which does not exist. #241 and #242 citedocs/research/217-test-suite-speed.md, which does not exist either. The real file is217-test-performance-and-agent-workflows.md.Failure scenario: an agent picks up #238 and adds unnecessary worker-ID DB naming to conftest, justified by a lock that was never reproduced. An agent picks up #240 and batches inserts, which the doc warns would lose
record_event_async's counter and telemetry semantics (line 107), to chase a saving the measurements contradict. Every handoff link is broken.Fix: rewrite the #238 and #240 briefs against the measured findings, or return them to needs-triage. Fix the doc path in all four issues, or rename the doc to the brief's
217-test-suite-speed.md.P2-2. Setting
DATABASE_URLbypasses the #210 sandbox: tests and the import-timeinit_db()write to the configured database. CONFIRMED (probe).tests/conftest.py:17-28overrides onlyDATABASE_PATHandsettings.db_path.app/db/connection.py:46resolvesurl or settings.database_url or settings.db_path, soDATABASE_URL, a documented setting (ADR 0003), wins. #243's fixture already handles this case withsettings.database_url = None(conftest.py:151), but the session bootstrap does not.Probe:
DATABASE_URL=sqlite:///<scratch>/fake_prod.db pytest tests/<probe>.py. The target wasfake_prod.db, and the file was created and seeded with schema and users (296 KB). Any shell,.envor host that setsDATABASE_URLtherefore runs the whole suite against that database. On a deployment host that means the real one: test users are seeded, and occupancy tables are deleted byoccupancy_reset.pyafter every test.This gap was already on master, but #210's contract ("tests stay inside their sandbox") is this PR's deliverable, and the PR now runs
init_db()at import time. Fix: in the bootstrap, popDATABASE_URLfromos.environand setsettings.database_url = None, then add a regression test.P3
P3-1. The final numbers describe an older tree, and the acceptance artefacts are missing. CONFIRMED.
The acceptance criterion asks for "the full suite's final wall time and --durations=25 report ... in the doc and the PR". The doc's only full-suite figures are for
ffee469(539 tests). The head4abfff3runs 604 tests: the PR body says 604, but fix reply 4419 says 610, which is inconsistent. There is no--durations=25report in the doc or the PR;--durations=0logs exist only gzipped in the evidence directory.The brief also requires "an otherwise idle machine". The serial baseline (219.85 s at load 10-22) and after (198.32 s at load 22→9) medians are load-confounded, which the doc admits. The serial before/after delta is therefore not evidence of a quick-win saving. The per-test #216 numbers are robust: offline here they took 0.16 s each.
P3-2. The
test_review_script.pyskip is module-wide, and 36 of the 38 tests don't need git. CONFIRMED.tests/test_review_script.py:10-15skips the module whenROOT/.gitis missing. With the skip disabled in the export, 36 tests pass. Onlytest_wt_review_forwards_arguments_from_nested_directoryandtest_wt_ignores_a_regular_scripts_package_on_pythonpathfail ("not a git repository"). Skip those two instead, so archive-based runs (#235's remote and snapshot mechanisms, and D14's immutable snapshots) keep the coverage. Also, a checkout nested in another repo would have git available yet still skip.P3-3. The doc's policy text contradicts the confirmed decisions it says it is aligned with. CONFIRMED.
Add a pointer to c4305 and #265, and mark these lines as superseded.
P3-4. The PR body's claim that production behaviour is unchanged is inaccurate for
db_sync_service. PLAUSIBLE.app/services/db_sync_service.py:95,356moves the snapshot and staging directories from<cwd>/data/...todirname(settings.db_path)/.... On a host withDATABASE_PATHoutside./data, snapshots now land next to the DB. That is probably desirable, but it is a production change the PR body says doesn't happen. It also still ignoresDATABASE_URL, which ties in with P2-2. Mention it in the PR body.P3-5. #269's cause is described loosely. CONFIRMED (text only).
Doc line 206 says the test fails "when preceded by door tests due to lingering lookup/retry state". #269 names one specific predecessor,
test_forced_open_with_card_keeps_forced_trigger, and says the leaked state is not yet identified. Use the issue's wording so the doc doesn't assert an unverified cause.Verdict
The research and quick wins deliver the #217 brief, and the pass-1 fixes are real. Before merge:
DATABASE_URLhole in the session bootstrap (P2-2).Pass 2 fixes (
8d6e3d0)Addressed the required pass-2 findings from r58:
Validation: full suite passed (613 tests); the final existing-artifact regression checks passed; Ruff check and format check passed; all pre-commit hooks, including the full suite, passed. No checkout data/ or logs/ artifacts appeared. Working tree is clean after commit.
Remaining P3s are deferred to #272, as requested. Please run review pass 3 on
8d6e3d0.Code review, pass 3 (
origin/master...8d6e3d0, spec #216 + #210)Summary: 2 P2s. Four of the five r58 must-fixes are fixed, and the
DATABASE_URLone is only partial. Fix the P2s, then request pass 4. The new P3s are added to #272.Checks on a git-archive export of
8d6e3d0. Every run usedGIT_*unset and never had its cwd in the real checkout.5a02cd9(which includes #222) is clean. On the merged tree, test_sandbox_isolation, test_occupancy and test_next_day_settings_activation pass.Must fix:
scripts/check_docs.pystill letsDATABASE_URLfrom.envoverride its temporary DB. Both axes confirmed this.check_docs.py:17-24sets onlyDATABASE_PATHbeforeinit_db(). Settings reads<repo>/.envby absolute path (app/config.py:6,11), anddatabase_urltakes precedence (connection.py:46)..env(scripts/wt:14), and CI runscheck_docs.pydirectly (ci.yml:38).DATABASE_URL=sqlite:///<dir>/cd_leak.dbin the export's.env.test_check_docs_creates_no_database_in_checkoutpasses but creates cd_leak.db: 296 KB, migrated, with users admin, operator and viewer seeded..envsetsDATABASE_URL(documented in ADR 0003), every pytest run, including pre-commit, migrates and seeds that real database. The snapshot check can't see it when the URL points outside the checkout.check_docs.py,os.environ.pop("DATABASE_URL", None), then either setDATABASE_URLto the temporary DB or setsettings.database_url = Nonebeforeinit_db()..envcase to the check_docs test.pytest_sessionfinish(tests/conftest.py:52-54) crashes pytest and loses the run's report.logs/new.logand another runsassert False. The output is.F, then a raw pluggy traceback ending inAssertionError: Test session changed checkout data/ or logs/ artifacts. There is no failure report and no summary.pytest_terminal_summaryand setsession.exitstatus = pytest.ExitCode.TESTS_FAILED, or move the check into a session-scoped autouse fixture's teardown so it appears as a normal error.Standards axis
DATABASE_URLbypass:24,:30) is fixed: with the variable set in either the environment or.env,test_viewer_rolecreates no file, and a fresh-process regression covers both.check_docs.pyis still open (P2-1).conftest.py:157-174patchesaudit_loggerplus all 7 consumers.git grepfinds no other import site.data/hikcentral.dbandlogs/lifecycle.lognow pass, where r58 had 2 failures.Subprocess hygiene is fine:
no_git_hook_envdropsGIT_*before the new subprocess test copiesos.environ.New P3s (added to #272):
conftest.py:159-168), so a new CLI module goes unguarded. Redirect at the source with a base-dir setting.data/hikcentral.db-wal. Ignore*-waland*-shm, or name concurrent writers in the message. Also catchFileNotFoundErrorduring the walk.Spec axis
DATABASE_URLbypasscheck_docs.log_lifecycle_eventthrough each of the 7 modules from the repo root creates nologs/.logs/app.logis caught.4abfff3, which survives a merge commit. #240 is needs-triage.Acceptance criteria:
--durations=25report is already in #272.check_docsexits 0 with no "Error loading doors" and creates nodata/orlogs/.wt pruneworks.New P3s (added to #272):
test_worktree_has_no_test_database_or_lifecycle_logs(test_sandbox_isolation.py:39) now allows existing artifacts. Rename it, for example totest_session_leaves_checkout_artifacts_unchanged.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.