Skip to content

Isolate daily summary race test stubs on Windows#7792

Open
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-daily-summary-race-test
Open

Isolate daily summary race test stubs on Windows#7792
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-daily-summary-race-test

Conversation

@tianmind-studio

@tianmind-studio tianmind-studio commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a lightweight pytz test stub so test_daily_summary_race_condition.py can collect in a minimal Windows backend venv.
  • Use a DST-aware New York fallback timezone instead of a fixed EST offset.
  • Clear stale/orphaned utils.* package entries left by earlier tests before importing the real utils.other.notifications module.
  • Keep handles to the stubbed collaborators imported by utils.other.notifications, then remove the stub modules and the imported utils.other.notifications module from sys.modules after import.
  • Update the notification integration assertions to patch/use those saved collaborator handles instead of reaching back into sys.modules.

Why

On a minimal Windows backend environment, this test failed at collection because pytz was not installed. After stubbing pytz, the file also polluted sys.modules with database.daily_summaries, which made the adjacent test_daily_summary_regenerate.py import a stale stub instead of the real module during collection.

In the broader Windows collect-only smoke path, earlier tests can also leave orphaned utils.* modules without a parent utils package, which raises KeyError: 'utils' while importing utils.other.notifications. This keeps the race-condition test self-contained while allowing later daily-summary tests and the broader collection pass to proceed cleanly.

Testing

  • python -m pytest tests\unit\test_daily_summary_race_condition.py -q -> 13 passed
  • python -m pytest tests\unit\test_daily_summary_race_condition.py tests\unit\test_daily_summary_regenerate.py -q -> 15 passed
  • python -m pytest tests\unit\test_daily_summary_regenerate.py tests\unit\test_daily_summary_race_condition.py -q -> 15 passed
  • python -m black --line-length 120 --skip-string-normalization tests\unit\test_daily_summary_race_condition.py --check
  • python -m py_compile tests\unit\test_daily_summary_race_condition.py
  • git diff --check -- backend/tests/unit/test_daily_summary_race_condition.py

@tianmind-studio tianmind-studio force-pushed the codex/windows-daily-summary-race-test branch from 83be990 to 008f6f6 Compare June 10, 2026 11:20
@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR isolates test_daily_summary_race_condition.py so it can collect on a minimal Windows environment without pytz installed, and removes stub modules from sys.modules after the import to prevent them from bleeding into adjacent test files like test_daily_summary_regenerate.py.

  • Adds a _PytzFixedTimezone stub and registers it as pytz in sys.modules only if pytz is absent, with the stub tracked for later removal.
  • Introduces _STUB_MODULE_NAMES, _remove_stub_module, and _remove_empty_stub_package helpers that surgically tear down all stubs (deepest-first) after utils.other.notifications is imported, saving live references to its collaborators beforehand.
  • Replaces all sys.modules["..."] lookups in test assertions with the pre-captured module-level handles (_CONVERSATIONS_DB, _DAILY_SUMMARIES_DB, etc.).

Confidence Score: 4/5

Safe to merge; changes are test-only and the core isolation mechanic works correctly for the stated goal.

The stub teardown approach is sound and the 15-test run in both orderings validates the primary goal. Two residual gaps: the _NY_TZ timezone is fixed at UTC-5 regardless of DST, which could produce an off-by-one date for lock keys when tests run during EDT hours near midnight; and utils.other.notifications itself is not removed from sys.modules after cleanup.

backend/tests/unit/test_daily_summary_race_condition.py — specifically the _NY_TZ fixed offset and the fact that utils.other.notifications lingers in sys.modules after stub cleanup.

Important Files Changed

Filename Overview
backend/tests/unit/test_daily_summary_race_condition.py Adds pytz stub, stub-removal helpers, and module-level handle caching so the test is self-contained on Windows; replaces sys.modules lookups in assertions with saved references. Minor concerns: fixed UTC-5 offset ignores DST for America/New_York, and utils.other.notifications is not cleaned from sys.modules after stub teardown.

Reviews (1): Last reviewed commit: "test(backend): isolate daily summary rac..." | Re-trigger Greptile

Comment on lines +73 to +74
def tzname(self, dt):
return self._zone.tzname(dt)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fixed EST offset ignores Daylight Saving Time

_NY_TZ is hardcoded to UTC-5 (EST), but America/New_York observes EDT (UTC-4) from roughly mid-March through early November. Any test that runs during DST and calls _send_summary_notification with the America/New_York timezone will compute a local date that is one hour behind the real local time. Near midnight, this produces a lock key for the previous day instead of the current day, which could mask failures or produce spurious passes in the date-boundary logic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the current head: _PytzEasternTimezone now switches between EST and EDT using the US DST transition bounds, so the New York fallback is not fixed at UTC-5.

Comment on lines 193 to 194
sub_mod = _stub_module("utils.subscription")
sub_mod.is_trial_paywalled = MagicMock(return_value=False)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 utils.other.notifications stays in sys.modules after cleanup

The cleanup loop removes every name in _STUB_MODULE_NAMES, but utils.other.notifications was imported with a real import statement and is not tracked in that set. It therefore remains in sys.modules for the rest of the process, carrying live references to all the stub MagicMock objects as its module-level globals. Any later test file that does import utils.other.notifications will receive this already-initialised instance with stubs baked into conversations_db, daily_summaries_db, etc., rather than a clean import. This is the same class of problem the PR is solving for database.daily_summaries.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the current head: the test keeps a handle to the imported module for patch.object(...), then explicitly removes utils.other.notifications from sys.modules after saving the collaborator references.

@tianmind-studio tianmind-studio force-pushed the codex/windows-daily-summary-race-test branch from 008f6f6 to 7b2ea57 Compare June 10, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant