fix(tests): mark profiler timing tests with @allow_sleep (follow-up #1580)#1581
Conversation
…1580) The merged version of PR #1580 tightened the profiler-test bounds from '> 0' to '>= 5' ms, but failed to opt out of conftest's autouse 'fast_sleep' fixture (tests/conftest.py:183) which clamps time.sleep to 1ms. Result: test_api_call_context_manager and test_streaming_tracker fail with ~1.1ms duration on every run. Add the documented opt-out marker (@pytest.mark.allow_sleep) so the tests get a real 50ms sleep and the >= 5ms assertion is meaningful. Verified locally: all 12 sandbox + profiler tests pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryAdds Confidence Score: 5/5Test-only change; safe to merge immediately. The change is minimal and correct: the allow_sleep marker is already registered in pytest.ini and handled by conftest.py, no production code is touched, and the fix directly addresses the CI failures introduced by #1580. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant pytest
participant conftest as conftest.py (fast_sleep)
participant test as test_profiler_advanced.py
participant sleep as time.sleep
Note over pytest,sleep: Before fix — fast_sleep clamps sleep to 1ms
pytest->>conftest: autouse fast_sleep fixture
conftest->>sleep: monkeypatch → 0.001s max
test->>sleep: time.sleep(0.05)
sleep-->>test: returns after ~1ms
test->>test: assert duration_ms >= 5 ❌ (~1ms)
Note over pytest,sleep: After fix — @allow_sleep opts out
pytest->>conftest: autouse fast_sleep fixture
conftest->>conftest: sees allow_sleep marker → return early
test->>sleep: time.sleep(0.05)
sleep-->>test: returns after ~50ms
test->>test: assert duration_ms >= 5 ✅ (~50ms)
Reviews (2): Last reviewed commit: "refactor(tests): move allow_sleep explan..." | Re-trigger Greptile |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the @pytest.mark.allow_sleep decorator to the test_api_call_context_manager and test_streaming_tracker unit tests in test_profiler_advanced.py. This allows the tests to bypass the fast_sleep fixture and measure actual wall-clock duration. The review feedback suggests moving the inline comments explaining these changes into the test docstrings to better adhere to PEP 257 documentation standards.
| # ``allow_sleep`` opts out of the conftest ``fast_sleep`` fixture which | ||
| # otherwise clamps ``time.sleep`` to 1ms — needed here because the test | ||
| # asserts a real wall-clock duration was measured. |
There was a problem hiding this comment.
This is a helpful comment explaining the need for @pytest.mark.allow_sleep. For better documentation and adherence to Python's style guides, this explanation should be moved into the test's docstring. This makes the purpose of the test and its requirements clearer to anyone reading the docstring.
For example:
def test_api_call_context_manager(self):
"""Should profile API call with context manager.
The ``@allow_sleep`` marker opts out of the conftest ``fast_sleep`` fixture which
otherwise clamps ``time.sleep`` to 1ms. This is needed here because the test
asserts a real wall-clock duration was measured.
"""References
- According to PEP 257, docstrings should contain explanations of a function's purpose and behavior. This comment would be better placed inside the function's docstring. (link)
| # ``allow_sleep`` opts out of conftest ``fast_sleep`` clamping so the | ||
| # TTFT measurement reflects real elapsed time. |
There was a problem hiding this comment.
Similar to the other test, this explanatory comment is valuable but would be more appropriately located within the test's docstring for better discoverability and consistency.
For example:
def test_streaming_tracker(self):
"""Should track streaming with context manager.
The ``@allow_sleep`` marker opts out of conftest ``fast_sleep`` clamping so the
TTFT measurement reflects real elapsed time.
"""References
- According to PEP 257, docstrings should contain explanations of a function's purpose and behavior. This comment would be better placed inside the function's docstring. (link)
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Todo List:
Scope: Python packages only (praisonaiagents, praisonai) - no Rust/TypeScript modifications. Phase 1: Architecture Review per AGENTS.md✅ Protocol-driven: This is a test-only change - no core SDK modifications. ✅ Backward compatible: Zero feature regressions - only fixes failing tests by allowing real ✅ Performance: No hot-path impact - test execution only. Phase 2: Issues Fixed✅ Addressed Gemini's feedback: Moved inline comments explaining Files Modified:
Commit: Phase 3: Final Verdict✅ APPROVED - This PR is ready to merge. Summary:
Risk Assessment: Minimal - test infrastructure improvement only, no runtime behavior changes. |
Per Gemini code review feedback, moved inline comments explaining the @pytest.mark.allow_sleep marker into test docstrings for better PEP 257 compliance. The marker opts out of conftest fast_sleep fixture clamping. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Why
The merged version of #1580 tightened the profiler-test bounds from
> 0to>= 5ms during review, but failed to opt out of conftest's autousefast_sleepfixture (tests/conftest.py:183) which clampstime.sleepto 1ms for unit tests.Result on
main:Fix
Add
@pytest.mark.allow_sleepto both timing-sensitive tests \u2014 the documented opt-out from thefast_sleepclamp. The 50ms sleep then runs for real and the>= 5msassertion becomes meaningful.Verification
PYTHONPATH=src/praisonai-agents:src/praisonai pytest \ src/praisonai/tests/unit/test_profiler_advanced.py::TestAPICallProfiler::test_api_call_context_manager \ src/praisonai/tests/unit/test_profiler_advanced.py::TestStreamingProfiler::test_streaming_tracker # 2 passedCombined with sandbox + ag2 + push_client suites: 12 passed, 2 skipped, 0 failed.
Risk
Test-only diff. No runtime behavior changes.
Summary by CodeRabbit