Testcases: Using DB Transactions #278
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4253 @fatchat Hello! I see you've tagged me on PR #278 which introduces per-test database transactions for proper test isolation and rollback. This looks like a significant improvement to the testing infrastructure! The codecov report shows excellent coverage - all modified lines are covered by tests, which is great to see for testing infrastructure changes. How can I help you with this PR? Would you like me to:
Just let me know what you need assistance with! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@fatchat I see you've tagged me again! I'm ready to help with PR #278. Could you please let me know what specific assistance you need? For example:
Just let me know what you'd like me to focus on and I'll provide detailed assistance! 🏁 Script executed: Length of output: 9186 @fatchat Great! I've reviewed the changes in PR #278. This is an excellent improvement to your testing infrastructure! Let me break down what I see: 🎯 Key Improvements1. Per-Test Transaction Isolation
2. Smart Savepoint Management nested = session.begin_nested()
@event.listens_for(session, "after_transaction_end")
def restart_savepoint(sess, trans):
if trans.nested and not trans._parent.nested:
sess.begin_nested()This is particularly clever - it allows your application code to call 3. Baseline Data Seeding
4. Dependency Override app.dependency_overrides[get_db] = lambda: dbThis ensures FastAPI uses the test's transaction-bound session instead of opening new connections. ✅ Benefits
🤔 ConsiderationsThe implementation looks solid overall. The only thing to watch for is ensuring that any tests requiring committed data (like testing background tasks that query the database) work correctly with this setup. 🔍 Minor NoteIn the test file, you added Is there anything specific about this testing setup you'd like me to dive deeper into or any questions about the implementation? |
nishika26
changed the title
AkhileshNegi
changed the title
WalkthroughTest fixtures in the FastAPI backend were refactored to use transactional savepoints for improved test isolation. Fixture scopes for Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/app/tests/api/routes/collections/test_collection_info.py (1)
11-11: Critical: Remove module-level client and use the fixture parameter.The module-level
clienton line 11 bypasses the transactional testing setup. Test functions receive the properclientfixture but continue using the module-level instance, which doesn't have theget_dbdependency override.Remove the module-level client:
-client = TestClient(app) - original_api_key = "ApiKey No3x47A5qoIGhm0kVKjQ77dhCqEdWRIQZlEPzzzh7i8"The test functions already receive the correct
clientparameter from the fixture and will use it automatically since there's no module-level variable shadowing it.Also applies to: 41-98
🧹 Nitpick comments (1)
backend/app/tests/conftest.py (1)
17-35: Excellent implementation of transactional test isolation!The nested transaction approach with automatic savepoint re-establishment is a robust solution that allows application code to commit normally while maintaining test isolation.
Regarding the static analysis hint about the unused
nestedvariable on line 23: This is a false positive. The variable starts the nested transaction which remains active throughout the test. Consider adding a comment to clarify this for future maintainers.def db() -> Generator[Session, None, None]: connection = engine.connect() transaction = connection.begin() session = Session(bind=connection) - nested = session.begin_nested() + # Start nested transaction - variable not referenced but transaction remains active + nested = session.begin_nested() @event.listens_for(session, "after_transaction_end") def restart_savepoint(sess, trans):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/tests/api/routes/collections/test_collection_info.py(3 hunks)backend/app/tests/conftest.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app/tests/conftest.py
23-23: Local variable nested is assigned to but never used
Remove assignment to unused variable nested
(F841)
🔇 Additional comments (2)
backend/app/tests/conftest.py (2)
38-42: Well-designed baseline data seeding approach.Moving the seeding to a separate session-scoped fixture ensures deterministic baseline data is available for all tests while keeping it separate from the per-test transactional sessions.
45-49: Correct alignment of client fixture with transactional testing.The function-scoped client properly integrates with the transactional db fixture by overriding the
get_dbdependency, ensuring routes see the test's uncommitted data.
4 participants
Summary
Target issue is #274
Explain the motivation for making this change. What existing problem does the pull request solve?
Previously, our test suite used a db fixture that maintained a single long-lived session for the entire pytest session, along with a single TestClient.
This approach caused all tests to operate on the same committed database state and share the same session throughout the test run, increasing the risk of stale session state and unintended interactions between tests.
Additionally, cleanup required explicitly deleting data at the end of the session, which was cumbersome.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Converted the db fixture to:
Use scope="function" (per-test) instead of session.
Open a transaction for each test and roll it back after the test finishes, ensuring a base DB for every test.
Now when we will have this transaction setup in place, our test data won't touch our real data. Every test runs in an isolated sandbox and rolls back at the end.
Add a savepoint (session.begin_nested()) at the start of each test to allow app code to safely call .commit() during tests without breaking the outer transaction.
Kept the seed_baseline fixture at scope="session":
Runs only once before the first test.
Seeds deterministic baseline data that all tests can rely on. to understand more, check this
Overrode get_db in the client fixture:
Ensures that FastAPI’s get_db dependency returns the test’s transaction-bound session (db).
Without this, FastAPI routes would open their own sessions, which would not see uncommitted test data. To understand more about, check here
Made client, superuser_token_headers, and normal_user_token_headers function-scoped to align with the per-test db.
Summary by CodeRabbit