Skip to content

Stabilize delete-account stripe test stubs on Windows#7795

Closed
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-delete-account-stripe-test
Closed

Stabilize delete-account stripe test stubs on Windows#7795
tianmind-studio wants to merge 1 commit into
BasedHardware:mainfrom
tianmind-studio:codex/windows-delete-account-stripe-test

Conversation

@tianmind-studio

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

Copy link
Copy Markdown
Contributor

Superseded by #7793, which now includes the same test_delete_account_stripe_cancel.py Windows stub isolation fix together with the sister purge-storage test.

@greptile-apps

greptile-apps Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR stabilises test_delete_account_stripe_cancel.py for minimal Windows test environments by adding pytz to the auto-stubbed import prefixes and by evicting any stale stub entries left by earlier tests from sys.modules before the _StubFinder is installed.

  • pytz stub: pytz is added to _STUB_PREFIXES so the heavy routers.users import graph no longer requires the optional package to be installed, fixing collection failures in a lightweight Windows venv.
  • Stale-module eviction: A new _remove_module_tree helper iterates sys.modules and pops entries matching each stubbed prefix; it is called for every prefix before the finder is installed so that previous tests' cached stubs (e.g. database.*, utils.*) cannot bypass this file's _StubFinder.
  • Side-effect timing: The eviction loop runs at module-import time (during collection), not at test-execution time, which is functionally correct for the described scenario but slightly surprising; the loop's iteration variable _prefix also leaks into the module namespace as the last value ('twilio').

Confidence Score: 4/5

Safe to merge; the two changes are narrow and correct, with no impact on production code.

Both changes touch only the test file. The pytz addition is a one-line fix with no risk. The _remove_module_tree logic correctly guards against dict-mutation by iterating a snapshot (list(sys.modules)) and uses .pop(..., None) to avoid KeyError. The only concerns are that the eviction loop runs at collection time rather than execution time (slightly surprising, but functionally correct for the described use-case) and that the loop variable _prefix is left in the module namespace after the loop completes.

No files require special attention; the single changed file is a unit test with no production code path.

Important Files Changed

Filename Overview
backend/tests/unit/test_delete_account_stripe_cancel.py Adds pytz to the auto-stub list and clears stale sys.modules entries before installing the meta-path finder; logic is correct, but the cleanup loop runs at collection time and leaves a loop-variable artifact in the module namespace.

Sequence Diagram

sequenceDiagram
    participant pytest
    participant test_module as test_delete_account_stripe_cancel.py
    participant sys_modules as sys.modules
    participant StubFinder as _StubFinder
    participant users_router as routers.users

    pytest->>test_module: import (collection phase)
    test_module->>sys_modules: _remove_module_tree(prefix) x N (evict stale stubs)
    test_module->>StubFinder: create _StubFinder()
    test_module->>sys_modules: sys.meta_path.insert(0, _finder)
    test_module->>sys_modules: install utils.other.endpoints shim
    test_module->>users_router: from routers import users
    users_router-->>StubFinder: "find_spec(database.*) → stub"
    users_router-->>StubFinder: find_spec(pytz) → stub (NEW)
    users_router-->>StubFinder: "find_spec(firebase_admin.*) → stub"
    users_router-->>sys_modules: find utils.other.endpoints → shim
    users_router-->>test_module: module loaded
    test_module->>sys_modules: remove _StubFinder from meta_path
    test_module->>sys_modules: pop all _finder.created + shim entries
    pytest->>test_module: run tests
    test_module->>users_router: patch.object(users_db, stripe_utils, auth, ...)
    users_router-->>test_module: assert Stripe cancel / no-cancel / best-effort
Loading

Reviews (1): Last reviewed commit: "test(backend): isolate delete account st..." | Re-trigger Greptile

Comment on lines +67 to +68
for _prefix in _STUB_PREFIXES:
_remove_module_tree(_prefix)

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 Module-level side-effect runs at collection time, not test-execution time

The bare for _prefix in _STUB_PREFIXES: _remove_module_tree(_prefix) block executes when pytest imports this file during collection, not when a test actually runs. In a large test suite collected in a single process, this means the eviction fires once — before any test in this file has begun — which is the desired ordering here, but it also means that if this file is collected after another test has already installed real (non-stub) versions of utils.* or database.* into sys.modules (e.g. through a session-scoped fixture in conftest), those real entries get silently evicted mid-collection. The modules would be cleanly re-imported on next access in environments where they are installable, but in the minimal Windows venv the test targets they are not importable at all — so any other test running after collection that later tries to freshly import from those namespaces would fail. Wrapping the loop in a setup_module() function would scope the eviction to test-execution time and make the intent more explicit.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +67 to +68
for _prefix in _STUB_PREFIXES:
_remove_module_tree(_prefix)

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 Loop variable _prefix leaks into the module namespace

After the for _prefix in _STUB_PREFIXES loop completes, _prefix remains bound in the module's global namespace with the value of the last element ('twilio'). While harmless in practice (it won't interfere with any test), it is inconsistent with the private-by-convention _ prefix the rest of the module uses, and static analysers will flag it as an unused global.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@github-actions

Copy link
Copy Markdown
Contributor

Hey @tianmind-studio 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our roadmap and vision

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community! 💜

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