Skip to content

fix: fix multiple bugs in _apply_deterministic_patches#1823

Open
aseembits93 wants to merge 2 commits intomainfrom
fix/deterministic-patches-bugs
Open

fix: fix multiple bugs in _apply_deterministic_patches#1823
aseembits93 wants to merge 2 commits intomainfrom
fix/deterministic-patches-bugs

Conversation

@aseembits93
Copy link
Contributor

Summary

  • Re-entrancy guard: Added _DETERMINISTIC_PATCHES_APPLIED flag to prevent infinite recursion if pytest_configure runs twice
  • Removed unnecessary original function calls: Every mock was calling the original function and discarding the result — unnecessary overhead and a failure point (e.g. uuid.uuid1() can raise on some platforms)
  • Actually patch datetime.datetime.now/utcnow: Previous code stored mocks to builtins but never patched datetime.datetime. Now uses a DeterministicDatetime subclass. Also fixes now(tz=None) incorrectly returning tz-aware datetime
  • Patch missing time functions: Added time_ns, monotonic, monotonic_ns (perf_counter_ns intentionally not patched — used by profiling infrastructure)
  • Removed useless np.random.default_rng(42): Created a Generator and discarded it — no effect
  • Removed redundant try/except around os.urandom: os is always available

Test plan

  • uv run pytest tests/test_pytest_plugin_deterministic_patches.py -v — 23 tests pass
  • uv run pytest tests/ -x --timeout=60 — 2645 passed, 0 failed
  • uv run prek run --from-ref origin/main — ruff check + format pass

🤖 Generated with Claude Code

- Add re-entrancy guard to prevent infinite recursion if pytest_configure runs twice
- Remove all unnecessary calls to original functions in mocks (overhead + failure point)
- Actually patch datetime.datetime.now/utcnow via DeterministicDatetime subclass
- Fix datetime.now(tz=None) returning tz-aware instead of naive datetime
- Patch missing time functions: time_ns, monotonic, monotonic_ns
- Remove useless np.random.default_rng(42) that created and discarded a Generator
- Remove redundant try/except around os.urandom (os is always available)
- Update tests to cover all new patches and datetime behavior

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Mar 12, 2026

Claude finished @aseembits93's task in 3m 40s —— View job


PR Review Summary

  • Triage changed files
  • Run prek lint/type checks
  • Resolve stale review threads (none found)
  • Code review
  • Duplicate detection
  • Test coverage analysis

Prek Checks

ruff check and ruff format both pass.

Mypy Notes

Two new mypy errors introduced by this PR in _apply_deterministic_patches:

Line Error Status
175 now return type datetime.datetime incompatible with supertype's DeterministicDatetime — due to typeshed using TypeVar/Self for cls return type Left alone (complex generic, changing behavior to fix)
181 Same for utcnow Left alone
184 Cannot assign to a type for datetime.datetime = DeterministicDatetime Left alone (inherent to monkey-patching approach)

Auto-fixed (committed): removed unused _ORIGINAL_MONOTONIC variable (captured _time_module.monotonic but never referenced anywhere in the file) and added missing tz: datetime.tzinfo | None = None type annotation to DeterministicDatetime.now.

Code Review

The fixes in this PR are correct and well-motivated. A few observations:

DeterministicDatetime.now patching limitation (pytest_plugin.py:173–185): The datetime.datetime = DeterministicDatetime assignment patches the datetime module's class attribute. Code that did from datetime import datetime before pytest_configure runs won't see the mock. This is a known limitation of subclass-based datetime patching, and it's an improvement over the old approach (which stored mock functions to builtins but never actually patched datetime.datetime at all). Worth noting in a comment for future maintainers. Fix this →

_DETERMINISTIC_PATCHES_APPLIED never reset (pytest_plugin.py:97): Patches are one-shot and permanent for the session lifetime — which is correct for a pytest plugin. However, if two test processes each load the plugin module fresh (which they do), the flag resets correctly per-process. No issue here.

perf_counter_ns exclusion rationale (pytest_plugin.py:157–158): The comment correctly explains the carve-out. Good.

mock_datetime_now tz bug fixed (pytest_plugin.py:176–179): The old code with tz=None returned the tz-aware fixed_datetime directly, so callers expecting a naive datetime would get surprising results. The new code correctly returns fixed_datetime.replace(tzinfo=None) for the naive case and fixed_datetime.astimezone(tz) for the aware case.

Duplicate Detection

No duplicates detected. The DeterministicDatetime subclass pattern is unique to pytest_plugin.py.

Test Coverage

All 23 tests pass. Coverage for pytest_plugin.py is 26% overall, which is expected — the bulk of the plugin's code runs through pytest hooks that require a full test session to exercise, not standalone unit tests. The deterministic-patches section (the main change in this PR) is well-covered by the test suite.


Last updated: 2026-03-12
| Branch

Co-authored-by: Aseem Saxena <undefined@users.noreply.github.com>
@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Mar 12, 2026

⚡️ Codeflash found optimizations for this PR

📄 13% (0.13x) speedup for _apply_deterministic_patches in codeflash/verification/pytest_plugin.py

⏱️ Runtime : 37.5 microseconds 33.1 microseconds (best of 67 runs)

A new Optimization Review has been created.

🔗 Review here

Static Badge

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