Skip to content

Conversation

@PranjaliBhardwaj
Copy link

@PranjaliBhardwaj PranjaliBhardwaj commented Jan 26, 2026

Issue: #1083

How I added tests for the Images API

  1. Followed existing test style

I used test_albums.py as a reference so everything stays consistent:
Same setup: imports → FastAPI app → include_router("/images") → TestClient, Same mocking style, Same structure: fixtures at the top, then test classes, Same pattern for requests (client.get / client.post) and assertions on status + response

  1. Added fixtures + mock data

_make_metadata() – creates minimal metadata needed by the API, mock_db_image – one sample image returned by db_get_all_images, mock_db_image_untagged – same image but with isTagged=False, These mocks make sure responses match the API schema without using a real DB.

  1. What the tests cover

GET /images/:
Success case (returns one image)
Empty list case
Tagged filter (tagged=True / False)
Internal error (returns 500 with proper message)
POST /images/toggle-favourite
Success case (favourite toggled)
Image not found (404) and Internal error (500) and Missing image_id (422 validation error)

Each test also checks that the correct DB/helper functions were called.

  1. Issue found + fix

The session DB setup in conftest.py was failing with a disk I/O error, so image tests were not running.
Since these tests are fully mocked and don’t need a real DB:
I added a flag: PICTOPY_SKIP_DB_SETUP=1
When this is set, DB creation is skipped and only TEST_MODE is enabled
This allows image tests to run cleanly in this environment.

output:
All 9 new tests are passing. The route fix ensures “image not found” correctly returns 404 instead of 500.

Summary by CodeRabbit

  • Bug Fixes

    • Improved error response handling for the toggle-favourite feature to ensure appropriate HTTP status codes are returned for different error types.
  • Tests

    • Expanded test coverage for the Images API to validate core operations including image retrieval, filtering, favourite toggling, and error handling scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Pranjali Bhardwaj <pranjalisharma6543@gmail.com>
@github-actions
Copy link
Contributor

⚠️ No issue was linked in the PR description.
Please make sure to link an issue (e.g., 'Fixes #issue_number')

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 26, 2026

📝 Walkthrough

Walkthrough

The pull request enhances exception handling in the Images API toggle-favourite route to explicitly re-raise HTTP exceptions while preserving generic error handling, adds conditional database setup skipping in test configuration, and introduces comprehensive test coverage for the Images API routes with 222 new test lines.

Changes

Cohort / File(s) Summary
Route Exception Handling
backend/app/routes/images.py
Added explicit except HTTPException: raise clause to the /toggle-favourite route to differentiate and re-raise HTTP exceptions with preserved status codes, while maintaining existing generic exception handling for logging and 500 error responses.
Test Infrastructure & Coverage
backend/tests/conftest.py, backend/tests/test_images.py
Extended test configuration with conditional PICTOPY_SKIP_DB_SETUP environment variable support to bypass database initialization for unit-only tests. Introduced new test module with comprehensive coverage for Images API routes: GET /images/ validation (retrieval, metadata fields, filtering, error handling) and POST /images/toggle-favourite validation (success path, 404/422/500 error scenarios).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

🐰 A hop, a skip, exceptions caught with care,
Test tables skip when signals flare,
Images toggle, favorites shine,
222 lines of tests divine!
HTTP flows, and mocks so fair,

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'added image api tests' directly reflects the main change: nine new test cases for the Images API (test_images.py module with 222 lines added). This accurately captures the primary purpose of the PR without unnecessary detail.
Docstring Coverage ✅ Passed Docstring coverage is 84.62% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
backend/app/routes/images.py (1)

108-114: Potential AttributeError if image is not found after toggle.

If db_toggle_image_favourite_status returns True but the image is subsequently not found in db_get_all_images() (e.g., race condition or data inconsistency), image will be None and line 114 will raise AttributeError: 'NoneType' object has no attribute 'get'.

Proposed fix to handle the None case
         image = next(
             (img for img in db_get_all_images() if img["id"] == image_id), None
         )
+        if image is None:
+            raise HTTPException(
+                status_code=404, detail="Image not found after toggle"
+            )
         return {
             "success": True,
             "image_id": image_id,
-            "isFavourite": image.get("isFavourite", False),
+            "isFavourite": image.get("isFavourite", False),
         }
🧹 Nitpick comments (1)
backend/tests/test_images.py (1)

62-74: Unused fixture mock_db_image_untagged.

This fixture is defined but not used in any of the current tests. Consider removing it to avoid dead code, or add tests that exercise the untagged image scenario (e.g., verifying that tags=None is handled correctly in the response).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant