Test/images api tests - add test suite for Images API endpoints #854
Conversation
Created test_images.py with 15+ test cases covering: - GET /images/ endpoint (success, empty, filtered, errors) - POST /images/toggle-favourite (success, not found, validation) - Edge cases (null metadata, GPS location data) - Integration workflow tests Follows existing test patterns from test_folders.py
Adds comprehensive docstring documenting all parameters and return value. Clarifies that confidence_threshold controls class name labeling, not detection visibility - detections below the threshold are labeled 'unknown' rather than being omitted.
📝 WalkthroughWalkthroughTwo changes add documentation and testing: a docstring for a utility function clarifying its purpose and parameters, and a comprehensive test suite for the Images API with fixtures, mock database interactions, and test cases covering success, edge cases, and error scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
|
3 similar comments
|
|
|
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/tests/test_images.py (2)
319-349: Remove redundant mock setup.Line 345 redundantly sets
mock_get_all.return_valueto the same value already configured on line 334. Since the mock persists throughout the test, this line can be removed.🔎 Proposed cleanup
# Verify by getting all images - mock_get_all.return_value = [updated_image] get_response = client.get("/images/")
1-349: Consider additional test coverage for completeness.While the current test suite is comprehensive, consider adding tests for:
- Invalid
image_idformats (empty string, null, special characters)- Multiple rapid toggle requests (idempotency verification)
- Query parameter edge cases (e.g.,
tagged=invalid_value)These are optional enhancements that could be deferred to future work.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/utils/YOLO.pybackend/tests/test_images.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_images.py (1)
backend/tests/test_folders.py (3)
test_db(20-33)app_with_state(64-72)client(76-78)
🔇 Additional comments (5)
backend/app/utils/YOLO.py (1)
164-177: LGTM! Clear and comprehensive docstring.The docstring clearly documents the function's purpose, all parameters, and return value. The explanation of
confidence_thresholdbehavior (labeling detections below threshold as "unknown") is particularly helpful.backend/tests/test_images.py (4)
116-189: Excellent coverage of GET /images/ endpoint.The test cases comprehensively cover:
- Success scenarios with data validation
- Empty result handling
- Query parameter filtering (tagged/untagged)
- Database error handling with proper status codes
The assertions verify both response structure and correct mock invocations.
195-251: Well-structured tests for POST /images/toggle-favourite endpoint.The test suite properly covers:
- Successful favourite toggle with state verification
- 404 handling for non-existent images
- Request validation (422 for missing required fields)
- Database error handling (500 errors)
All tests use appropriate mocks and verify expected behavior.
257-313: Good coverage of edge cases.The edge case tests appropriately validate:
- Handling of images with empty metadata (ensuring no crashes)
- Preservation of GPS location metadata in responses
These tests help ensure API robustness with various data conditions.
41-46: No action needed. Theapp_with_statefixture correctly omits executor mocking because the images router does not accessapp.state.executor. Executor mocking is specific to the folders router's heavyweight operations, not required here.
| @pytest.fixture(scope="function") | ||
| def test_db(): | ||
| """Create a temporary test database for each test.""" | ||
| db_fd, db_path = tempfile.mkstemp() | ||
|
|
||
| import app.config.settings | ||
|
|
||
| original_db_path = app.config.settings.DATABASE_PATH | ||
| app.config.settings.DATABASE_PATH = db_path | ||
|
|
||
| yield db_path | ||
|
|
||
| app.config.settings.DATABASE_PATH = original_db_path | ||
| os.close(db_fd) | ||
| os.unlink(db_path) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor to eliminate fixture duplication.
The test_db fixture is identical to the one in backend/tests/test_folders.py (lines 19-32). Consider extracting shared fixtures to a conftest.py file to follow DRY principles and ensure consistency across test modules.
🔎 Proposed refactor
Create a new file backend/tests/conftest.py:
import pytest
import tempfile
import os
@pytest.fixture(scope="function")
def test_db():
"""Create a temporary test database for each test."""
db_fd, db_path = tempfile.mkstemp()
import app.config.settings
original_db_path = app.config.settings.DATABASE_PATH
app.config.settings.DATABASE_PATH = db_path
yield db_path
app.config.settings.DATABASE_PATH = original_db_path
os.close(db_fd)
os.unlink(db_path)Then remove the test_db fixture from both test_images.py and test_folders.py.
🤖 Prompt for AI Agents
In backend/tests/test_images.py around lines 24 to 38 the test_db fixture
duplicates one in backend/tests/test_folders.py; extract this shared fixture
into backend/tests/conftest.py with the same implementation and remove the
duplicate test_db fixture from both test_images.py and test_folders.py so pytest
will automatically discover the shared fixture; ensure the conftest.py imports
pytest, tempfile and os and restores app.config.settings.DATABASE_PATH and
cleans up the temp file after yield exactly as in the original fixture.
|
Warning: This author is forking multiple ML projects such as google-deepmind/alphafold, ml-explore/mlx, openai/CLIP, pytorch/pytorch, tensorflow/tensorflow, anthropics/claude-code, vllm-project/vllm, and others, adding minimal "contributions" (often for tests or miscellaneous changes) without proper validation. A review of their commits shows mostly local implementations of TODOs copied from existing projects, with little to no substantive review or testing. So far, this author has forked 41 repositories following the same pattern. Be careful when accepting this PR. It’s also concerning how this author is able to submit PRs across four repositories in the same day, each requiring large context, which strongly suggests a highly automated workflow. |
|
Thank you for the feedback regarding my recent activity. I am a student and I have been using these smaller tasks as a way to familiarize myself with the architecture of various codebases. I was not aware that submitting multiple minor pull requests was considered disruptive to the maintainer workflow or seen as contribution padding. I appreciate the correction and will pay much closer attention to the impact of my work moving forward. I am closing this pull request now to focus on delivering more substantive technical contributions that provide genuine value to the community. |
2 participants
Summary
Fixes #853
Creates a comprehensive test suite for the Images API endpoints, filling a gap in the backend test coverage.
Changes Made
New File:
backend/tests/test_images.pyTest Coverage
Test Structure