test: pass-2 P3 follow-ups from PR #218 (sandbox harness and research doc polish) #272

New issue

Open

opened 2026-10-03 18:04:00 +00:00 by gabogg · 0 comments

gabogg commented

2026-10-03 18:04:00 +00:00

Owner

Follow-ups from review pass 2 of PR #218 (see the review on that PR). None blocks Milestone 3's delivery, so this issue stays outside it.

Harness (tests/conftest.py and related files):

The check-and-fail bodies in guarded_connect and guarded_connect_ex are duplicated (conftest.py:74-90). Extract one helper.
The _orig_db_path restore is dead code (conftest.py:23-24, 43-44): it is read after DATABASE_PATH is already set.
tests/test_review_script.py skips the whole module outside git. Only test_wt_review_forwards_arguments_from_nested_directory and test_wt_ignores_a_regular_scripts_package_on_pythonpath need git, so skip just those two and keep the other 36 running in git-archive exports.
The network guard is installed in a session fixture, so connects made at collection time are unguarded (plausible, not probed).
cmd_doctor.py:182-186 resolves the DB directory against the cwd instead of the repo root (plausible).

Research doc (docs/research/217-test-performance-and-agent-workflows.md):

The final full-suite numbers describe ffee469 (539 tests), not the merged head. The brief's --durations=25 report is missing, and the serial before/after medians are load-confounded, so present them as such.
Line 318 calls #242 "awaiting maintainer decision", but D4 decided it. Lines 286-287 recommend a host admission limit that D3/D15 don't endorse. Link the decision record (#217 comment 4305) and #265, and mark those lines superseded.
Line 206 states a cause for #269's flake; use the issue's wording instead (the predecessor is test_forced_open_with_card_keeps_forced_trigger, and the leaked state is not yet identified).
The PR body says production behaviour is unchanged, but db_sync_service now places snapshots and staging next to settings.db_path. Note this in the CHANGELOG or release notes.

Acceptance criteria

Each item above is fixed or explicitly declined with a reason.

Follow-ups from review pass 2 of PR #218 (see the review on that PR). None blocks Milestone 3's delivery, so this issue stays outside it. Harness (`tests/conftest.py` and related files): - The check-and-fail bodies in `guarded_connect` and `guarded_connect_ex` are duplicated (conftest.py:74-90). Extract one helper. - The `_orig_db_path` restore is dead code (conftest.py:23-24, 43-44): it is read after DATABASE_PATH is already set. - `tests/test_review_script.py` skips the whole module outside git. Only `test_wt_review_forwards_arguments_from_nested_directory` and `test_wt_ignores_a_regular_scripts_package_on_pythonpath` need git, so skip just those two and keep the other 36 running in git-archive exports. - The network guard is installed in a session fixture, so connects made at collection time are unguarded (plausible, not probed). - `cmd_doctor.py:182-186` resolves the DB directory against the cwd instead of the repo root (plausible). Research doc (`docs/research/217-test-performance-and-agent-workflows.md`): - The final full-suite numbers describe ffee469 (539 tests), not the merged head. The brief's `--durations=25` report is missing, and the serial before/after medians are load-confounded, so present them as such. - Line 318 calls #242 "awaiting maintainer decision", but D4 decided it. Lines 286-287 recommend a host admission limit that D3/D15 don't endorse. Link the decision record (#217 comment 4305) and #265, and mark those lines superseded. - Line 206 states a cause for #269's flake; use the issue's wording instead (the predecessor is `test_forced_open_with_card_keeps_forced_trigger`, and the leaked state is not yet identified). - The PR body says production behaviour is unchanged, but `db_sync_service` now places snapshots and staging next to `settings.db_path`. Note this in the CHANGELOG or release notes. ## Acceptance criteria - [ ] Each item above is fixed or explicitly declined with a reason.

gabogg added the

ready-for-agent

low-priority

labels

2026-10-03 18:04:00 +00:00

gabogg referenced this issue

2026-10-03 18:16:29 +00:00

fix(tests): keep tests inside their sandbox and cut suite time #218