Stabilize lock bypass tests in lightweight backend env#7789
Stabilize lock bypass tests in lightweight backend env#7789tianmind-studio wants to merge 2 commits into
Conversation
Greptile SummaryThis PR makes
Confidence Score: 4/5Safe to merge; all changes are test-only and do not touch production code paths. The stub additions and mock corrections are well-targeted and the lock-filtering assertions remain valid. The pytz.timezone = ZoneInfo substitution causes the timezone-aware branch in _send_summary_notification to silently fall back to the UTC path during every summary test, so that branch is never actually exercised. The _ToolWrapper.invoke helper also only handles dict inputs and would fail unexpectedly for string inputs. Neither issue invalidates the current passing assertions, but both reduce test fidelity. backend/tests/unit/test_lock_bypass_fixes.py — the pytz stub and _ToolWrapper.invoke edge case are worth a follow-up. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pytest collects test file] --> B[Stub injection: 30+ optional deps into sys.modules]
B --> C[Concrete overrides: ZoneInfo for pytz.timezone, _tool wrapper]
C --> D[Test class executes]
D --> E{Daily summary test?}
E -->|Yes| F[Patch is_trial_paywalled, try_acquire_daily_summary_lock, get_daily_summary_by_date]
F --> G[_send_summary_notification called with UTC tz arg]
G --> H[ZoneInfo.localize raises AttributeError]
H -->|caught silently| I[Falls back to UTC date window]
I --> J[Filter conversations by is_locked]
J --> K[Assert LLM called only with unlocked conversations]
E -->|No| L[Other lock bypass assertions: suggest_goal, MCP endpoints, routers]
Reviews (1): Last reviewed commit: "test(backend): stub optional deps for lo..." | Re-trigger Greptile |
| sys.modules['pytz'].timezone = ZoneInfo | ||
| sys.modules['pytz'].utc = timezone.utc |
There was a problem hiding this comment.
ZoneInfo lacks localize(), silently falling back to UTC path in all tests
sys.modules['pytz'].timezone = ZoneInfo means _send_summary_notification executes ZoneInfo('UTC').localize(datetime.combine(...)), which raises AttributeError because ZoneInfo objects don't have a localize method. This error is silently caught by the try/except Exception block in _send_summary_notification, causing both summary tests to exercise the UTC fallback path rather than the timezone-aware branch. The lock-filtering assertions still pass, but the test never actually exercises the pytz.timezone-based date window calculation. A simple alternative is to use a minimal localize-aware wrapper if the timezone-aware path is meant to be covered.
There was a problem hiding this comment.
Addressed in 8301e85d5. The pytz stand-in is now _PytzZoneInfo, a minimal tzinfo wrapper around ZoneInfo/fixed offsets that supports .localize(...) and datetime.now(tz), so the summary tests exercise the timezone-aware path instead of falling through to UTC via AttributeError.
Revalidated on the Windows backend venv:
python -m pytest tests\unit\test_lock_bypass_fixes.py -q-> 57 passedpython -m black --line-length 120 --skip-string-normalization tests\unit\test_lock_bypass_fixes.py --checkpython -m py_compile tests\unit\test_lock_bypass_fixes.py
| def invoke(self, args=None, config=None): | ||
| kwargs = dict(args or {}) | ||
| if config is not None: | ||
| kwargs['config'] = config | ||
| return self.fn(**kwargs) |
There was a problem hiding this comment.
_ToolWrapper.invoke silently breaks for non-dict inputs
dict(args or {}) works when args is a dict, but raises ValueError if args is a plain string (e.g. tool.invoke("my_input")). The real LangChain StructuredTool.invoke accepts Union[str, dict] as input, so any future test that calls a stubbed @tool function with a string argument will get a confusing error pointing inside _ToolWrapper rather than at the failing test. Adding an isinstance(args, dict) branch would make the stub more robust.
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!
There was a problem hiding this comment.
Addressed in 8301e85d5. _ToolWrapper.invoke now branches on non-dict input and forwards it as a positional argument, matching the stubbed LangChain tool use case instead of trying dict(args). I also added a regression check that wrapped.invoke('hello') == 'hello'.
Revalidated on the Windows backend venv:
python -m pytest tests\unit\test_lock_bypass_fixes.py -q-> 57 passedpython -m black --line-length 120 --skip-string-normalization tests\unit\test_lock_bypass_fixes.py --checkpython -m py_compile tests\unit\test_lock_bypass_fixes.py
tianmind-studio
left a comment
There was a problem hiding this comment.
The current head includes the stub-fidelity follow-up: the pytz stand-in now supports .localize(...) and datetime.now(tz), and the LangChain tool wrapper handles non-dict positional input. I updated the PR description to make that clear.
Summary
@toolstand-in so.invoke(...)tests still exercise the wrapped tool functions, including non-dict positional input.pytzstand-in with.localize(...)anddatetime.now(tz)support so scheduled-summary tests keep exercising timezone-aware code paths.get_llm, and keep scheduled summary tests focused on locked-conversation filtering.Why
On a minimal Windows backend setup,
tests/unit/test_lock_bypass_fixes.pycould fail during import or branch into unrelated paywall/duplicate-summary guards before reaching the lock filtering assertions. These changes make the file runnable without installing optional audio, LLM, Deepgram, Twilio, and related SDKs while keeping the relevant test paths faithful enough to exercise the intended behavior.Testing
python -m pytest tests\unit\test_lock_bypass_fixes.py -q->57 passed, 23 warningspython -m black --line-length 120 --skip-string-normalization tests\unit\test_lock_bypass_fixes.py --checkpython -m py_compile tests\unit\test_lock_bypass_fixes.pygit diff --check -- backend/tests/unit/test_lock_bypass_fixes.py