Add db connection test and album test#1235
Closed
NancyWei123 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Closed
Add db connection test and album test#1235NancyWei123 wants to merge 2 commits intoAOSSIE-Org:mainfrom
NancyWei123 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
added 2 commits
March 10, 2026 23:23
Contributor
|
|
Contributor
WalkthroughTest configuration and comprehensive test suites were added across multiple layers: database connection management, album CRUD operations, image-album associations, and album API endpoints. The changes include modifying the test configuration file to enable proper module imports and introducing approximately 900 lines of new test code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
Contributor
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
backend/tests/test_db/test_images.py (1)
74-83: PEP 8: Missing spaces around comparison operators.- assert image[0]=="ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494" + assert image[0] == "ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494" ... - assert image ==[] + assert image == []As per coding guidelines, Python code should follow PEP 8 conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_db/test_images.py` around lines 74 - 83, The test contains PEP8 spacing issues around comparison operators; update the assertions and equality checks in this block (references: variable "image", assertion using "assert image[0]==\"ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494\"", the pytest.raises block calling db_remove_image_from_album, and the final check "assert image ==[]") to include spaces around operators (e.g., change "image[0]==..." to "image[0] == ...", "assert image ==[]" to "assert image == []") so the code conforms to PEP8.backend/tests/test_routes/test_albums.py (1)
1-11: Remove redundantsys.pathmanipulation.Lines 1-2 import
sysandos, and line 11 appends tosys.path. This is redundant sinceconftest.pyalready handles path configuration at the session level. The imports can be cleaned up:-import sys -import os import pytest import bcrypt from fastapi import FastAPI from fastapi.testclient import TestClient from unittest.mock import patch import uuid from app.routes import albums as albums_router - -sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "..")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_routes/test_albums.py` around lines 1 - 11, In tests/test_albums.py remove the redundant sys.path manipulation: delete the imports of sys and os and the sys.path.append(...) statement (they're unnecessary because conftest.py already sets up PYTHONPATH), leaving the rest of the imports and the reference to albums_router intact; ensure there are no unused-import warnings after removing sys/os.backend/tests/test_db/test_connect.py (1)
56-67: Simplify PRAGMA assertion usingunittest.mock.callhelper.The current assertion logic is correct but verbose. Consider using
assert_has_callsfor cleaner verification:- expected_calls = [ - (("PRAGMA foreign_keys = ON;",),), - (("PRAGMA ignore_check_constraints = OFF;",),), - (("PRAGMA recursive_triggers = ON;",),), - (("PRAGMA defer_foreign_keys = OFF;",),), - (("PRAGMA case_sensitive_like = ON;",),), - ] - - executed = [call.args for call in mock_conn.execute.call_args_list] - - for pragma_call in expected_calls: - assert pragma_call[0] in executed + from unittest.mock import call + expected_calls = [ + call("PRAGMA foreign_keys = ON;"), + call("PRAGMA ignore_check_constraints = OFF;"), + call("PRAGMA recursive_triggers = ON;"), + call("PRAGMA defer_foreign_keys = OFF;"), + call("PRAGMA case_sensitive_like = ON;"), + ] + mock_conn.execute.assert_has_calls(expected_calls, any_order=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_db/test_connect.py` around lines 56 - 67, The loop that checks each PRAGMA via executed = [call.args for call in mock_conn.execute.call_args_list] can be simplified: import unittest.mock.call (or use mock.call) and build expected_calls as [call("PRAGMA foreign_keys = ON;"), call("PRAGMA ignore_check_constraints = OFF;"), ...] then replace the manual for-loop with mock_conn.execute.assert_has_calls(expected_calls, any_order=True) so the test uses assert_has_calls against mock_conn.execute instead of iterating over mock_conn.execute.call_args_list.backend/tests/conftest.py (1)
1-5: Consider using pytest'spythonpathconfiguration instead ofsys.pathmanipulation.Modifying
sys.pathat import time works but is fragile and can lead to inconsistencies. A cleaner approach is to configure pytest'spythonpathinpyproject.tomlorpytest.ini:# pytest.ini [pytest] pythonpath = backendThis centralizes path configuration and ensures consistent module resolution across all test files.
🤖 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 - 5, Replace the ad-hoc sys.path modification in conftest.py (the sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) call and its imports of sys/os) with a pytest configuration entry so tests use the project package root consistently; remove the sys.path.append line and corresponding imports from backend/tests/conftest.py, and add a pytest [pythonpath] entry in pytest.ini or pyproject.toml (e.g., pythonpath = backend) so pytest handles module resolution instead of manipulating sys.path at import time.backend/tests/test_db/test_albums.py (2)
34-43: Unnecessary f-string usage and inconsistentdb_insert_albumparameter count.
- Line 36:
f"Test Album"is an f-string without placeholders; use a plain string.- Lines 38-43:
db_insert_albumis called with 4 positional arguments, but line 113 calls it with only 2. Review the function signature to ensure consistent usage.- album_name = f"Test Album" + album_name = "Test Album"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_db/test_albums.py` around lines 34 - 43, Replace the unnecessary f-string in test_insert_and_get_album (change f"Test Album" to "Test Album") and make db_insert_album usage consistent with its actual signature: inspect the db_insert_album function and then either (a) update this test's call in test_insert_and_get_album to pass the correct number and names of parameters (use explicit keyword arguments like db_insert_album(album_id=..., name=..., description=..., public=...) if the signature supports them) or (b) if other tests (e.g., the call around line 113) assume a two-argument signature, refactor db_insert_album to accept the shorter form with sensible defaults or overloads so all test calls use the same parameter shape; ensure the symbol db_insert_album is updated consistently across tests.
77-95: Missing space in variable assignment (PEP 8).Line 80 violates PEP 8 spacing conventions:
- album_password_id=str(uuid.uuid4()) + album_password_id = str(uuid.uuid4())As per coding guidelines, Python code should follow PEP 8 conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_db/test_albums.py` around lines 77 - 95, In test_update_album, fix PEP8 spacing for the album_password_id assignment inside the function: change the declaration of the album_password_id variable in test_update_album so there is a space on both sides of the '=' (i.e., album_password_id = str(uuid.uuid4())), ensuring consistent spacing around assignment operators in that test function.
🤖 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/tests/test_connect.py`:
- Around line 1-67: Remove the duplicate test file that defines the
TestDatabaseConnection class and exercises get_db_connection (the redundant copy
that contains the four tests: test_get_db_connection_returns_connection,
test_commit_on_success, test_rollback_on_exception, test_pragmas_are_executed);
keep only the canonical test file (the other copy) so pytest discovers a single
source of truth for these connection tests and delete this redundant file from
the repository.
- Around line 25-30: The test patches the global "sqlite3.connect" which can
miss the actual call site; change the patch target to the module where
get_db_connection is defined so you're mocking the usage site (e.g.
patch("<module_where_get_db_connection_is_defined>.sqlite3.connect",
return_value=mock_conn)), keep using get_db_connection() in the with block and
assert conn == mock_conn, then verify mock_conn.commit.assert_called_once() and
mock_conn.close.assert_called_once(); update the import/target string to the
actual module name that defines get_db_connection.
In `@backend/tests/test_db/test_albums.py`:
- Around line 1-11: Remove the unused imports and fix the module path: delete
the unused imports os and sys and remove db_get_album_images and
db_add_images_to_album from the import list, and change the long import prefix
from backend.app... to app... so the file imports DATABASE_PATH and the used DB
functions (verify_album_password, db_insert_album, db_delete_album,
db_get_album, db_update_album, db_get_all_albums, db_get_album_by_name) from
app.config.settings and app.database.albums respectively; keep the remaining
import names unchanged so tests still reference verify_album_password,
db_insert_album, db_delete_album, db_get_album, db_update_album,
db_get_all_albums, and db_get_album_by_name.
In `@backend/tests/test_db/test_connect.py`:
- Around line 25-30: The tests currently patch "sqlite3.connect" directly but
should patch it in the module under test; update the patch target to point to
the module where get_db_connection is defined (patch the connect symbol used by
that module) so the mock intercepts calls inside get_db_connection; apply this
change for all three test methods that mock sqlite3.connect (the test blocks
around get_db_connection) so mock_conn is used and assertions on
mock_conn.commit and mock_conn.close remain valid.
In `@backend/tests/test_db/test_images.py`:
- Around line 6-9: Update the import statements in this test to match the
project convention used by other tests (use "from app..." rather than "from
backend.app...") and remove unused imports; specifically change the module
imports for DATABASE_PATH, db_insert_album, db_get_album_images,
db_add_images_to_album, db_remove_image_from_album, db_remove_images_from_album
to use the "app" package path and delete the unused ImageRecord and
db_get_all_images imports from backend.app.database.images so only the actually
used symbols remain.
- Around line 85-107: The test is failing due to duplicate primary key and
UNIQUE constraint collisions: create new unique values for img1["id"] and
img2["id"] (different from the earlier img and any prior test fixtures) and
ensure img2["thumbnailPath"] is unique (not equal to img1["thumbnailPath"] or
any previously inserted thumbnailPath); update the "path" and "metadata" fields
if they must also be unique in the DB, and then call db_bulk_insert_images with
the two fresh, non-duplicated dicts (symbols to edit: img1, img2, their "id" and
"thumbnailPath" fields, and the call to db_bulk_insert_images).
---
Nitpick comments:
In `@backend/tests/conftest.py`:
- Around line 1-5: Replace the ad-hoc sys.path modification in conftest.py (the
sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "..")))
call and its imports of sys/os) with a pytest configuration entry so tests use
the project package root consistently; remove the sys.path.append line and
corresponding imports from backend/tests/conftest.py, and add a pytest
[pythonpath] entry in pytest.ini or pyproject.toml (e.g., pythonpath = backend)
so pytest handles module resolution instead of manipulating sys.path at import
time.
In `@backend/tests/test_db/test_albums.py`:
- Around line 34-43: Replace the unnecessary f-string in
test_insert_and_get_album (change f"Test Album" to "Test Album") and make
db_insert_album usage consistent with its actual signature: inspect the
db_insert_album function and then either (a) update this test's call in
test_insert_and_get_album to pass the correct number and names of parameters
(use explicit keyword arguments like db_insert_album(album_id=..., name=...,
description=..., public=...) if the signature supports them) or (b) if other
tests (e.g., the call around line 113) assume a two-argument signature, refactor
db_insert_album to accept the shorter form with sensible defaults or overloads
so all test calls use the same parameter shape; ensure the symbol
db_insert_album is updated consistently across tests.
- Around line 77-95: In test_update_album, fix PEP8 spacing for the
album_password_id assignment inside the function: change the declaration of the
album_password_id variable in test_update_album so there is a space on both
sides of the '=' (i.e., album_password_id = str(uuid.uuid4())), ensuring
consistent spacing around assignment operators in that test function.
In `@backend/tests/test_db/test_connect.py`:
- Around line 56-67: The loop that checks each PRAGMA via executed = [call.args
for call in mock_conn.execute.call_args_list] can be simplified: import
unittest.mock.call (or use mock.call) and build expected_calls as [call("PRAGMA
foreign_keys = ON;"), call("PRAGMA ignore_check_constraints = OFF;"), ...] then
replace the manual for-loop with
mock_conn.execute.assert_has_calls(expected_calls, any_order=True) so the test
uses assert_has_calls against mock_conn.execute instead of iterating over
mock_conn.execute.call_args_list.
In `@backend/tests/test_db/test_images.py`:
- Around line 74-83: The test contains PEP8 spacing issues around comparison
operators; update the assertions and equality checks in this block (references:
variable "image", assertion using "assert
image[0]==\"ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494\"", the pytest.raises block
calling db_remove_image_from_album, and the final check "assert image ==[]") to
include spaces around operators (e.g., change "image[0]==..." to "image[0] ==
...", "assert image ==[]" to "assert image == []") so the code conforms to PEP8.
In `@backend/tests/test_routes/test_albums.py`:
- Around line 1-11: In tests/test_albums.py remove the redundant sys.path
manipulation: delete the imports of sys and os and the sys.path.append(...)
statement (they're unnecessary because conftest.py already sets up PYTHONPATH),
leaving the rest of the imports and the reference to albums_router intact;
ensure there are no unused-import warnings after removing sys/os.
🪄 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: 567555c7-7bcb-449a-a5bd-aa5eb5ed9473
📒 Files selected for processing (8)
backend/__init__.pybackend/tests/conftest.pybackend/tests/test_connect.pybackend/tests/test_db/__init__.pybackend/tests/test_db/test_albums.pybackend/tests/test_db/test_connect.pybackend/tests/test_db/test_images.pybackend/tests/test_routes/test_albums.py
Comment on lines
+1
to
+67
| import pytest | ||
| import sqlite3 | ||
| from unittest.mock import patch, MagicMock | ||
| from app.database.connection import get_db_connection | ||
|
|
||
|
|
||
| class TestDatabaseConnection: | ||
| """ | ||
| Test suite for the SQLite database connection context manager. | ||
| """ | ||
|
|
||
| def test_get_db_connection_returns_connection(self): | ||
| """ | ||
| Ensure the context manager yields a valid sqlite3 connection. | ||
| """ | ||
| with get_db_connection() as conn: | ||
| assert isinstance(conn, sqlite3.Connection) | ||
|
|
||
| def test_commit_on_success(self): | ||
| """ | ||
| Ensure commit is called when no exception occurs. | ||
| """ | ||
| mock_conn = MagicMock() | ||
|
|
||
| with patch("sqlite3.connect", return_value=mock_conn): | ||
| with get_db_connection() as conn: | ||
| assert conn == mock_conn | ||
|
|
||
| mock_conn.commit.assert_called_once() | ||
| mock_conn.close.assert_called_once() | ||
|
|
||
| def test_rollback_on_exception(self): | ||
| """ | ||
| Ensure rollback is called when an exception occurs. | ||
| """ | ||
| mock_conn = MagicMock() | ||
|
|
||
| with patch("sqlite3.connect", return_value=mock_conn): | ||
| with pytest.raises(RuntimeError): | ||
| with get_db_connection(): | ||
| raise RuntimeError("Force rollback") | ||
|
|
||
| mock_conn.rollback.assert_called_once() | ||
| mock_conn.close.assert_called_once() | ||
|
|
||
| def test_pragmas_are_executed(self): | ||
| """ | ||
| Ensure PRAGMA statements are executed when connection is created. | ||
| """ | ||
| mock_conn = MagicMock() | ||
|
|
||
| with patch("sqlite3.connect", return_value=mock_conn): | ||
| with get_db_connection(): | ||
| pass | ||
|
|
||
| expected_calls = [ | ||
| (("PRAGMA foreign_keys = ON;",),), | ||
| (("PRAGMA ignore_check_constraints = OFF;",),), | ||
| (("PRAGMA recursive_triggers = ON;",),), | ||
| (("PRAGMA defer_foreign_keys = OFF;",),), | ||
| (("PRAGMA case_sensitive_like = ON;",),), | ||
| ] | ||
|
|
||
| executed = [call.args for call in mock_conn.execute.call_args_list] | ||
|
|
||
| for pragma_call in expected_calls: | ||
| assert pragma_call[0] in executed No newline at end of file |
Contributor
There was a problem hiding this comment.
Duplicate test file detected - remove one copy.
This file is nearly identical to backend/tests/test_db/test_connect.py. Both define TestDatabaseConnection with the same four test methods. Pytest will discover and run both, causing redundant test execution and potential confusion.
Remove this file and keep only backend/tests/test_db/test_connect.py to maintain a single source of truth for connection tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_connect.py` around lines 1 - 67, Remove the duplicate test
file that defines the TestDatabaseConnection class and exercises
get_db_connection (the redundant copy that contains the four tests:
test_get_db_connection_returns_connection, test_commit_on_success,
test_rollback_on_exception, test_pragmas_are_executed); keep only the canonical
test file (the other copy) so pytest discovers a single source of truth for
these connection tests and delete this redundant file from the repository.
Comment on lines
+25
to
+30
| with patch("sqlite3.connect", return_value=mock_conn): | ||
| with get_db_connection() as conn: | ||
| assert conn == mock_conn | ||
|
|
||
| mock_conn.commit.assert_called_once() | ||
| mock_conn.close.assert_called_once() |
Contributor
There was a problem hiding this comment.
Incorrect mock patch target may cause unreliable tests.
Patching "sqlite3.connect" patches the module globally. The recommended practice is to patch where the function is used, not where it's defined:
- with patch("sqlite3.connect", return_value=mock_conn):
+ with patch("app.database.connection.sqlite3.connect", return_value=mock_conn):This ensures the mock is applied specifically to the get_db_connection function's usage of sqlite3.connect.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with patch("sqlite3.connect", return_value=mock_conn): | |
| with get_db_connection() as conn: | |
| assert conn == mock_conn | |
| mock_conn.commit.assert_called_once() | |
| mock_conn.close.assert_called_once() | |
| with patch("app.database.connection.sqlite3.connect", return_value=mock_conn): | |
| with get_db_connection() as conn: | |
| assert conn == mock_conn | |
| mock_conn.commit.assert_called_once() | |
| mock_conn.close.assert_called_once() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_connect.py` around lines 25 - 30, The test patches the
global "sqlite3.connect" which can miss the actual call site; change the patch
target to the module where get_db_connection is defined so you're mocking the
usage site (e.g.
patch("<module_where_get_db_connection_is_defined>.sqlite3.connect",
return_value=mock_conn)), keep using get_db_connection() in the with block and
assert conn == mock_conn, then verify mock_conn.commit.assert_called_once() and
mock_conn.close.assert_called_once(); update the import/target string to the
actual module name that defines get_db_connection.
Comment on lines
+1
to
+11
| import os | ||
| import sqlite3 | ||
| import sys | ||
| import uuid | ||
|
|
||
| import pytest | ||
|
|
||
| from backend.app.config.settings import DATABASE_PATH | ||
| from backend.app.database.albums import verify_album_password, db_insert_album, db_delete_album, db_get_album, \ | ||
| db_update_album, db_get_all_albums, db_get_album_by_name, db_get_album_images, db_add_images_to_album | ||
|
|
Contributor
There was a problem hiding this comment.
Remove unused imports and fix import path inconsistency.
osandsys(lines 1, 3) are imported but never used.db_get_album_imagesanddb_add_images_to_album(line 10) are imported but never used.- Import path uses
backend.app...instead ofapp...(inconsistent with conftest.py setup).
-import os
import sqlite3
-import sys
import uuid
import pytest
-from backend.app.config.settings import DATABASE_PATH
-from backend.app.database.albums import verify_album_password, db_insert_album, db_delete_album, db_get_album, \
- db_update_album, db_get_all_albums, db_get_album_by_name, db_get_album_images, db_add_images_to_album
+from app.config.settings import DATABASE_PATH
+from app.database.albums import (
+ verify_album_password,
+ db_insert_album,
+ db_delete_album,
+ db_get_album,
+ db_update_album,
+ db_get_all_albums,
+ db_get_album_by_name,
+)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_db/test_albums.py` around lines 1 - 11, Remove the unused
imports and fix the module path: delete the unused imports os and sys and remove
db_get_album_images and db_add_images_to_album from the import list, and change
the long import prefix from backend.app... to app... so the file imports
DATABASE_PATH and the used DB functions (verify_album_password, db_insert_album,
db_delete_album, db_get_album, db_update_album, db_get_all_albums,
db_get_album_by_name) from app.config.settings and app.database.albums
respectively; keep the remaining import names unchanged so tests still reference
verify_album_password, db_insert_album, db_delete_album, db_get_album,
db_update_album, db_get_all_albums, and db_get_album_by_name.
Comment on lines
+25
to
+30
| with patch("sqlite3.connect", return_value=mock_conn): | ||
| with get_db_connection() as conn: | ||
| assert conn == mock_conn | ||
|
|
||
| mock_conn.commit.assert_called_once() | ||
| mock_conn.close.assert_called_once() |
Contributor
There was a problem hiding this comment.
Patch target should reference the module where sqlite3.connect is used.
Same issue as noted in the duplicate file. Patch where the function is used for reliable mocking:
- with patch("sqlite3.connect", return_value=mock_conn):
+ with patch("app.database.connection.sqlite3.connect", return_value=mock_conn):Apply this change to all three test methods that use mocking (lines 25, 38, 52).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with patch("sqlite3.connect", return_value=mock_conn): | |
| with get_db_connection() as conn: | |
| assert conn == mock_conn | |
| mock_conn.commit.assert_called_once() | |
| mock_conn.close.assert_called_once() | |
| with patch("app.database.connection.sqlite3.connect", return_value=mock_conn): | |
| with get_db_connection() as conn: | |
| assert conn == mock_conn | |
| mock_conn.commit.assert_called_once() | |
| mock_conn.close.assert_called_once() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_db/test_connect.py` around lines 25 - 30, The tests
currently patch "sqlite3.connect" directly but should patch it in the module
under test; update the patch target to point to the module where
get_db_connection is defined (patch the connect symbol used by that module) so
the mock intercepts calls inside get_db_connection; apply this change for all
three test methods that mock sqlite3.connect (the test blocks around
get_db_connection) so mock_conn is used and assertions on mock_conn.commit and
mock_conn.close remain valid.
Comment on lines
+6
to
+9
| from backend.app.config.settings import DATABASE_PATH | ||
| from backend.app.database.albums import db_insert_album, db_get_album_images, db_add_images_to_album, \ | ||
| db_remove_image_from_album, db_remove_images_from_album | ||
| from backend.app.database.images import db_bulk_insert_images, ImageRecord, db_get_all_images |
Contributor
There was a problem hiding this comment.
Inconsistent import paths and unused imports.
-
Import paths use
backend.app...while other test files usefrom app.... This inconsistency may cause import errors depending on how pytest is invoked. -
ImageRecordanddb_get_all_imagesare imported but never used.
-from backend.app.config.settings import DATABASE_PATH
-from backend.app.database.albums import db_insert_album, db_get_album_images, db_add_images_to_album, \
+from app.config.settings import DATABASE_PATH
+from app.database.albums import db_insert_album, db_get_album_images, db_add_images_to_album, \
db_remove_image_from_album, db_remove_images_from_album
-from backend.app.database.images import db_bulk_insert_images, ImageRecord, db_get_all_images
+from app.database.images import db_bulk_insert_images🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_db/test_images.py` around lines 6 - 9, Update the import
statements in this test to match the project convention used by other tests (use
"from app..." rather than "from backend.app...") and remove unused imports;
specifically change the module imports for DATABASE_PATH, db_insert_album,
db_get_album_images, db_add_images_to_album, db_remove_image_from_album,
db_remove_images_from_album to use the "app" package path and delete the unused
ImageRecord and db_get_all_images imports from backend.app.database.images so
only the actually used symbols remain.
Comment on lines
+85
to
+107
| img1 = { | ||
| "id": "ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494", | ||
| "path": "C:\\Users\\user123\\Pictures\\Acer\\Acer_Wallpaper_01_5000x2814.jpg", | ||
| "folder_id": "e6d83d84-ad5f-4901-9519-2a1be50977d7", | ||
| "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_ced4be84.jpg", | ||
| "metadata": '{"name": "Acer_Wallpaper_01_5000x2814.jpg"}', | ||
| "isTagged": 0, | ||
| "latitude": None, | ||
| "longitude": None, | ||
| "captured_at": "2020-10-21T08:36:00", | ||
| } | ||
| img2 = { | ||
| "id": "f423c320-080e-4d93-832f-2c2ccfd249ec", | ||
| "path": "C:\\Users\\user123\\Pictures\\Acer\\Acer_Wallpaper_02_5000x2813.jpg", | ||
| "folder_id": "e6d83d84-ad5f-4901-9519-2a1be50977d7", | ||
| "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_ced4be84.jpg", | ||
| "metadata": '{"name": "Acer_Wallpaper_02_5000x2813.jpg"}', | ||
| "isTagged": 0, | ||
| "latitude": None, | ||
| "longitude": None, | ||
| "captured_at": "2020-10-21T08:36:00", | ||
| } | ||
| db_bulk_insert_images([img1,img2]) |
Contributor
There was a problem hiding this comment.
Duplicate primary key and UNIQUE constraint violations will cause test failure.
Several issues in the second part of this test:
- Line 86:
img1["id"]is identical toimg["id"](line 58). Re-inserting the same primary key will fail. - Line 100:
img2["thumbnailPath"]duplicatesimg1["thumbnailPath"]but the schema requiresthumbnailPathto be UNIQUE. - Line 107:
db_bulk_insert_images([img1, img2])attempts to insertimg1which was already inserted at line 68.
Use distinct IDs and paths for img1 and img2:
🐛 Proposed fix
img1 = {
- "id": "ced4be84-4e16-4c1b-a9c1-b7f2e2ee8494",
- "path": "C:\\Users\\user123\\Pictures\\Acer\\Acer_Wallpaper_01_5000x2814.jpg",
+ "id": "ced4be84-4e16-4c1b-a9c1-b7f2e2ee8495",
+ "path": "C:\\Users\\user123\\Pictures\\Acer\\Acer_Wallpaper_03_5000x2814.jpg",
"folder_id": "e6d83d84-ad5f-4901-9519-2a1be50977d7",
- "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_ced4be84.jpg",
- "metadata": '{"name": "Acer_Wallpaper_01_5000x2814.jpg"}',
+ "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_ced4be85.jpg",
+ "metadata": '{"name": "Acer_Wallpaper_03_5000x2814.jpg"}',
...
}
img2 = {
"id": "f423c320-080e-4d93-832f-2c2ccfd249ec",
"path": "C:\\Users\\user123\\Pictures\\Acer\\Acer_Wallpaper_02_5000x2813.jpg",
"folder_id": "e6d83d84-ad5f-4901-9519-2a1be50977d7",
- "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_ced4be84.jpg",
+ "thumbnailPath": "C:\\Users\\user123\\AppData\\Local\\PictoPy\\PictoPy\\thumbnails\\thumbnail_f423c320.jpg",
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_db/test_images.py` around lines 85 - 107, The test is
failing due to duplicate primary key and UNIQUE constraint collisions: create
new unique values for img1["id"] and img2["id"] (different from the earlier img
and any prior test fixtures) and ensure img2["thumbnailPath"] is unique (not
equal to img1["thumbnailPath"] or any previously inserted thumbnailPath); update
the "path" and "metadata" fields if they must also be unique in the DB, and then
call db_bulk_insert_images with the two fresh, non-duplicated dicts (symbols to
edit: img1, img2, their "id" and "thumbnailPath" fields, and the call to
db_bulk_insert_images).
|
Please mention the issue you are addressing! |
NancyWei123
changed the title
Contributor
|
|
Author
|
|
Hey thanks for contributing but currently we are closing all prs that are linked to unreviewed issues please wait for mentors to discuss the issue mention your issue with some idea in the discord channel and wait for them to assign it to you then go ahead with the pr! |
Author
|
Sure! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #(TODO:issue number)
Improve the test coverage to 57%.
Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
ChatGPT
Checklist
Summary by CodeRabbit