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

Merged
gabogg merged 6 commits from fix/test-order-dependence-221 into master 2026-10-03 12:58:15 +00:00
Owner

Summary

Fixes #221: occupancy tests left daily_reset_time='04:00', calibration configuration, weekly schedules and singleton state behind, causing the later quiet-window calibration test to skip calibration at 03:45.

The conftest autouse fixture now owns occupancy cleanup across the suite. It resets the manager before each test and restores configuration, weekly schedules, holidays, events, frozen business-day schedules and their audits after each test. The shared reset lives in tests/occupancy_reset.py; neither tests nor fixtures import tests.conftest.

Frozen schedule rows must be deleted, rather than merely clearing is_holiday: a retained closed day wins over the next test's default schedule through INSERT OR IGNORE. The regression runs a fixed two-test child pytest session using the actual conftest fixtures: the first test stamps a closed exception with custom hours, the second freezes and resolves the same date with default hours and a fresh audit. The outer suite can shuffle without weakening that proof. The old test_step1/test_step2 pair and unused imports are removed.

Closes #221. Deferred pass-2 P3 findings are tracked in #264. Merge order remains #233 first, then #218 adopts the shared reset.

Architectural Impact

  • OccupancyManager.__init__ delegates transient state initialization to reset().
  • Configuration restoration and schema initialization share seed_default_config; weekly schedules share DEFAULT_DAILY_SCHEDULE. Only the synchronous repository reset helpers remain.
  • Conftest is the sole fixture owner; tests/occupancy_reset.py provides the plain helper for both fixture cleanup and explicit regression coverage. isolated_repository_db remains unchanged.
  • No new schema or HTTP interfaces. Merged current origin/master (9a72ff4) before verification.

Verification / Test Evidence

  • Before the pass-2 deletion fix, the explicit reset regression failed with one frozen schedule remaining, and the child session passed the stamping test then failed the second test's fresh-freeze assertion. Both pass with the fix.
  • Original #221 pair plus reset regressions: 25 passed.
  • ruff check ., ruff format --check ., and python scripts/check_docs.py pass (43 Markdown files, 90 HTTP operations).
  • Normal-order full suite: 604 passed in 154.02 seconds.
  • Pre-commit hooks are installed. The all-files run exposed existing whitespace/newline problems in eight unrelated files; those automatic edits were reverted to keep this PR scoped. Its pytest hook nevertheless passed all 604 tests. Scoped commit-hook results are recorded below.
  • Full-suite shuffles via the PYTEST_SHUFFLE_SEED collection hook: seed 233 passed 604/604 in 135.34 seconds; seed 264 passed 603/604 in 136.22 seconds, with the unrelated cardholder failure below. The earlier concurrently launched runs exited 137 and are excluded from these results.

Other order dependence: found. Shuffled the complete 604-test Python pytest collection (all files under tests, including frontend tests exercised through the Python suite), with seeds 233 and 264. No failures with 233; 264 fails tests/test_cardholder_name_resolution.py::test_failed_lookup_is_retried_after_a_pause_not_every_poll: calls are ['69', '67'] instead of ['67']. This also reproduces with only test_forced_open_with_card_keeps_forced_trigger followed by test_failed_lookup_is_retried_after_a_pause_not_every_poll (1 passed, 1 failed in 0.69 seconds). Earlier unresolved person 69 survives in door/cycle state. This finding is separate from occupancy cleanup; it has not been changed in this PR. Each shuffle treats the new reset regression as one collected test; its child pytest session deliberately runs the stamping/default-resolution pair in fixed order.

Checklist

  • Centralize fixture-owned cleanup and share default configuration/schedule seeds.
  • P2-A: move reset to a plain helper; remove all tests.conftest imports.
  • P2-B: delete frozen schedules and their audit rows alongside holidays/events and their audits.
  • Add direct cleanup assertions and a non-vacuous two-test isolation regression; demonstrate failures before the fix and passes afterward.
  • Remove the order-dependent step pair and unused imports.
  • Merge origin/master and report exact shuffle scope, seeds and additional findings.
  • Link deferred pass-2 P3 cleanup to #264.
  • Pass 3 review requested; do not merge until required review gates pass.

Scoped commit hooks passed at 1885b27: trailing whitespace, final newlines, ruff lint/format and the full pytest suite. Pass-2 fix reply maps every finding below the review.

## Summary Fixes #221: occupancy tests left `daily_reset_time='04:00'`, calibration configuration, weekly schedules and singleton state behind, causing the later quiet-window calibration test to skip calibration at 03:45. The conftest autouse fixture now owns occupancy cleanup across the suite. It resets the manager before each test and restores configuration, weekly schedules, holidays, events, frozen business-day schedules and their audits after each test. The shared reset lives in `tests/occupancy_reset.py`; neither tests nor fixtures import `tests.conftest`. Frozen schedule rows must be deleted, rather than merely clearing `is_holiday`: a retained closed day wins over the next test's default schedule through `INSERT OR IGNORE`. The regression runs a fixed two-test child pytest session using the actual conftest fixtures: the first test stamps a closed exception with custom hours, the second freezes and resolves the same date with default hours and a fresh audit. The outer suite can shuffle without weakening that proof. The old `test_step1`/`test_step2` pair and unused imports are removed. Closes #221. Deferred pass-2 P3 findings are tracked in #264. Merge order remains #233 first, then #218 adopts the shared reset. ## Architectural Impact - `OccupancyManager.__init__` delegates transient state initialization to `reset()`. - Configuration restoration and schema initialization share `seed_default_config`; weekly schedules share `DEFAULT_DAILY_SCHEDULE`. Only the synchronous repository reset helpers remain. - Conftest is the sole fixture owner; `tests/occupancy_reset.py` provides the plain helper for both fixture cleanup and explicit regression coverage. `isolated_repository_db` remains unchanged. - No new schema or HTTP interfaces. Merged current `origin/master` (9a72ff4) before verification. ## Verification / Test Evidence - Before the pass-2 deletion fix, the explicit reset regression failed with one frozen schedule remaining, and the child session passed the stamping test then failed the second test's fresh-freeze assertion. Both pass with the fix. - Original #221 pair plus reset regressions: 25 passed. - `ruff check .`, `ruff format --check .`, and `python scripts/check_docs.py` pass (43 Markdown files, 90 HTTP operations). - Normal-order full suite: 604 passed in 154.02 seconds. - Pre-commit hooks are installed. The all-files run exposed existing whitespace/newline problems in eight unrelated files; those automatic edits were reverted to keep this PR scoped. Its pytest hook nevertheless passed all 604 tests. Scoped commit-hook results are recorded below. - Full-suite shuffles via the `PYTEST_SHUFFLE_SEED` collection hook: seed `233` passed 604/604 in 135.34 seconds; seed `264` passed 603/604 in 136.22 seconds, with the unrelated cardholder failure below. The earlier concurrently launched runs exited 137 and are excluded from these results. **Other order dependence: found.** Shuffled the complete 604-test Python pytest collection (all files under `tests`, including frontend tests exercised through the Python suite), with seeds `233` and `264`. No failures with `233`; `264` fails `tests/test_cardholder_name_resolution.py::test_failed_lookup_is_retried_after_a_pause_not_every_poll`: calls are `['69', '67']` instead of `['67']`. This also reproduces with only `test_forced_open_with_card_keeps_forced_trigger` followed by `test_failed_lookup_is_retried_after_a_pause_not_every_poll` (1 passed, 1 failed in 0.69 seconds). Earlier unresolved person 69 survives in door/cycle state. This finding is separate from occupancy cleanup; it has not been changed in this PR. Each shuffle treats the new reset regression as one collected test; its child pytest session deliberately runs the stamping/default-resolution pair in fixed order. ## Checklist - [x] Centralize fixture-owned cleanup and share default configuration/schedule seeds. - [x] P2-A: move reset to a plain helper; remove all `tests.conftest` imports. - [x] P2-B: delete frozen schedules and their audit rows alongside holidays/events and their audits. - [x] Add direct cleanup assertions and a non-vacuous two-test isolation regression; demonstrate failures before the fix and passes afterward. - [x] Remove the order-dependent step pair and unused imports. - [x] Merge origin/master and report exact shuffle scope, seeds and additional findings. - [x] Link deferred pass-2 P3 cleanup to #264. - [ ] Pass 3 review requested; do not merge until required review gates pass. Scoped commit hooks passed at `1885b27`: trailing whitespace, final newlines, ruff lint/format and the full pytest suite. Pass-2 fix reply maps every finding below the review.
fix(tests): reset occupancy config, schedule, and singleton state between tests
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m45s
1ec6a327b8
Tests in tests/test_occupancy.py (specifically test_config_daily_reset_and_guard_fields_api
and test_transit_bleed_daytime_and_nocturnal) mutate occupancy_config.daily_reset_time
to '04:00' and calibration_mode to 'FLAT_OFFSET', as well as mutating daily schedules.
In the shared session SQLite database, these mutations persisted after test_occupancy.py
completed. When tests/test_calibration_reconciliation.py::test_auto_calibration_in_quiet_window
subsequently ran, its evaluation at 03:45 fell inside the straddling quiet window (03:30-04:30)
before the 04:00 reset, causing the auto-calibration guard to skip execution because the cycle
had not yet completed.

Reset the state at its source:
- Add reset() to OccupancyManager to clear transient cache and date guard state.
- Add reset_config_to_defaults_sync and reset_daily_schedules_sync to OccupancyRepository.
- Update setup_test_db in tests/test_occupancy.py to reset counting events, cameras,
  calibration logs, holidays, config, schedules, and singleton state on setup and teardown.
- Add reset_occupancy_state_in_tests autouse fixture in tests/conftest.py so singleton and
  configuration state are reliably restored across all test runs.

Closes #221.
gabogg changed title from WIP: fix(tests): test order dependence between test_occupancy and test_calibration_reconciliation to fix(tests): test order dependence between test_occupancy and test_calibration_reconciliation 2026-10-03 07:46:09 +00:00
Author
Owner

First code push is committed and pushed (1ec6a32). PR #233 has been promoted from draft to ready.

Summary of changes

  1. app/services/occupancy_service.py: Added OccupancyManager.reset() to reset in-memory caches, rate trackers, and _last_auto_calib_date guard state.
  2. app/db/occupancy_repository.py: Added reset_config_to_defaults_sync / _async and reset_daily_schedules_sync / _async to restore occupancy_config row 1 and occupancy_daily_schedule rows to pristine system defaults without inline SQL at call sites.
  3. tests/test_occupancy.py: Updated setup_test_db autouse fixture to reset counting events, cameras, calibration logs, holidays, configuration, schedules, and singleton state on both setup and teardown (yield).
  4. tests/conftest.py: Added reset_occupancy_state_in_tests autouse fixture so singleton and configuration state are cleanly reset across all tests in the session.

Verification

  • pytest tests/test_occupancy.py tests/test_calibration_reconciliation.py: 23 passed
  • pytest tests/test_calibration_reconciliation.py tests/test_occupancy.py: 23 passed
  • pytest tests/test_calibration_reconciliation.py::test_auto_calibration_in_quiet_window: 1 passed
  • pytest: 534 passed (100% green)
  • ruff check . & ruff format .: clean
  • python3 scripts/check_docs.py: passed

Ready for first pass review.

First code push is committed and pushed (`1ec6a32`). PR #233 has been promoted from draft to ready. ### Summary of changes 1. **`app/services/occupancy_service.py`**: Added `OccupancyManager.reset()` to reset in-memory caches, rate trackers, and `_last_auto_calib_date` guard state. 2. **`app/db/occupancy_repository.py`**: Added `reset_config_to_defaults_sync` / `_async` and `reset_daily_schedules_sync` / `_async` to restore `occupancy_config` row 1 and `occupancy_daily_schedule` rows to pristine system defaults without inline SQL at call sites. 3. **`tests/test_occupancy.py`**: Updated `setup_test_db` autouse fixture to reset counting events, cameras, calibration logs, holidays, configuration, schedules, and singleton state on both setup and teardown (`yield`). 4. **`tests/conftest.py`**: Added `reset_occupancy_state_in_tests` autouse fixture so singleton and configuration state are cleanly reset across all tests in the session. ### Verification - `pytest tests/test_occupancy.py tests/test_calibration_reconciliation.py`: 23 passed - `pytest tests/test_calibration_reconciliation.py tests/test_occupancy.py`: 23 passed - `pytest tests/test_calibration_reconciliation.py::test_auto_calibration_in_quiet_window`: 1 passed - `pytest`: 534 passed (100% green) - `ruff check .` & `ruff format .`: clean - `python3 scripts/check_docs.py`: passed Ready for first pass review.
gabogg left a comment

