add database tests for albums and faces (#1238)#1342
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds unit tests for album and face database helpers, covering table creation, album query and write paths, password verification, image batch deletion, face embedding insertion, cluster queries and updates, and cluster mean calculations. ChangesDatabase helper tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/test_albums_db.py (2)
21-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant test comments that restate method names.
Most inline comments just repeat what the test names already say, adding noise without extra clarity.
As per path instructions, “Point out redundant obvious comments that do not add clarity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_albums_db.py` around lines 21 - 293, Remove the redundant inline test comments in the album DB tests that only restate the surrounding method names, and keep only comments that add extra context or intent. Update the tests around db_create_albums_table, db_get_all_albums, db_get_album_by_name, db_get_album, db_insert_album, db_update_album, db_delete_album, db_get_album_images, db_remove_images_from_album, and verify_album_password so the file is less noisy and the useful assertions remain easy to scan.Source: Path instructions
23-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefactor repeated mock setup into fixtures/helpers.
The same connect/conn/cursor setup is duplicated across many tests. A shared fixture (e.g., returning
(mock_connect, mock_conn, mock_cursor)) would reduce repetition and make intent clearer.As per path instructions, “Look for code duplication.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_albums_db.py` around lines 23 - 301, The album database tests repeat the same sqlite3 connect/conn/cursor mocking setup across many methods, so refactor the duplicated arrangement into a shared pytest fixture or small helper that returns the mocked connect, connection, and cursor. Update the tests in TestDbCreateAlbumsTable, TestDbGetAllAlbums, TestDbGetAlbumByName, TestDbGetAlbum, TestDbInsertAlbum, TestDbUpdateAlbum, TestDbGetAlbumImages, TestDbRemoveImagesFromAlbum, and TestVerifyAlbumPassword to use that shared setup while keeping each assertion focused on the behavior under test.Source: Path instructions
backend/tests/test_faces_db.py (1)
19-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared sqlite mock setup into pytest fixtures to remove duplication.
The repeated
connectpatch +conn/cursorwiring appears in most tests; a fixture will make tests shorter and easier to maintain.As per path instructions, “Look for code duplication.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_faces_db.py` around lines 19 - 219, The tests in TestDbCreateFacesTable, TestDbInsertFaceEmbeddings, TestDbGetFacesUnassignedClusters, TestDbGetAllFacesWithClusterNames, TestDbUpdateFaceClusterIdsBatch, and TestDbGetClusterMeanEmbeddings repeat the same sqlite3.connect patch and mock connection/cursor wiring. Extract that shared setup into a pytest fixture (or helper fixture pair) that yields the mock connect/conn/cursor objects, then update each test to reuse it and keep only the scenario-specific assertions and side effects.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_albums_db.py`:
- Around line 41-50: The TestDbCreateAlbumImagesTable class is missing the
failure-path coverage for db_create_album_images_table()’s finally-close
behavior. Add a test that patches app.database.albums.sqlite3.connect, makes
mock_conn.cursor().execute raise an exception, and asserts that mock_conn.close
is still called even when the insert/create logic fails. Reuse the same style as
the existing success test, and target the db_create_album_images_table function
so the resource cleanup contract is verified.
In `@backend/tests/test_faces_db.py`:
- Around line 86-220: Add connection cleanup assertions to the remaining
database helper tests so the DB helper contract is fully covered. In the test
classes for db_get_faces_unassigned_clusters,
db_get_all_faces_with_cluster_names, db_update_face_cluster_ids_batch, and
db_get_cluster_mean_embeddings, verify the mocked connection’s close method is
called after each helper runs, matching the cleanup coverage already present for
table creation/insert. Use the existing sqlite3.connect patch setup and the
helper function names to place the assertions without changing the production
logic.
---
Nitpick comments:
In `@backend/tests/test_albums_db.py`:
- Around line 21-293: Remove the redundant inline test comments in the album DB
tests that only restate the surrounding method names, and keep only comments
that add extra context or intent. Update the tests around
db_create_albums_table, db_get_all_albums, db_get_album_by_name, db_get_album,
db_insert_album, db_update_album, db_delete_album, db_get_album_images,
db_remove_images_from_album, and verify_album_password so the file is less noisy
and the useful assertions remain easy to scan.
- Around line 23-301: The album database tests repeat the same sqlite3
connect/conn/cursor mocking setup across many methods, so refactor the
duplicated arrangement into a shared pytest fixture or small helper that returns
the mocked connect, connection, and cursor. Update the tests in
TestDbCreateAlbumsTable, TestDbGetAllAlbums, TestDbGetAlbumByName,
TestDbGetAlbum, TestDbInsertAlbum, TestDbUpdateAlbum, TestDbGetAlbumImages,
TestDbRemoveImagesFromAlbum, and TestVerifyAlbumPassword to use that shared
setup while keeping each assertion focused on the behavior under test.
In `@backend/tests/test_faces_db.py`:
- Around line 19-219: The tests in TestDbCreateFacesTable,
TestDbInsertFaceEmbeddings, TestDbGetFacesUnassignedClusters,
TestDbGetAllFacesWithClusterNames, TestDbUpdateFaceClusterIdsBatch, and
TestDbGetClusterMeanEmbeddings repeat the same sqlite3.connect patch and mock
connection/cursor wiring. Extract that shared setup into a pytest fixture (or
helper fixture pair) that yields the mock connect/conn/cursor objects, then
update each test to reuse it and keep only the scenario-specific assertions and
side effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e87f89e-bef4-4dd5-90ef-b11a2d576c95
📒 Files selected for processing (2)
backend/tests/test_albums_db.pybackend/tests/test_faces_db.py
Signed-off-by: Mansi2007275 <mansi@example.com>
|
@rohan-pandeyy please review my PR.....if its okay then merged it .... |
1 participant
Summary
Closes #1238
Adds unit tests for the database layer for:
backend/app/database/albums.pybackend/app/database/faces.pyTest files added
backend/tests/test_albums_db.py— 18 testsbackend/tests/test_faces_db.py— 14 testsWhat is covered
Notes
unittest.mockto avoid real DB callstest_albums.pySummary by CodeRabbit