test: increase coverage for metadata database module#1199
test: increase coverage for metadata database module#1199Riddhi8077 wants to merge 7 commits intoAOSSIE-Org:mainfrom
Conversation
|
|
WalkthroughReplaced a session-scoped autouse test setup with a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ 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 |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/.gitignore`:
- Around line 7-8: Remove the duplicate ignore patterns ".coverage" and
".pytest_cache/" that were added near the top of the .gitignore; these entries
already exist later in the "Unit test / coverage reports" section, so delete the
top occurrences and leave the existing entries in that section (look for the
patterns ".coverage" and ".pytest_cache/" to locate the duplicates).
In `@backend/tests/conftest.py`:
- Around line 28-93: The four test functions
(test_connection_creation_and_pragmas, test_commit_on_success,
test_rollback_on_exception, test_connection_closed_after_context) should be
removed from conftest.py and placed into the test module that already contains
the duplicate tests (e.g., test_connection.py), leaving conftest.py to only
expose the temp_db_path fixture; ensure the temp_db_path fixture (and imports
like get_db_connection) remain in conftest.py and that the moved tests import
nothing extra (they will use the fixture from conftest.py).
In `@backend/tests/database/test_connection.py`:
- Around line 1-93: This file duplicates the temp_db_path fixture and tests
already defined elsewhere; remove the duplication by keeping a single shared
fixture and the tests in one place: delete the temp_db_path fixture from this
file and ensure the fixture named temp_db_path remains in conftest (or,
alternatively, delete this entire file and keep the tests in conftest if you
prefer); specifically, keep the test functions
test_connection_creation_and_pragmas, test_commit_on_success,
test_rollback_on_exception, and test_connection_closed_after_context in only one
location and ensure the single temp_db_path fixture is defined only once
(conftest) so tests do not run twice or shadow each other.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/.gitignorebackend/tests/conftest.pybackend/tests/database/test_connection.pybackend/tests/database/test_metadata.py
|
|
1 similar comment
|
|
|
|
1 similar comment
|
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/tests/database/test_connection.py (1)
9-74:⚠️ Potential issue | 🟠 MajorConsolidate these tests into a single location to avoid duplicate execution.
backend/tests/database/test_connection.pycurrently duplicates the same four tests already present inbackend/tests/conftest.py(seebackend/tests/conftest.py:27-92). This will run the same assertions twice and increases maintenance cost. Keep these tests in one module only (preferably here) and leave only shared fixtures inconftest.py.As per coding guidelines, "Look for code duplication."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/database/test_connection.py` around lines 9 - 74, The four tests (test_connection_creation_and_pragmas, test_commit_on_success, test_rollback_on_exception, test_connection_closed_after_context) are duplicated elsewhere; remove the duplicate copies from conftest.py and keep these test implementations only in this test_connection.py module, leaving conftest.py to contain shared fixtures only (e.g., temp_db_path and get_db_connection provider functions). Ensure you only remove the test function definitions from conftest.py and not the fixture code (retaining any helper fixtures or imports used by these tests).
🧹 Nitpick comments (1)
backend/tests/database/test_connection.py (1)
52-56: Use a specific expected exception in the rollback-path test.At Line 52,
pytest.raises(Exception)is too broad and can hide unintended failures. Use a specific exception type raised by the test trigger.Suggested refinement
- with pytest.raises(Exception): + with pytest.raises(RuntimeError, match="Force rollback"): with get_db_connection() as conn: conn.execute("CREATE TABLE test (id INTEGER);") conn.execute("INSERT INTO test (id) VALUES (1);") - raise Exception("Force rollback") + raise RuntimeError("Force rollback")As per coding guidelines, "Ensure that test code is automated, comprehensive, and follows testing best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/database/test_connection.py` around lines 52 - 56, The test currently uses a broad pytest.raises(Exception) while the inner code raises a generic Exception; change the test to use a specific exception type (e.g., RuntimeError) by updating the raised error in the rollback block and matching pytest.raises(RuntimeError) so get_db_connection() rollback-path test asserts a precise exception; update the raise "Force rollback" to raise RuntimeError("Force rollback") and replace pytest.raises(Exception) with pytest.raises(RuntimeError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/tests/database/test_connection.py`:
- Around line 9-74: The four tests (test_connection_creation_and_pragmas,
test_commit_on_success, test_rollback_on_exception,
test_connection_closed_after_context) are duplicated elsewhere; remove the
duplicate copies from conftest.py and keep these test implementations only in
this test_connection.py module, leaving conftest.py to contain shared fixtures
only (e.g., temp_db_path and get_db_connection provider functions). Ensure you
only remove the test function definitions from conftest.py and not the fixture
code (retaining any helper fixtures or imports used by these tests).
---
Nitpick comments:
In `@backend/tests/database/test_connection.py`:
- Around line 52-56: The test currently uses a broad pytest.raises(Exception)
while the inner code raises a generic Exception; change the test to use a
specific exception type (e.g., RuntimeError) by updating the raised error in the
rollback block and matching pytest.raises(RuntimeError) so get_db_connection()
rollback-path test asserts a precise exception; update the raise "Force
rollback" to raise RuntimeError("Force rollback") and replace
pytest.raises(Exception) with pytest.raises(RuntimeError).
|
|
1 similar comment
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/conftest.py (1)
18-25: Table creation order could cause confusion with foreign key relationships.The
db_create_faces_table()(line 20) anddb_create_album_images_table()(line 23) reference theimagestable via foreign keys, butdb_create_images_table()is called later at line 24. While SQLite doesn't enforce FK existence at table creation time, this ordering is counterintuitive and could lead to issues if test data insertion is added to this fixture later.Consider reordering to create tables before their dependents:
♻️ Suggested reordering for clarity
db_create_YOLO_classes_table() + db_create_folders_table() + db_create_images_table() db_create_clusters_table() db_create_faces_table() - db_create_folders_table() db_create_albums_table() db_create_album_images_table() - db_create_images_table() db_create_metadata_table()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 18 - 25, Reorder the table-creation calls so parent tables are created before their dependents: move db_create_images_table() to run before db_create_faces_table() and db_create_album_images_table() (i.e., ensure db_create_images_table() is called prior to db_create_faces_table() and db_create_album_images_table()); keep other calls intact so any tables that reference images via foreign keys are created after images to avoid confusing ordering and potential future test-data insertion issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/conftest.py`:
- Around line 18-25: Reorder the table-creation calls so parent tables are
created before their dependents: move db_create_images_table() to run before
db_create_faces_table() and db_create_album_images_table() (i.e., ensure
db_create_images_table() is called prior to db_create_faces_table() and
db_create_album_images_table()); keep other calls intact so any tables that
reference images via foreign keys are created after images to avoid confusing
ordering and potential future test-data insertion issues.
|
|
1 similar comment
|
|
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 (1)
backend/tests/conftest.py (1)
1-13:⚠️ Potential issue | 🟡 MinorRemove unused imports.
sqlite3(line 4) and alldb_create_*functions (lines 7-13) are imported but never used in this file. These appear to be leftovers from the removedsetup_before_all_testsfixture.🧹 Proposed fix
import pytest import os import tempfile -import sqlite3 - -# Import database table creation functions -from app.database.faces import db_create_faces_table -from app.database.images import db_create_images_table -from app.database.face_clusters import db_create_clusters_table -from app.database.yolo_mapping import db_create_YOLO_classes_table -from app.database.albums import db_create_albums_table, db_create_album_images_table -from app.database.folders import db_create_folders_table -from app.database.metadata import db_create_metadata_table + `@pytest.fixture` def temp_db_path(monkeypatch):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 1 - 13, Remove the unused imports in backend/tests/conftest.py: delete the imported module symbol sqlite3 and the unused function symbols db_create_faces_table, db_create_images_table, db_create_clusters_table, db_create_YOLO_classes_table, db_create_albums_table, db_create_album_images_table, db_create_folders_table, and db_create_metadata_table so the file only imports pytest, os, and tempfile (or whatever remaining imports are actually used).
🧹 Nitpick comments (1)
backend/tests/conftest.py (1)
48-48: Remove trailing whitespace.Line 48 contains trailing whitespace which violates PEP 8.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` at line 48, Remove the trailing whitespace on line 48 of backend/tests/conftest.py (strip the extra space/whitespace character at the end of that line) so the file conforms to PEP 8; no code logic changes are required—just delete the trailing space and save the file.
🤖 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 `@backend/tests/conftest.py`:
- Around line 1-13: Remove the unused imports in backend/tests/conftest.py:
delete the imported module symbol sqlite3 and the unused function symbols
db_create_faces_table, db_create_images_table, db_create_clusters_table,
db_create_YOLO_classes_table, db_create_albums_table,
db_create_album_images_table, db_create_folders_table, and
db_create_metadata_table so the file only imports pytest, os, and tempfile (or
whatever remaining imports are actually used).
---
Nitpick comments:
In `@backend/tests/conftest.py`:
- Line 48: Remove the trailing whitespace on line 48 of
backend/tests/conftest.py (strip the extra space/whitespace character at the end
of that line) so the file conforms to PEP 8; no code logic changes are
required—just delete the trailing space and save the file.
|
@coderabbitai Review |
✅ Actions performedReview triggered.
|
✅ Actions performedReviews resumed. |
Removed unused imports and commented-out code from conftest.py.
|
|
1 similar comment
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/conftest.py (1)
6-36: Good refactoring — test functions properly moved out.The fixture-only structure now follows pytest conventions. The
temp_db_pathfixture provides good test isolation with proper cleanup.Two observations:
raising=Falsesilently ignores missing attributes — If a module doesn't haveDATABASE_PATH, the patch will be skipped without notice. This is fine if intentional (e.g., future-proofing), but could mask configuration errors.Type hints are missing — Consider adding for consistency with Python best practices.
Optional: Add type hints
+from typing import Generator +from pytest import MonkeyPatch + + `@pytest.fixture` -def temp_db_path(monkeypatch): +def temp_db_path(monkeypatch: MonkeyPatch) -> Generator[str, None, None]: """ Creates a temporary SQLite database file and patches DATABASE_PATH in all database modules. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 6 - 36, The temp_db_path pytest fixture currently uses monkeypatch.setattr(..., raising=False) which silently skips if a target module/attribute is missing; change this to raising=True (or explicitly import each module and assert hasattr(module, "DATABASE_PATH") before patching) so missing DATABASE_PATH attributes raise errors during tests; also add a return type hint to the fixture signature (temp_db_path -> str or Iterator[str] if using yield) and import the corresponding typing symbol (e.g., Iterator) so the fixture has explicit type annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/tests/conftest.py`:
- Around line 6-36: The temp_db_path pytest fixture currently uses
monkeypatch.setattr(..., raising=False) which silently skips if a target
module/attribute is missing; change this to raising=True (or explicitly import
each module and assert hasattr(module, "DATABASE_PATH") before patching) so
missing DATABASE_PATH attributes raise errors during tests; also add a return
type hint to the fixture signature (temp_db_path -> str or Iterator[str] if
using yield) and import the corresponding typing symbol (e.g., Iterator) so the
fixture has explicit type annotations.
1 participant
This PR increases test coverage for the metadata database module.
Functions Covered:
Changes:
Coverage:
Before: 14%
After: 88%
How to run tests:
pytest --cov=app --cov-report=term-missing
Summary by CodeRabbit
Tests
Chores