Conversation
Quite significant speedups for the `php_api` suite, locally from 140s to 90s.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUpdated async type annotations and test fixtures. In src/main.py the Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The session-scoped
appfixture has itsdependency_overridesmutated in the function-scopedpy_apifixture but never reset, so consider clearing or restoringapp.dependency_overridesafter each test to avoid cross-test leakage of overrides. - The comment in
py_apiabout not using the Lifespan manager and relying on the removedone_lifespanfixture is now outdated and should be updated or removed to reflect the new fixture structure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The session-scoped `app` fixture has its `dependency_overrides` mutated in the function-scoped `py_api` fixture but never reset, so consider clearing or restoring `app.dependency_overrides` after each test to avoid cross-test leakage of overrides.
- The comment in `py_api` about not using the Lifespan manager and relying on the removed `one_lifespan` fixture is now outdated and should be updated or removed to reflect the new fixture structure.
## Individual Comments
### Comment 1
<location path="tests/conftest.py" line_range="105-106" />
<code_context>
app.dependency_overrides[expdb_connection] = override_expdb
app.dependency_overrides[userdb_connection] = override_userdb
+
# We do not use the Lifespan manager for now because our auto-use fixture
# `one_lifespan` will do setup and teardown at a session scope level instead.
async with httpx.AsyncClient(
</code_context>
<issue_to_address>
**suggestion:** Update or remove the outdated comment about not using the Lifespan manager.
After the fixture restructuring, the session-scoped `app` fixture now uses `LifespanManager` and the `one_lifespan` fixture no longer exists, so this comment no longer reflects reality. Please adjust it to describe the current fixture setup (session-scoped `app` with `LifespanManager` plus function-scoped DB fixtures) or remove it to avoid confusion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
=======================================
Coverage 93.56% 93.57%
=======================================
Files 72 72
Lines 3109 3113 +4
Branches 222 222
=======================================
+ Hits 2909 2913 +4
Misses 143 143
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.py (1)
40-43:⚠️ Potential issue | 🔴 CriticalAwait shutdown tasks in the lifespan cleanup.
At line 40,
asyncio.gather()is called but never awaited, preventinglogger.complete()andclose_databases()from executing. This causes resource leaks on shutdown.Proposed fix
- asyncio.gather( + await asyncio.gather( logger.complete(), close_databases(), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.py` around lines 40 - 43, The shutdown code calls asyncio.gather(...) but does not await it, so logger.complete() and close_databases() never run; update the lifespan cleanup to await the gather call (await asyncio.gather(...)) or await each coroutine directly so logger.complete() and close_databases() are executed and resources are closed; locate the gather invocation using asyncio.gather and the coroutines logger.complete() and close_databases() and change the call to be awaited.tests/conftest.py (1)
102-113:⚠️ Potential issue | 🟠 MajorRestore dependency overrides after each
py_apiuse to prevent test contamination.The overrides set at lines 102–103 are applied to a session-scoped
appinstance and never restored, leaving stale overrides that can affect subsequent tests. Additionally, the comment at lines 105–106 references aone_lifespanfixture that no longer exists in the codebase.Proposed fix
- app.dependency_overrides[expdb_connection] = override_expdb - app.dependency_overrides[userdb_connection] = override_userdb - - # We do not use the Lifespan manager for now because our auto-use fixture - # `one_lifespan` will do setup and teardown at a session scope level instead. - async with httpx.AsyncClient( - transport=httpx.ASGITransport(app=app), - base_url="http://test", - follow_redirects=True, - ) as client: - yield client + previous_expdb_override = app.dependency_overrides.get(expdb_connection) + previous_userdb_override = app.dependency_overrides.get(userdb_connection) + app.dependency_overrides[expdb_connection] = override_expdb + app.dependency_overrides[userdb_connection] = override_userdb + try: + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://test", + follow_redirects=True, + ) as client: + yield client + finally: + if previous_expdb_override is None: + app.dependency_overrides.pop(expdb_connection, None) + else: + app.dependency_overrides[expdb_connection] = previous_expdb_override + + if previous_userdb_override is None: + app.dependency_overrides.pop(userdb_connection, None) + else: + app.dependency_overrides[userdb_connection] = previous_userdb_override🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 102 - 113, The session-scoped dependency overrides (app.dependency_overrides) set for expdb_connection and userdb_connection must be restored after the py_api fixture to avoid test contamination: wrap the override assignment and the async httpx.AsyncClient context in a try/finally, capture any previous overrides for expdb_connection and userdb_connection before assigning override_expdb/override_userdb, yield the client inside the try, and in the finally restore the originals (or delete the keys if originals were None); also remove or update the stale comment referencing the non-existent one_lifespan fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main.py`:
- Around line 40-43: The shutdown code calls asyncio.gather(...) but does not
await it, so logger.complete() and close_databases() never run; update the
lifespan cleanup to await the gather call (await asyncio.gather(...)) or await
each coroutine directly so logger.complete() and close_databases() are executed
and resources are closed; locate the gather invocation using asyncio.gather and
the coroutines logger.complete() and close_databases() and change the call to be
awaited.
In `@tests/conftest.py`:
- Around line 102-113: The session-scoped dependency overrides
(app.dependency_overrides) set for expdb_connection and userdb_connection must
be restored after the py_api fixture to avoid test contamination: wrap the
override assignment and the async httpx.AsyncClient context in a try/finally,
capture any previous overrides for expdb_connection and userdb_connection before
assigning override_expdb/override_userdb, yield the client inside the try, and
in the finally restore the originals (or delete the keys if originals were
None); also remove or update the stale comment referencing the non-existent
one_lifespan fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c9c75871-d0ea-4335-adf4-a01e88ca723d
📒 Files selected for processing (4)
src/main.pysrc/routers/dependencies.pytests/conftest.pytests/routers/openml/migration/setups_migration_test.py
1 participant
Restructure fixtures to reduce per-test fixture overhead.