refactor(tests): tidy the occupancy test-reset seed helpers #264

New issue

Open

opened 2026-10-03 12:03:16 +00:00 by gabogg · 1 comment

gabogg commented

2026-10-03 12:03:16 +00:00

Owner

P3 follow-up from PR #233 review pass 2 (standards axis).

Finding

Duplicated Code. The config seed (occupancy_repository.py ~478-479) hardcodes the trust_* literals (10, 0.35, 0.80, 1.30, 5000, 1000), which also appear in the ALTER defaults (database.py ~243) and in the get_config_sync fallback. It sets only some of the trust and statistics columns; trust_negative_net_limit, trust_spike_* and statistics_min_comparison_coverage fall back to column defaults. List all of them, or rely on column defaults for all of them.
Middle Man. The module-level seed_default_occupancy_config (~3363) and reset_config_to_defaults_sync (~501) only forward to seed_default_config. The production repository also carries a destructive INSERT OR REPLACE reset that only tests call. Consider moving the reset into the test helper module.
Stale rows. reset_daily_schedules_sync(conn=...) (~887) reads its return value through a new connection before the caller commits, so under WAL it returns the old schedule. Reuse conn.
Primitive Obsession. DEFAULT_DAILY_SCHEDULE is a positional 5-tuple indexed with day[0], and its hours are bare strings even though OpeningHours exists.
Undocumented hook. PYTEST_SHUFFLE_SEED (tests/conftest.py ~72-84) is undocumented, imports random inside the function, and overlaps #217/#218. Document it in the test docs, or fold it into the #218 test-loop work.

Acceptance Criteria

One source for the default config values, shared by schema defaults, the seed and the fallback.
No forwarding-only wrappers, and no test-only destructive reset in production code (or a documented reason for keeping it).
The reset returns committed rows.
PYTEST_SHUFFLE_SEED is documented, or owned by #218.

P3 follow-up from PR #233 review pass 2 (standards axis). ### Finding - **Duplicated Code.** The config seed (`occupancy_repository.py` ~478-479) hardcodes the `trust_*` literals (`10, 0.35, 0.80, 1.30, 5000, 1000`), which also appear in the `ALTER` defaults (`database.py` ~243) and in the `get_config_sync` fallback. It sets only some of the trust and statistics columns; `trust_negative_net_limit`, `trust_spike_*` and `statistics_min_comparison_coverage` fall back to column defaults. List all of them, or rely on column defaults for all of them. - **Middle Man.** The module-level `seed_default_occupancy_config` (~3363) and `reset_config_to_defaults_sync` (~501) only forward to `seed_default_config`. The production repository also carries a destructive `INSERT OR REPLACE` reset that only tests call. Consider moving the reset into the test helper module. - **Stale rows.** `reset_daily_schedules_sync(conn=...)` (~887) reads its return value through a new connection before the caller commits, so under WAL it returns the old schedule. Reuse `conn`. - **Primitive Obsession.** `DEFAULT_DAILY_SCHEDULE` is a positional 5-tuple indexed with `day[0]`, and its hours are bare strings even though `OpeningHours` exists. - **Undocumented hook.** `PYTEST_SHUFFLE_SEED` (`tests/conftest.py` ~72-84) is undocumented, imports `random` inside the function, and overlaps #217/#218. Document it in the test docs, or fold it into the #218 test-loop work. ### Acceptance Criteria - One source for the default config values, shared by schema defaults, the seed and the fallback. - No forwarding-only wrappers, and no test-only destructive reset in production code (or a documented reason for keeping it). - The reset returns committed rows. - `PYTEST_SHUFFLE_SEED` is documented, or owned by #218.

gabogg added the

ready-for-agent

enhancement

labels

2026-10-03 12:03:16 +00:00

gabogg referenced this issue

2026-10-03 12:29:22 +00:00

fix(tests): test order dependence between test_occupancy and test_calibration_reconciliation #233

gabogg referenced this issue

2026-10-03 12:29:26 +00:00

fix(tests): test order dependence between test_occupancy and test_calibration_reconciliation #233

gabogg commented

2026-10-03 12:56:24 +00:00

Author

Owner

Adding scope from PR #233 review pass 3 (P3). tests/occupancy_reset.py:11 checks only that occupancy_config exists before deleting from six other tables. A test that points the database at a partly initialized schema would crash in teardown. Guard each table, or check the whole set.

Adding scope from PR #233 review pass 3 (P3). `tests/occupancy_reset.py:11` checks only that `occupancy_config` exists before deleting from six other tables. A test that points the database at a partly initialized schema would crash in teardown. Guard each table, or check the whole set.

gabogg referenced this issue

2026-10-03 13:10:44 +00:00

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

gabogg referenced this issue from a commit

2026-10-03 13:12:37 +00:00

fix(tests): skip review tests outside git repo and note order issues (#264, #269)

gabogg referenced this issue

2026-10-03 13:12:55 +00:00

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

gabogg referenced this issue

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

perf(tests): evaluate pytest-xdist safe parallelism with isolated worker databases #238