fix(tests): test order dependence between test_occupancy and test_calibration_reconciliation #233
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!233
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/test-order-dependence-221"
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?
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 importtests.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 throughINSERT 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 oldtest_step1/test_step2pair 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 toreset().seed_default_config; weekly schedules shareDEFAULT_DAILY_SCHEDULE. Only the synchronous repository reset helpers remain.tests/occupancy_reset.pyprovides the plain helper for both fixture cleanup and explicit regression coverage.isolated_repository_dbremains unchanged.origin/master(9a72ff4) before verification.Verification / Test Evidence
ruff check .,ruff format --check ., andpython scripts/check_docs.pypass (43 Markdown files, 90 HTTP operations).PYTEST_SHUFFLE_SEEDcollection hook: seed233passed 604/604 in 135.34 seconds; seed264passed 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 seeds233and264. No failures with233;264failstests/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 onlytest_forced_open_with_card_keeps_forced_triggerfollowed bytest_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
tests.conftestimports.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.WIP: fix(tests): test order dependence between test_occupancy and test_calibration_reconciliationto fix(tests): test order dependence between test_occupancy and test_calibration_reconciliationFirst code push is committed and pushed (
1ec6a32). PR #233 has been promoted from draft to ready.Summary of changes
app/services/occupancy_service.py: AddedOccupancyManager.reset()to reset in-memory caches, rate trackers, and_last_auto_calib_dateguard state.app/db/occupancy_repository.py: Addedreset_config_to_defaults_sync/_asyncandreset_daily_schedules_sync/_asyncto restoreoccupancy_configrow 1 andoccupancy_daily_schedulerows to pristine system defaults without inline SQL at call sites.tests/test_occupancy.py: Updatedsetup_test_dbautouse fixture to reset counting events, cameras, calibration logs, holidays, configuration, schedules, and singleton state on both setup and teardown (yield).tests/conftest.py: Addedreset_occupancy_state_in_testsautouse 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 passedpytest tests/test_calibration_reconciliation.py tests/test_occupancy.py: 23 passedpytest tests/test_calibration_reconciliation.py::test_auto_calibration_in_quiet_window: 1 passedpytest: 534 passed (100% green)ruff check .&ruff format .: cleanpython3 scripts/check_docs.py: passedReady for first pass review.
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(nowb64e805with #178) and request a second pass.016a012, runningpytest tests/test_occupancy.py tests/test_calibration_reconciliation.pyfailstest_auto_calibration_in_quiet_window.1ec6a32, both orders pass (23 of 23). Nothing is skipped, marked xfail or reordered, and the test itself is unchanged.daily_reset_time='04:00',calibration_mode, schedules and holidays.fix/test-isolation) without textual conflicts.Spec
P2
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 theexcept, and let it raise.occupancy_business_day_schedules.is_holidayandoccupancy_holiday_audit, and addsoccupancy_events.tests/test_occupancy.py:21deletes onlyoccupancy_holidays, so after the master merge staleis_holidayflags and events can leak between tests.P3
test_business_day_schedules.py,test_occupancy_proportional_calibration.pyandtest_statistics_*.-p randomlywith 2 or 3 seeds, or a reversed collection), and report what you find.reset_config_to_defaults_async(occupancy_repository.py:476) andreset_daily_schedules_async(:871). Delete them (raised on both axes).Standards
P3
reset_config_to_defaults_sync(occupancy_repository.py:429-474) andreset_daily_schedules_sync(:848-869) copy theinit_dbseed literals fromapp/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 oneDEFAULT_DAILY_SCHEDULEand one config-seed helper, used by bothinit_dband the reset.test_occupancy.pyresets config plus schedules. Make conftest the single owner: reset config, schedules and holiday state there, and drop the per-module copy.OccupancyManager.reset()repeats the__init__field list (occupancy_service.py:103-140). A new field could be missed. Have__init__callreset().import asynciosits inside function bodies (occupancy_repository.py:478,:873). Move it to the module top. This goes away if P3-2 deletes the helpers.conn: Any | Noneis typedAny(occupancy_repository.py:429). Usesqlite3.Connection | None.isolated_test_dband runsinit_db()when conftest is imported. Agree there on a single place where test state is reset.Pass 1 fixes (
b596985)Addressed every finding in review r39. Merged origin/master (
7f17d00with PR #243) inb596985; fixes are in37438ed.except Exception: passblock intests/conftest.py. Failures inreset_test_occupancy_state()will now raise immediately to prevent silent state leakage.reset_test_occupancy_state()intests/conftest.pyto resetoccupancy_business_day_schedules.is_holiday = 0, delete all rows fromoccupancy_holidays,occupancy_holiday_audit,occupancy_events, andoccupancy_event_audit. Addedtests/test_holiday_state_reset.pyproving that holiday and event state created in one test does not carry over to subsequent tests.pytest_collection_modifyitemshook intests/conftest.pycontrolled byPYTEST_SHUFFLE_SEED(supporting random seeds and"reverse"). Tested suite and subsets with reversed order and multiple random seeds (e.g. seed 42) acrosstest_business_day_schedules.py,test_occupancy_proportional_calibration.py,test_occupancy.py,test_calibration_reconciliation.py, andtest_holiday_state_reset.py. All tests passed cleanly without order dependence.reset_config_to_defaults_asyncandreset_daily_schedules_asyncfromapp/db/occupancy_repository.py.DEFAULT_DAILY_SCHEDULEintoapp/schemas/occupancy_models.pyandseed_default_occupancy_configintoapp/db/occupancy_repository.py. Unifiedinit_schema()inapp/db/database.pyand repository reset methods to share the same constants and seed logic.tests/conftest.py(reset_test_occupancy_stateandreset_occupancy_state_in_testsautouse fixture). Cleaned uptests/test_occupancy.py'ssetup_test_dbto only delete test-specific event records and cameras, eliminating duplicate reset logic.OccupancyManager.__init__inapp/services/occupancy_service.pyto callself.reset(), keeping field initialization and reset in a single place.app/db/occupancy_repository.py.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.isolated_repository_dbfixture added in PR #243 (00a9b28) and merged viaorigin/master(7f17d00). Did not duplicate or renameisolated_repository_db.How the conftest reset and
isolated_repository_dbfixture work togetherisolated_repository_dbis 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 usesmonkeypatchto setsettings.database_url = None, redirectsettings.db_pathtotmp_path / "repository.db", and invokeinit_db().reset_occupancy_state_in_testsis an autouse fixture running for every test in the suite. At test setup, it clears in-memory singleton caches and rate trackers viaoccupancy_manager.reset(). At test teardown (yield), it callsreset_test_occupancy_state().reset_test_occupancy_state()connects viaget_db_connection(), which resolves whichever database target is active at that moment (settings.db_path/settings.database_url).isolated_repository_db, the teardown reset operates on the temporary database and ensuresoccupancy_managerin-memory state is clean. Monkeypatch then restoressettings.db_pathanddatabase_urlback to the session database upon exiting the test.reset_test_occupancy_state()restores configuration row 1 to pristine defaults, resets daily opening schedules to baseline, resetsoccupancy_business_day_schedules.is_holiday = 0, and deletes all test rows fromoccupancy_holidays,occupancy_holiday_audit,occupancy_events, andoccupancy_event_audit.isolated_repository_dbis 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:
PYTEST_SHUFFLE_SEED=reverse,PYTEST_SHUFFLE_SEED=42): passed.Please run review pass 2 against
b596985.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/masterand request a third pass.b596985:exceptis gone.DEFAULT_DAILY_SCHEDULE,seed_default_config) forinit_schemaand the reset.__init__callsreset().isolated_repository_dbis untouched.test_holiday_state_reset.pypasses in both orders.Spec
Pass-1 status
exceptP2
tests/conftest.py:103).UPDATE occupancy_business_day_schedules SET is_holiday = 0. The stamped rows stay, withis_open = 0,is_exception = 1,exception_nameand custom hours.occupancy_business_day_schedule_auditisn't cleared either.ensure_business_day_schedule_record_async(occupancy_repository.py~1490) usesINSERT OR IGNORE, so a later test that resolves the same date reads the leaked closed or custom-hours schedule instead of the defaults.test_step1leaves 2026-12-25 closed as "Navidad".DELETE FROM occupancy_business_day_schedulesandDELETE 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)
test_step1/test_step2intest_holiday_state_reset.pydepend on file order: run shuffled or alone, step2 passes vacuously.test_explicit_reset_clears_holiday_schedules_and_configalready covers the reset directly. Drop the pair, or replace it with the explicit test from P2-B._asynchelpers 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
tests/test_holiday_state_reset.py:5).from tests.conftest import reset_test_occupancy_statemakes pytest'sconftestrun again as the moduletests.conftest. A probe showssys.modules['conftest'] is not sys.modules['tests.conftest'].settings.db_pathand runsinit_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.reset_test_occupancy_stateinto a plain helper module (for exampletests/occupancy_reset.py), and import it from conftest and from the test.P3 → filed as #264
Filed as #264:
reset_daily_schedules_sync(conn=...)returns stale rows;DEFAULT_DAILY_SCHEDULEis a positional tuple;PYTEST_SHUFFLE_SEEDhook 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.
Pass 2 fixes (
1885b27)Addressed r46 after merging
origin/masterat9a72ff4(merge commitff64473).reset_test_occupancy_statelives intests/occupancy_reset.py, imported by both conftest and the explicit test. Notests.conftestimports remain.occupancy_business_day_schedulesandoccupancy_business_day_schedule_auditalongside 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.test_step1/test_step2,sqlite3,HolidayCreateOrUpdate, andoccupancy_manager. The fixed-order child session is contained within one collected regression, so an outer shuffle cannot reverse or omit its setup.reset_daily_schedules_sync(conn=...)DEFAULT_DAILY_SCHEDULEtupleVerification:
test_forced_open_with_card_keeps_forced_triggerfollowed bytest_failed_lookup_is_retried_after_a_pause_not_every_pollreproduces 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.Please run pass 3 against
origin/master...1885b27(spec #221). Remaining pass-2 P3s stay tracked in #264; PR #233 remains open for review.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.
tests/occupancy_reset.py, and nothing importstests.conftest. A probe after the reset tests shows onlytestsandtests.occupancy_resetinsys.modules.occupancy_reset.py:18-23deletes 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.Probes:
test_holiday_state_reset.pypasses 2 of 2, in normal and reversed order.merge-treemerges cleanly. In the merged tree, #218 sets up the database when conftest is imported, and the reset works throughsettings.db_path, so everything stays on one database. The new file plus the occupancy pair, reversed, pass 25 of 25.P3
test_forced_open_with_card_keeps_forced_triggerruns first,test_failed_lookup_is_retried_after_a_pause_not_every_pollsees calls['69','67']instead of['67']. → #269occupancy_configbefore deleting from six tables (occupancy_reset.py:11). A partly initialized schema would crash in teardown. → added to #264Merge order: this PR merges next, then #218 merges
origin/masterand adopts it.