Code review, pass 1 (origin/master...1ec6a32, spec #221)

Result: 2 P2s and 9 P3s. Not mergeable yet. This is pass 1, so fix every finding, merge origin/master (now b64e805 with #178) and request a second pass.

  • Good news: the root cause is fixed, not masked.
    • On 016a012, running pytest tests/test_occupancy.py tests/test_calibration_reconciliation.py fails test_auto_calibration_in_quiet_window.
    • At 1ec6a32, both orders pass (23 of 23). Nothing is skipped, marked xfail or reordered, and the test itself is unchanged.
    • The PR names the leaked state: daily_reset_time='04:00', calibration_mode, schedules and holidays.
    • It merges with master and with #218 (fix/test-isolation) without textual conflicts.

Spec

P2

  • P2-1. The global reset silently swallows its own failure (tests/conftest.py:73-76, except Exception: pass; raised on both axes). If schema drift or a locked DB breaks the reset, every later test inherits dirty config with no signal, which brings back the order dependence #221 is about. Remove the except, and let it raise.
  • P2-2. After #178, the reset misses the new holiday state (plausible). #178 copies holidays into occupancy_business_day_schedules.is_holiday and occupancy_holiday_audit, and adds occupancy_events. tests/test_occupancy.py:21 deletes only occupancy_holidays, so after the master merge stale is_holiday flags and events can leak between tests.
    • Fix: after merging master, reset those tables too.
    • Prove it: a test that creates a holiday must leave no holiday flag behind for the next test.

P3

  • P3-1. Other order dependence isn't reported. The spec says "Other order-dependence not shown by this pair. Note any found in the PR." The PR notes none.
    • The global fixture resets only config and the manager.
    • These files write config, schedules or holidays with no reset: test_business_day_schedules.py, test_occupancy_proportional_calibration.py and test_statistics_*.
    • Fix: run the suite shuffled (for example -p randomly with 2 or 3 seeds, or a reversed collection), and report what you find.
  • P3-2. Two new helpers have no callers: reset_config_to_defaults_async (occupancy_repository.py:476) and reset_daily_schedules_async (:871). Delete them (raised on both axes).

Standards

P3

  • P3-3. The default seed is duplicated (smell: Duplicated Code; raised on both axes). reset_config_to_defaults_sync (occupancy_repository.py:429-474) and reset_daily_schedules_sync (:848-869) copy the init_db seed literals from app/db/database.py:276-326, including the Fri/Sat "08:00-22:00" and Sun "09:00-20:00" schedules. If one default changes, the test reset quietly drifts from a fresh DB. Extract one DEFAULT_DAILY_SCHEDULE and one config-seed helper, used by both init_db and the reset.
  • P3-4. The reset is split between two owners (Shotgun Surgery). The conftest fixture resets config, while test_occupancy.py resets config plus schedules. Make conftest the single owner: reset config, schedules and holiday state there, and drop the per-module copy.
  • P3-5. OccupancyManager.reset() repeats the __init__ field list (occupancy_service.py:103-140). A new field could be missed. Have __init__ call reset().
  • P3-6. import asyncio sits inside function bodies (occupancy_repository.py:478, :873). Move it to the module top. This goes away if P3-2 deletes the helpers.
  • P3-7. conn: Any | None is typed Any (occupancy_repository.py:429). Use sqlite3.Connection | None.
  • P3-8. Every test now does an extra DB write. Keep it cheap, and state the measured overhead in the PR, since #218 is about suite time.
  • P3-9. Coordinate with #218. #218 rewrites isolated_test_db and runs init_db() when conftest is imported. Agree there on a single place where test state is reset.
## Code review, pass 1 (`origin/master...1ec6a32`, spec #221) Result: **2 P2s and 9 P3s. Not mergeable yet.** This is pass 1, so fix every finding, merge `origin/master` (now `b64e805` with #178) and request a **second pass**. - **Good news:** the root cause is fixed, not masked. - On `016a012`, running `pytest tests/test_occupancy.py tests/test_calibration_reconciliation.py` fails `test_auto_calibration_in_quiet_window`. - At `1ec6a32`, both orders pass (23 of 23). Nothing is skipped, marked xfail or reordered, and the test itself is unchanged. - The PR names the leaked state: `daily_reset_time='04:00'`, `calibration_mode`, schedules and holidays. - It merges with master and with #218 (`fix/test-isolation`) without textual conflicts. ## Spec ### P2 - **P2-1. The global reset silently swallows its own failure** (`tests/conftest.py:73-76`, `except Exception: pass`; raised on both axes). If schema drift or a locked DB breaks the reset, every later test inherits dirty config with no signal, which brings back the order dependence #221 is about. Remove the `except`, and let it raise. - **P2-2. After #178, the reset misses the new holiday state** (plausible). #178 copies holidays into `occupancy_business_day_schedules.is_holiday` and `occupancy_holiday_audit`, and adds `occupancy_events`. `tests/test_occupancy.py:21` deletes only `occupancy_holidays`, so after the master merge stale `is_holiday` flags and events can leak between tests. - Fix: after merging master, reset those tables too. - Prove it: a test that creates a holiday must leave no holiday flag behind for the next test. ### P3 - **P3-1. Other order dependence isn't reported.** The spec says *"Other order-dependence not shown by this pair. Note any found in the PR."* The PR notes none. - The global fixture resets only config and the manager. - These files write config, schedules or holidays with no reset: `test_business_day_schedules.py`, `test_occupancy_proportional_calibration.py` and `test_statistics_*`. - Fix: run the suite shuffled (for example `-p randomly` with 2 or 3 seeds, or a reversed collection), and report what you find. - **P3-2. Two new helpers have no callers:** `reset_config_to_defaults_async` (`occupancy_repository.py:476`) and `reset_daily_schedules_async` (`:871`). Delete them (raised on both axes). ## Standards ### P3 - **P3-3. The default seed is duplicated** (smell: Duplicated Code; raised on both axes). `reset_config_to_defaults_sync` (`occupancy_repository.py:429-474`) and `reset_daily_schedules_sync` (`:848-869`) copy the `init_db` seed literals from `app/db/database.py:276-326`, including the Fri/Sat "08:00-22:00" and Sun "09:00-20:00" schedules. If one default changes, the test reset quietly drifts from a fresh DB. Extract one `DEFAULT_DAILY_SCHEDULE` and one config-seed helper, used by both `init_db` and the reset. - **P3-4. The reset is split between two owners** (Shotgun Surgery). The conftest fixture resets config, while `test_occupancy.py` resets config plus schedules. Make conftest the single owner: reset config, schedules and holiday state there, and drop the per-module copy. - **P3-5. `OccupancyManager.reset()` repeats the `__init__` field list** (`occupancy_service.py:103-140`). A new field could be missed. Have `__init__` call `reset()`. - **P3-6. `import asyncio` sits inside function bodies** (`occupancy_repository.py:478`, `:873`). Move it to the module top. This goes away if P3-2 deletes the helpers. - **P3-7. `conn: Any | None` is typed `Any`** (`occupancy_repository.py:429`). Use `sqlite3.Connection | None`. - **P3-8. Every test now does an extra DB write.** Keep it cheap, and state the measured overhead in the PR, since #218 is about suite time. - **P3-9. Coordinate with #218.** #218 rewrites `isolated_test_db` and runs `init_db()` when conftest is imported. Agree there on a single place where test state is reset.
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Merge origin/master into fix/test-order-dependence-221
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m16s
b596985a40
Author
Owner

Pass 1 fixes (b596985)

Addressed every finding in review r39. Merged origin/master (7f17d00 with PR #243) in b596985; fixes are in 37438ed.

Finding Fix
P2-1 (Spec & Standards: global reset silently swallows failure) Removed the bare except Exception: pass block in tests/conftest.py. Failures in reset_test_occupancy_state() will now raise immediately to prevent silent state leakage.
P2-2 (Spec: reset misses new holiday state after #178) Updated reset_test_occupancy_state() in tests/conftest.py to reset occupancy_business_day_schedules.is_holiday = 0, delete all rows from occupancy_holidays, occupancy_holiday_audit, occupancy_events, and occupancy_event_audit. Added tests/test_holiday_state_reset.py proving that holiday and event state created in one test does not carry over to subsequent tests.
P3-1 (Spec: other order dependence not reported) Added pytest_collection_modifyitems hook in tests/conftest.py controlled by PYTEST_SHUFFLE_SEED (supporting random seeds and "reverse"). Tested suite and subsets with reversed order and multiple random seeds (e.g. seed 42) across test_business_day_schedules.py, test_occupancy_proportional_calibration.py, test_occupancy.py, test_calibration_reconciliation.py, and test_holiday_state_reset.py. All tests passed cleanly without order dependence.
P3-2 (Spec & Standards: unused async helpers) Removed uncalled methods reset_config_to_defaults_async and reset_daily_schedules_async from app/db/occupancy_repository.py.
P3-3 (Standards: duplicated default seed literals) Extracted DEFAULT_DAILY_SCHEDULE into app/schemas/occupancy_models.py and seed_default_occupancy_config into app/db/occupancy_repository.py. Unified init_schema() in app/db/database.py and repository reset methods to share the same constants and seed logic.
P3-4 (Standards: shotgun surgery in reset ownership) Centralized all state restoration inside tests/conftest.py (reset_test_occupancy_state and reset_occupancy_state_in_tests autouse fixture). Cleaned up tests/test_occupancy.py's setup_test_db to only delete test-specific event records and cameras, eliminating duplicate reset logic.
P3-5 (Standards: OccupancyManager field repetition) Updated OccupancyManager.__init__ in app/services/occupancy_service.py to call self.reset(), keeping field initialization and reset in a single place.
P3-6 (Standards: local asyncio imports) Resolved by removing the unused async helpers from app/db/occupancy_repository.py.
P3-7 (Standards: connection type annotation) Changed `conn: Any
P3-8 (Standards: extra DB write overhead) Benchmarked reset_test_occupancy_state(): averages ~3.65 ms per test execution on temporary SQLite DB, resulting in ~2.0 seconds total overhead across the entire 566-test suite.
P3-9 (Standards: coordination with #218 & #243) Maintained full compatibility with the opt-in isolated_repository_db fixture added in PR #243 (00a9b28) and merged via origin/master (7f17d00). Did not duplicate or rename isolated_repository_db.

How the conftest reset and isolated_repository_db fixture work together

  1. Orthogonal responsibilities: isolated_repository_db is an opt-in fixture designed for test modules that require a dedicated, separate SQLite database file on disk (e.g. tests/test_business_cycle_labels.py). It uses monkeypatch to set settings.database_url = None, redirect settings.db_path to tmp_path / "repository.db", and invoke init_db().
  2. Global lifecycle hygiene: reset_occupancy_state_in_tests is an autouse fixture running for every test in the suite. At test setup, it clears in-memory singleton caches and rate trackers via occupancy_manager.reset(). At test teardown (yield), it calls reset_test_occupancy_state().
  3. Dynamic connection resolution: reset_test_occupancy_state() connects via get_db_connection(), which resolves whichever database target is active at that moment (settings.db_path / settings.database_url).
    • When a test runs with isolated_repository_db, the teardown reset operates on the temporary database and ensures occupancy_manager in-memory state is clean. Monkeypatch then restores settings.db_path and database_url back to the session database upon exiting the test.
    • When a test runs against the shared session database, reset_test_occupancy_state() restores configuration row 1 to pristine defaults, resets daily opening schedules to baseline, resets occupancy_business_day_schedules.is_holiday = 0, and deletes all test rows from occupancy_holidays, occupancy_holiday_audit, occupancy_events, and occupancy_event_audit.
  4. Zero interference: Because isolated_repository_db is opt-in, it does not collide with the autouse reset; instead, both ensure that tests leave neither in-memory nor persistent state behind for subsequent tests, regardless of execution order.

Verification:

  • Full test suite: 566 passed (100% green).
  • Shuffled & reversed runs (PYTEST_SHUFFLE_SEED=reverse, PYTEST_SHUFFLE_SEED=42): passed.
  • Ruff check & format: clean.
  • Docs check: passed (42 Markdown files and 89 HTTP operations).

Please run review pass 2 against b596985.

## Pass 1 fixes (b596985) Addressed every finding in review r39. Merged origin/master (`7f17d00` with PR #243) in `b596985`; fixes are in `37438ed`. | Finding | Fix | | --- | --- | | **P2-1** (Spec & Standards: global reset silently swallows failure) | Removed the bare `except Exception: pass` block in `tests/conftest.py`. Failures in `reset_test_occupancy_state()` will now raise immediately to prevent silent state leakage. | | **P2-2** (Spec: reset misses new holiday state after #178) | Updated `reset_test_occupancy_state()` in `tests/conftest.py` to reset `occupancy_business_day_schedules.is_holiday = 0`, delete all rows from `occupancy_holidays`, `occupancy_holiday_audit`, `occupancy_events`, and `occupancy_event_audit`. Added `tests/test_holiday_state_reset.py` proving that holiday and event state created in one test does not carry over to subsequent tests. | | **P3-1** (Spec: other order dependence not reported) | Added `pytest_collection_modifyitems` hook in `tests/conftest.py` controlled by `PYTEST_SHUFFLE_SEED` (supporting random seeds and `"reverse"`). Tested suite and subsets with reversed order and multiple random seeds (e.g. seed 42) across `test_business_day_schedules.py`, `test_occupancy_proportional_calibration.py`, `test_occupancy.py`, `test_calibration_reconciliation.py`, and `test_holiday_state_reset.py`. All tests passed cleanly without order dependence. | | **P3-2** (Spec & Standards: unused async helpers) | Removed uncalled methods `reset_config_to_defaults_async` and `reset_daily_schedules_async` from `app/db/occupancy_repository.py`. | | **P3-3** (Standards: duplicated default seed literals) | Extracted `DEFAULT_DAILY_SCHEDULE` into `app/schemas/occupancy_models.py` and `seed_default_occupancy_config` into `app/db/occupancy_repository.py`. Unified `init_schema()` in `app/db/database.py` and repository reset methods to share the same constants and seed logic. | | **P3-4** (Standards: shotgun surgery in reset ownership) | Centralized all state restoration inside `tests/conftest.py` (`reset_test_occupancy_state` and `reset_occupancy_state_in_tests` autouse fixture). Cleaned up `tests/test_occupancy.py`'s `setup_test_db` to only delete test-specific event records and cameras, eliminating duplicate reset logic. | | **P3-5** (Standards: OccupancyManager field repetition) | Updated `OccupancyManager.__init__` in `app/services/occupancy_service.py` to call `self.reset()`, keeping field initialization and reset in a single place. | | **P3-6** (Standards: local asyncio imports) | Resolved by removing the unused async helpers from `app/db/occupancy_repository.py`. | | **P3-7** (Standards: connection type annotation) | Changed `conn: Any | None` to `conn: sqlite3.Connection | None = None` in `reset_config_to_defaults_sync`, `reset_daily_schedules_sync`, and `seed_default_config`. | | **P3-8** (Standards: extra DB write overhead) | Benchmarked `reset_test_occupancy_state()`: averages ~3.65 ms per test execution on temporary SQLite DB, resulting in ~2.0 seconds total overhead across the entire 566-test suite. | | **P3-9** (Standards: coordination with #218 & #243) | Maintained full compatibility with the opt-in `isolated_repository_db` fixture added in PR #243 (`00a9b28`) and merged via `origin/master` (`7f17d00`). Did not duplicate or rename `isolated_repository_db`. | ### How the conftest reset and `isolated_repository_db` fixture work together 1. **Orthogonal responsibilities**: `isolated_repository_db` is an **opt-in** fixture designed for test modules that require a dedicated, separate SQLite database file on disk (e.g. `tests/test_business_cycle_labels.py`). It uses `monkeypatch` to set `settings.database_url = None`, redirect `settings.db_path` to `tmp_path / "repository.db"`, and invoke `init_db()`. 2. **Global lifecycle hygiene**: `reset_occupancy_state_in_tests` is an **autouse** fixture running for every test in the suite. At test setup, it clears in-memory singleton caches and rate trackers via `occupancy_manager.reset()`. At test teardown (`yield`), it calls `reset_test_occupancy_state()`. 3. **Dynamic connection resolution**: `reset_test_occupancy_state()` connects via `get_db_connection()`, which resolves whichever database target is active at that moment (`settings.db_path` / `settings.database_url`). - When a test runs with `isolated_repository_db`, the teardown reset operates on the temporary database and ensures `occupancy_manager` in-memory state is clean. Monkeypatch then restores `settings.db_path` and `database_url` back to the session database upon exiting the test. - When a test runs against the shared session database, `reset_test_occupancy_state()` restores configuration row 1 to pristine defaults, resets daily opening schedules to baseline, resets `occupancy_business_day_schedules.is_holiday = 0`, and deletes all test rows from `occupancy_holidays`, `occupancy_holiday_audit`, `occupancy_events`, and `occupancy_event_audit`. 4. **Zero interference**: Because `isolated_repository_db` is opt-in, it does not collide with the autouse reset; instead, both ensure that tests leave neither in-memory nor persistent state behind for subsequent tests, regardless of execution order. Verification: - Full test suite: 566 passed (100% green). - Shuffled & reversed runs (`PYTEST_SHUFFLE_SEED=reverse`, `PYTEST_SHUFFLE_SEED=42`): passed. - Ruff check & format: clean. - Docs check: passed (42 Markdown files and 89 HTTP operations). Please run **review pass 2** against `b596985`.
gabogg left a comment

Code review, pass 2 (origin/master...b596985, spec #221)

Result: 2 P2s and 8 P3s. Not mergeable yet. Fix both P2s. Fix the stale PR body and the step-pair test along with them, since they are part of the P2 changes. The other P3s are filed as #264. Then merge origin/master and request a third pass.

  • Good news: most of pass 1 is fixed, verified on an export of b596985:
    • The except is gone.
    • There is one shared default seed (DEFAULT_DAILY_SCHEDULE, seed_default_config) for init_schema and the reset.
    • Conftest owns the reset.
    • __init__ calls reset().
    • The async helpers are deleted, and the typing is fixed.
    • The reset costs about 3.65 ms per test, as measured.
    • isolated_repository_db is untouched.
  • Test runs:
    • The #221 pair passes in both orders (23 of 23).
    • test_holiday_state_reset.py passes in both orders.
    • Targeted shuffles over the occupancy, schedule and statistics files are clean (53/53 with two seeds, 98/98).

Spec

Pass-1 status

r39 Status
P2-1 silent except FIXED
P2-2 reset misses #178 holiday state PARTIAL, see P2-B
P3-1 report other order dependence PARTIAL: the note is only in comment 4117, not in the PR body
P3-2 uncalled async helpers FIXED

P2

  • P2-B. The reset clears the holiday flag but leaves the frozen day behind (tests/conftest.py:103).
    • What the reset does: it only runs UPDATE occupancy_business_day_schedules SET is_holiday = 0. The stamped rows stay, with is_open = 0, is_exception = 1, exception_name and custom hours. occupancy_business_day_schedule_audit isn't cleared either.
    • Why that leaks: ensure_business_day_schedule_record_async (occupancy_repository.py ~1490) uses INSERT OR IGNORE, so a later test that resolves the same date reads the leaked closed or custom-hours schedule instead of the defaults.
    • Example: test_step1 leaves 2026-12-25 closed as "Navidad".
    • Plausible from reading the code; no failing probe.
    • Fix: DELETE FROM occupancy_business_day_schedules and DELETE FROM occupancy_business_day_schedule_audit, alongside the holiday, event and holiday-audit tables. Add a test where one test stamps a closed day and the next resolves the same date and gets the defaults.

P3 (fix with the P2s)

  • The step pair proves nothing under shuffle. test_step1/test_step2 in test_holiday_state_reset.py depend on file order: run shuffled or alone, step2 passes vacuously. test_explicit_reset_clears_holiday_schedules_and_config already covers the reset directly. Drop the pair, or replace it with the explicit test from P2-B.
  • The PR body is stale. It still describes the deleted _async helpers and the per-module reset. Rewrite it, and add the "other order dependence: found or not found" line the spec asks for ("Note any found in the PR"). Say exactly what was shuffled: the full suite or which subset, and with which seeds.

Standards

Pass-1 status

P3-3 to P3-9 are FIXED.

P2

  • P2-A. The test imports conftest a second time (tests/test_holiday_state_reset.py:5).
    • Cause: from tests.conftest import reset_test_occupancy_state makes pytest's conftest run again as the module tests.conftest. A probe shows sys.modules['conftest'] is not sys.modules['tests.conftest'].
    • Why it matters: it's harmless today. But #218 creates a temp dir, sets settings.db_path and runs init_db() when conftest is imported. On a tree with #218 merged, a probe shows the session switching to a second database whose temp dir is never cleaned up.
    • Fix: move reset_test_occupancy_state into a plain helper module (for example tests/occupancy_reset.py), and import it from conftest and from the test.

P3 → filed as #264

Filed as #264:

  • the trust and statistics seed literals are duplicated;
  • the forwarding wrappers are Middle Men, and production carries a test-only destructive reset;
  • reset_daily_schedules_sync(conn=...) returns stale rows;
  • DEFAULT_DAILY_SCHEDULE is a positional tuple;
  • the PYTEST_SHUFFLE_SEED hook is undocumented (it overlaps #217/#218).

Unused imports in the new test file (sqlite3, HolidayCreateOrUpdate, occupancy_manager): remove them with the step pair.

Merge order reminder: #233 merges next, then #218 adopts it.

## Code review, pass 2 (`origin/master...b596985`, spec #221) Result: **2 P2s and 8 P3s. Not mergeable yet.** Fix both **P2s**. Fix the stale PR body and the step-pair test along with them, since they are part of the P2 changes. The other P3s are filed as **#264**. Then merge `origin/master` and request a **third pass**. - **Good news:** most of pass 1 is fixed, verified on an export of `b596985`: - The `except` is gone. - There is one shared default seed (`DEFAULT_DAILY_SCHEDULE`, `seed_default_config`) for `init_schema` and the reset. - Conftest owns the reset. - `__init__` calls `reset()`. - The async helpers are deleted, and the typing is fixed. - The reset costs about 3.65 ms per test, as measured. - `isolated_repository_db` is untouched. - **Test runs:** - The #221 pair passes in both orders (23 of 23). - `test_holiday_state_reset.py` passes in both orders. - Targeted shuffles over the occupancy, schedule and statistics files are clean (53/53 with two seeds, 98/98). ## Spec ### Pass-1 status | r39 | Status | |---|---| | P2-1 silent `except` | FIXED | | P2-2 reset misses #178 holiday state | **PARTIAL**, see P2-B | | P3-1 report other order dependence | PARTIAL: the note is only in comment 4117, not in the PR body | | P3-2 uncalled async helpers | FIXED | ### P2 - **P2-B. The reset clears the holiday flag but leaves the frozen day behind** (`tests/conftest.py:103`). - **What the reset does:** it only runs `UPDATE occupancy_business_day_schedules SET is_holiday = 0`. The stamped rows stay, with `is_open = 0`, `is_exception = 1`, `exception_name` and custom hours. `occupancy_business_day_schedule_audit` isn't cleared either. - **Why that leaks:** `ensure_business_day_schedule_record_async` (`occupancy_repository.py` ~1490) uses `INSERT OR IGNORE`, so a later test that resolves the same date reads the leaked closed or custom-hours schedule instead of the defaults. - **Example:** `test_step1` leaves 2026-12-25 closed as "Navidad". - Plausible from reading the code; no failing probe. - **Fix:** `DELETE FROM occupancy_business_day_schedules` and `DELETE FROM occupancy_business_day_schedule_audit`, alongside the holiday, event and holiday-audit tables. Add a test where one test stamps a closed day and the next resolves the same date and gets the defaults. ### P3 (fix with the P2s) - **The step pair proves nothing under shuffle.** `test_step1`/`test_step2` in `test_holiday_state_reset.py` depend on file order: run shuffled or alone, step2 passes vacuously. `test_explicit_reset_clears_holiday_schedules_and_config` already covers the reset directly. Drop the pair, or replace it with the explicit test from P2-B. - **The PR body is stale.** It still describes the deleted `_async` helpers and the per-module reset. Rewrite it, and add the "other order dependence: found or not found" line the spec asks for (*"Note any found in the PR"*). Say exactly what was shuffled: the full suite or which subset, and with which seeds. ## Standards ### Pass-1 status P3-3 to P3-9 are FIXED. ### P2 - **P2-A. The test imports conftest a second time** (`tests/test_holiday_state_reset.py:5`). - **Cause:** `from tests.conftest import reset_test_occupancy_state` makes pytest's `conftest` run again as the module `tests.conftest`. A probe shows `sys.modules['conftest'] is not sys.modules['tests.conftest']`. - **Why it matters:** it's harmless today. But #218 creates a temp dir, sets `settings.db_path` and runs `init_db()` when conftest is imported. On a tree with #218 merged, a probe shows the session switching to a second database whose temp dir is never cleaned up. - **Fix:** move `reset_test_occupancy_state` into a plain helper module (for example `tests/occupancy_reset.py`), and import it from conftest and from the test. ### P3 → filed as #264 Filed as #264: - the trust and statistics seed literals are duplicated; - the forwarding wrappers are Middle Men, and production carries a test-only destructive reset; - `reset_daily_schedules_sync(conn=...)` returns stale rows; - `DEFAULT_DAILY_SCHEDULE` is a positional tuple; - the `PYTEST_SHUFFLE_SEED` hook is undocumented (it overlaps #217/#218). Unused imports in the new test file (`sqlite3`, `HolidayCreateOrUpdate`, `occupancy_manager`): remove them with the step pair. **Merge order reminder:** #233 merges next, then #218 adopts it.
Author
Owner

Pass 2 fixes (1885b27)

Addressed r46 after merging origin/master at 9a72ff4 (merge commit ff64473).

Finding Disposition / evidence
P2-A: importing conftest twice Fixed: reset_test_occupancy_state lives in tests/occupancy_reset.py, imported by both conftest and the explicit test. No tests.conftest imports remain.
P2-B: retained frozen schedule and audit state Fixed: delete occupancy_business_day_schedules and occupancy_business_day_schedule_audit alongside holiday/event tables and their audits. Direct regression asserts both tables are empty. A child pytest session using the actual conftest runs two tests: stamp a closed exception with custom hours, then freeze/resolve the same date with defaults and only a fresh FREEZE audit. Both regressions failed before deletion and pass afterward.
P3: vacuous step pair / unused imports Removed test_step1/test_step2, sqlite3, HolidayCreateOrUpdate, and occupancy_manager. The fixed-order child session is contained within one collected regression, so an outer shuffle cannot reverse or omit its setup.
P3: stale PR body / missing other-order-dependence report Rewritten around the current fixture/helper and synchronous seed reset implementation. Includes exact full-suite scope, seeds 233 and 264, counts, and the unrelated cardholder failure reproduced by a two-test sequence.
P3: duplicated trust/statistics seed literals Deferred to #264 as directed.
P3: forwarding wrappers and production test-only destructive reset Deferred to #264 as directed.
P3: stale return from reset_daily_schedules_sync(conn=...) Deferred to #264 as directed.
P3: positional DEFAULT_DAILY_SCHEDULE tuple Deferred to #264 as directed.
P3: undocumented shuffle hook / #217–#218 overlap Deferred to #264 as directed.

Verification:

  • Original #221 pair plus reset regressions: 25 passed.
  • Normal full suite: 604 passed in 154.02 seconds; commit-hook full suite also passed.
  • Full-suite shuffle seed 233: 604 passed in 135.34 seconds.
  • Full-suite shuffle seed 264: 603 passed, one unrelated cardholder failure in 136.22 seconds. test_forced_open_with_card_keeps_forced_trigger followed by test_failed_lookup_is_retried_after_a_pause_not_every_poll reproduces the extra person-69 lookup (1 passed, 1 failed in 0.69 seconds). Recorded in the PR body rather than changing unrelated door/cycle isolation.
  • Ruff lint, formatting, docs check and all scoped commit hooks passed. All-files pre-commit exposed pre-existing whitespace/newline problems in eight unrelated files; its automatic edits were reverted, and its pytest run passed all 604 tests.

Please run pass 3 against origin/master...1885b27 (spec #221). Remaining pass-2 P3s stay tracked in #264; PR #233 remains open for review.

## Pass 2 fixes (1885b27) Addressed r46 after merging `origin/master` at `9a72ff4` (merge commit `ff64473`). | Finding | Disposition / evidence | | --- | --- | | P2-A: importing conftest twice | Fixed: `reset_test_occupancy_state` lives in `tests/occupancy_reset.py`, imported by both conftest and the explicit test. No `tests.conftest` imports remain. | | P2-B: retained frozen schedule and audit state | Fixed: delete `occupancy_business_day_schedules` and `occupancy_business_day_schedule_audit` alongside holiday/event tables and their audits. Direct regression asserts both tables are empty. A child pytest session using the actual conftest runs two tests: stamp a closed exception with custom hours, then freeze/resolve the same date with defaults and only a fresh FREEZE audit. Both regressions failed before deletion and pass afterward. | | P3: vacuous step pair / unused imports | Removed `test_step1`/`test_step2`, `sqlite3`, `HolidayCreateOrUpdate`, and `occupancy_manager`. The fixed-order child session is contained within one collected regression, so an outer shuffle cannot reverse or omit its setup. | | P3: stale PR body / missing other-order-dependence report | Rewritten around the current fixture/helper and synchronous seed reset implementation. Includes exact full-suite scope, seeds 233 and 264, counts, and the unrelated cardholder failure reproduced by a two-test sequence. | | P3: duplicated trust/statistics seed literals | Deferred to #264 as directed. | | P3: forwarding wrappers and production test-only destructive reset | Deferred to #264 as directed. | | P3: stale return from `reset_daily_schedules_sync(conn=...)` | Deferred to #264 as directed. | | P3: positional `DEFAULT_DAILY_SCHEDULE` tuple | Deferred to #264 as directed. | | P3: undocumented shuffle hook / #217–#218 overlap | Deferred to #264 as directed. | Verification: - Original #221 pair plus reset regressions: 25 passed. - Normal full suite: 604 passed in 154.02 seconds; commit-hook full suite also passed. - Full-suite shuffle seed 233: 604 passed in 135.34 seconds. - Full-suite shuffle seed 264: 603 passed, one unrelated cardholder failure in 136.22 seconds. `test_forced_open_with_card_keeps_forced_trigger` followed by `test_failed_lookup_is_retried_after_a_pause_not_every_poll` reproduces the extra person-69 lookup (1 passed, 1 failed in 0.69 seconds). Recorded in the PR body rather than changing unrelated door/cycle isolation. - Ruff lint, formatting, docs check and all scoped commit hooks passed. All-files pre-commit exposed pre-existing whitespace/newline problems in eight unrelated files; its automatic edits were reverted, and its pytest run passed all 604 tests. Please run **pass 3** against `origin/master...1885b27` (spec #221). Remaining pass-2 P3s stay tracked in #264; PR #233 remains open for review.
gabogg left a comment

Code review, pass 3 (origin/master...1885b27, spec #221)

Result: no P1, no P2 and 2 P3s. Mergeable. The second order dependence this PR found is now tracked as #269, in the test-loop milestone. The teardown-guard P3 is added to #264.

r46 Status Evidence
P2-A conftest imported twice FIXED The reset helper is in tests/occupancy_reset.py, and nothing imports tests.conftest. A probe after the reset tests shows only tests and tests.occupancy_reset in sys.modules.
P2-B frozen schedules and audits survive FIXED occupancy_reset.py:18-23 deletes both business-day schedule tables, holidays, events and their audits. The child-pytest regression (test_holiday_state_reset.py:80-149) stamps a closed day, then resolves the same date and gets the defaults. With the two DELETE lines removed, the regression fails, so the test isn't vacuous.
P3 step pair and unused imports FIXED
P3 PR body FIXED It reports "Other order dependence: found", with the whole 604-test collection, seeds 233 and 264, and a two-test reproduction.
Other P3s Deferred #264

Probes:

  • test_holiday_state_reset.py passes 2 of 2, in normal and reversed order.
  • The #221 pair passes 23 of 23 in both orders and with the reverse shuffle.
  • Combined with #218: merge-tree merges cleanly. In the merged tree, #218 sets up the database when conftest is imported, and the reset works through settings.db_path, so everything stays on one database. The new file plus the occupancy pair, reversed, pass 25 of 25.

P3

  • The cardholder order dependence has no tracking issue. When test_forced_open_with_card_keeps_forced_trigger runs first, test_failed_lookup_is_retried_after_a_pause_not_every_poll sees calls ['69','67'] instead of ['67']. → #269
  • The reset checks only occupancy_config before deleting from six tables (occupancy_reset.py:11). A partly initialized schema would crash in teardown. → added to #264

Merge order: this PR merges next, then #218 merges origin/master and adopts it.

## Code review, pass 3 (`origin/master...1885b27`, spec #221) Result: **no P1, no P2 and 2 P3s. Mergeable.** The second order dependence this PR found is now tracked as **#269**, in the test-loop milestone. The teardown-guard P3 is added to #264. | r46 | Status | Evidence | |---|---|---| | P2-A conftest imported twice | **FIXED** | The reset helper is in `tests/occupancy_reset.py`, and nothing imports `tests.conftest`. A probe after the reset tests shows only `tests` and `tests.occupancy_reset` in `sys.modules`. | | P2-B frozen schedules and audits survive | **FIXED** | `occupancy_reset.py:18-23` deletes both business-day schedule tables, holidays, events and their audits. The child-pytest regression (`test_holiday_state_reset.py:80-149`) stamps a closed day, then resolves the same date and gets the defaults. **With the two DELETE lines removed, the regression fails**, so the test isn't vacuous. | | P3 step pair and unused imports | FIXED | | | P3 PR body | FIXED | It reports "Other order dependence: found", with the whole 604-test collection, seeds 233 and 264, and a two-test reproduction. | | Other P3s | Deferred | #264 | **Probes:** - `test_holiday_state_reset.py` passes 2 of 2, in normal and reversed order. - The #221 pair passes 23 of 23 in both orders and with the reverse shuffle. - **Combined with #218:** `merge-tree` merges cleanly. In the merged tree, #218 sets up the database when conftest is imported, and the reset works through `settings.db_path`, so everything stays on one database. The new file plus the occupancy pair, reversed, pass 25 of 25. ### P3 - **The cardholder order dependence has no tracking issue.** When `test_forced_open_with_card_keeps_forced_trigger` runs first, `test_failed_lookup_is_retried_after_a_pause_not_every_poll` sees calls `['69','67']` instead of `['67']`. → **#269** - **The reset checks only `occupancy_config` before deleting from six tables** (`occupancy_reset.py:11`). A partly initialized schema would crash in teardown. → added to **#264** **Merge order:** this PR merges next, then #218 merges `origin/master` and adopts it.
gabogg merged commit 75c3b5d1e5 into master 2026-10-03 12:58:15 +00:00
gabogg deleted branch fix/test-order-dependence-221 2026-10-03 12:58:16 +00:00
Sign in to join this conversation.
No description provided.