Skip to content

test(backend): stop test_action_items_timezone from poisoning sys.modules (#8661)#8680

Merged
kodjima33 merged 1 commit into
BasedHardware:mainfrom
ZachL111:zach/fix-sys-modules-test-poisoning
Jul 1, 2026
Merged

test(backend): stop test_action_items_timezone from poisoning sys.modules (#8661)#8680
kodjima33 merged 1 commit into
BasedHardware:mainfrom
ZachL111:zach/fix-sys-modules-test-poisoning

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

What

Fixes the test_action_items_timezone.py part of #8661.

This file installed its stubs by writing to sys.modules at module scope and never restored them. The _stub_package() helper replaced real packages (utils, utils.retrieval, utils.retrieval.tools, utils.conversations, database, langchain_core) with empty types.ModuleType stubs. During a bulk pytest tests/unit/ run, pytest collects every file, so this file's module-level code ran and permanently replaced those packages, and every test file collected alphabetically after it then failed to import from them.

Fix

Snapshot sys.modules before the stubbing begins, and restore it right after the module under test (action_item_tools) is imported:

_SYS_MODULES_SNAPSHOT = dict(sys.modules)
# ... install stubs, load the module under test ...
for _name in list(sys.modules):
    if _name in _SYS_MODULES_SNAPSHOT:
        if sys.modules[_name] is not _SYS_MODULES_SNAPSHOT[_name]:
            sys.modules[_name] = _SYS_MODULES_SNAPSHOT[_name]
    else:
        del sys.modules[_name]

The loaded module keeps its references to the stubbed dependencies (Python binds them at import time), and the tests patch action_item_tools.action_items_db directly rather than by string path, so the restore does not change behaviour.

Verification

pytest tests/unit/test_action_items_timezone.py -q                 # 11 passed (own tests unchanged)
pytest tests/unit/test_action_items_timezone.py \
       tests/unit/test_apps_create_app_json.py -q                  # 26 passed (was a collection ERROR before)

This addresses one of the three module-level-stub files in #8661. The other two (test_action_item_date_validation.py, test_tools_router.py) and the missing fake-firestore dependency are out of scope here.

Review in cubic

…ules (BasedHardware#8661)

The module-level stub setup replaced real packages (utils, utils.retrieval,
utils.conversations, database, langchain_core) with empty types.ModuleType stubs
and never restored them. During a bulk `pytest tests/unit/` run, every test file
collected alphabetically after this one then failed to import from those packages
(issue BasedHardware#8661: 39 collection errors, 38 of which pass when run individually).

Snapshot sys.modules before stubbing and restore it right after the module under
test is imported. The tool reference is already bound to the stubbed dependencies,
and the tests patch action_item_tools.action_items_db directly, so behaviour is
unchanged.

Verified: the file's own tests still pass (11 passed), and the issue's repro
`pytest tests/unit/test_action_items_timezone.py tests/unit/test_apps_create_app_json.py`
now passes (26 passed, previously a collection ERROR).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@Git-on-my-level Git-on-my-level added the needs-maintainer-review Needs human maintainer review before merge label Jun 30, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for isolating this — the fix is small and targeted, and the approach makes sense for the current test structure.

I reviewed the diff and ran the focused regression locally in a clean worktree with secrets stripped:

python -m pytest tests/unit/test_action_items_timezone.py tests/unit/test_apps_create_app_json.py -q

Result: 26 passed.

I’m leaving this as a positive signal rather than a formal approval because this is explicitly one slice of the broader #8661 sys.modules/test-isolation cleanup, so a human maintainer should decide whether to merge this partial fix as-is or wait for the remaining related cleanup. No blocking code concerns from my review.

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

test-only stabilization: stop test_action_items_timezone poisoning sys.modules (#8661); CI green

@kodjima33 kodjima33 merged commit 4cea53a into BasedHardware:main Jul 1, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-maintainer-review Needs human maintainer review before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants