Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes comprehensive unit and integration test coverage for the backend FastAPI application, adding test suites for all major API endpoints including notes, library, papers, authentication, conversations, academic search, and health checks.
Key Changes:
- Added 9 new test files with extensive integration tests covering CRUD operations, error handling, and edge cases
- Created test infrastructure with conftest.py fixtures for database and client setup
- Added comprehensive TESTING.md documentation explaining test execution and troubleshooting
- Made minor code adjustments to support test execution (directory creation, .gitignore updates)
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_notes.py | Integration tests for notes CRUD operations and validation |
| tests/test_library.py | Tests for library folder management and file upload/download |
| backend/tests/test_papers.py | Tests for paper upload, listing, and import functionality |
| backend/tests/test_notes.py | Duplicate test suite in backend directory for notes endpoints |
| backend/tests/test_library.py | Duplicate test suite in backend directory for library endpoints |
| backend/tests/test_health.py | Simple health check endpoint validation |
| backend/tests/test_conversations.py | Tests for conversation creation, updates, and access control |
| backend/tests/test_auth.py | Authentication flow tests including GitHub OAuth |
| backend/tests/test_academic.py | Academic search service and provider tests |
| backend/tests/test_mineru_comprehensive.py | MinerU PDF parser integration tests with fixture improvements |
| tests/conftest.py | Test fixtures for async client and database setup |
| tests/TESTING.md | Comprehensive testing guide with setup and troubleshooting instructions |
| backend/app/api/v1/endpoints/papers.py | Added directory creation to support test environment |
| backend/app/api/v1/init.py | Trailing whitespace addition |
| redis.conf | Added blank line for formatting |
| .gitignore | Added htmlcov directory for coverage reports |
| backend/.coverage | Binary coverage database file (should not be committed) |
Comments suppressed due to low confidence (1)
tests/conftest.py:16
- Import of 'user' is not used.
from app.models import user # noqa: F401
| async def _register_user(async_client: AsyncClient, email: str, password: str) -> None: | ||
| payload: dict[str, str] = {"email": email, "password": password} | ||
| response = await async_client.post("/api/v1/users", json=payload) | ||
| assert response.status_code == 201 | ||
|
|
||
|
|
||
| async def _login(async_client: AsyncClient, email: str, password: str) -> str: | ||
| response = await async_client.post( | ||
| "/api/v1/auth/token", | ||
| data={"username": email, "password": password}, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| ) | ||
| assert response.status_code == 200 | ||
| return response.json()["access_token"] | ||
|
|
||
|
|
||
| def _auth_headers(token: str) -> dict[str, str]: | ||
| return {"Authorization": f"Bearer {token}"} |
There was a problem hiding this comment.
The test helper functions _register_user, _login, and _auth_headers are duplicated across multiple test files. This creates maintenance overhead and violates the DRY (Don't Repeat Yourself) principle. Consider extracting these common helpers into a shared module (e.g., backend/tests/helpers.py or adding them to conftest.py) so they can be reused across all test files.
| async def _register_user(async_client: AsyncClient, email: str, password: str) -> None: | ||
| payload: dict[str, str] = {"email": email, "password": password} | ||
| response = await async_client.post("/api/v1/users", json=payload) | ||
| assert response.status_code == 201 | ||
|
|
||
|
|
||
| async def _login(async_client: AsyncClient, email: str, password: str) -> str: | ||
| response = await async_client.post( | ||
| "/api/v1/auth/token", | ||
| data={"username": email, "password": password}, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| ) | ||
| assert response.status_code == 200 | ||
| return response.json()["access_token"] | ||
|
|
||
|
|
||
| def _auth_headers(token: str) -> dict[str, str]: | ||
| return {"Authorization": f"Bearer {token}"} | ||
|
|
||
|
|
||
| async def _upload_pdf(async_client: AsyncClient, token: str, name: str = "sample.pdf") -> int: | ||
| pdf_bytes = b"%PDF-1.4\ncontent" | ||
| resp = await async_client.post( | ||
| "/api/v1/papers/upload", | ||
| headers=_auth_headers(token), | ||
| files={"file": (name, pdf_bytes, "application/pdf")}, | ||
| ) | ||
| assert resp.status_code == 201 | ||
| return resp.json()["id"] |
There was a problem hiding this comment.
The test helper functions _register_user, _login, and _auth_headers are duplicated across multiple test files. This creates maintenance overhead and violates the DRY (Don't Repeat Yourself) principle. Consider extracting these common helpers into a shared module (e.g., backend/tests/helpers.py or adding them to conftest.py) so they can be reused across all test files.
| TEST_MEDIA_DIR = Path(__file__).resolve().parent / "tmp_media" | ||
| TEST_FILES: List[Path] = [ | ||
| TEST_MEDIA_DIR / "sample_table.pdf", | ||
| TEST_MEDIA_DIR / "complex_test.pdf", | ||
| ] |
There was a problem hiding this comment.
The TEST_MEDIA_DIR path is defined using a relative path from the test file location, but this pattern is fragile if the test file is moved or if tests are run from different working directories. Consider using a more robust approach, such as defining this in conftest.py alongside other test configuration, or using the MEDIA_ROOT environment variable that's already set up in the test configuration. This would ensure consistency across all test files.
| # Ensure the upload directory exists even if MEDIA_ROOT is cleaned between tests. | ||
| UPLOAD_DIR.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
While creating the upload directory is necessary for tests, this change adds runtime overhead to every upload operation. The directory should already exist in production environments, so this check is only needed for test scenarios. Consider moving this directory creation to the test setup (conftest.py) or application startup code rather than executing it on every upload request. This would improve performance and separate test-specific concerns from production code.
| @@ -1,4 +1,5 @@ | |||
| # Redis configuration for InsightReading | |||
|
|
|||
There was a problem hiding this comment.
An empty line was added after the comment header. While this improves readability, it's a very minor cosmetic change. If this blank line addition is intentional for formatting consistency, that's acceptable. However, if this change was accidental or unrelated to the test additions, it should be removed to keep the PR focused on test-related changes only.
|
|
||
| api_router.include_router(library.router, prefix="/library", tags=["library"]) | ||
|
|
||
|
No newline at end of file |
There was a problem hiding this comment.
Trailing whitespace should be removed from this line to maintain code cleanliness and adhere to standard formatting practices.
| async def search(self, request: IntelligentSearchRequest) -> IntelligentSearchResponse: # pragma: no cover - behavior via HTTP | ||
| raise ValueError("Invalid intelligent search request") |
There was a problem hiding this comment.
The pragma comment "# pragma: no cover - behavior via HTTP" is used to exclude a method from coverage analysis, but this reasoning seems inconsistent. If the method is called during tests (even indirectly via HTTP), it should be covered. If it's intentionally not tested, consider whether this represents a gap in test coverage that should be addressed. The comment suggests the behavior is tested via HTTP, which would mean it should be covered.
| async def _register_user(async_client: AsyncClient, email: str, password: str) -> None: | ||
| payload: dict[str, str] = {"email": email, "password": password} | ||
| response = await async_client.post("/api/v1/users", json=payload) | ||
| assert response.status_code == 201 | ||
|
|
||
|
|
||
| async def _login(async_client: AsyncClient, email: str, password: str) -> str: | ||
| response = await async_client.post( | ||
| "/api/v1/auth/token", | ||
| data={"username": email, "password": password}, | ||
| headers={"Content-Type": "application/x-www-form-urlencoded"}, | ||
| ) | ||
| assert response.status_code == 200 | ||
| return response.json()["access_token"] | ||
|
|
||
|
|
||
| def _auth_headers(token: str) -> dict[str, str]: | ||
| return {"Authorization": f"Bearer {token}"} | ||
|
|
||
|
|
||
| async def _upload_pdf(async_client: AsyncClient, token: str, name: str = "sample.pdf") -> int: | ||
| pdf_bytes = b"%PDF-1.4\ncontent" | ||
| resp = await async_client.post( | ||
| "/api/v1/papers/upload", | ||
| headers=_auth_headers(token), | ||
| files={"file": (name, pdf_bytes, "application/pdf")}, | ||
| ) | ||
| assert resp.status_code == 201 | ||
| return resp.json()["id"] |
There was a problem hiding this comment.
The test helper functions _register_user, _login, _auth_headers, and _upload_pdf are duplicated across multiple test files. This creates maintenance overhead and violates the DRY (Don't Repeat Yourself) principle. Consider extracting these common helpers into a shared module (e.g., tests/helpers.py or adding them to conftest.py) so they can be reused across all test files.
| async def test_academic_search_uses_service(monkeypatch: pytest.MonkeyPatch, async_client: AsyncClient) -> None: | ||
| from app.api.v1.endpoints import academic as academic_endpoint | ||
|
|
||
| monkeypatch.setattr(academic_endpoint, "AcademicSearchService", lambda: DummyAcademicSearchService()) |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
| ) -> None: | ||
| from app.api.v1.endpoints import academic as academic_endpoint | ||
|
|
||
| monkeypatch.setattr(academic_endpoint, "IntelligentSearchService", lambda: DummyIntelligentSearchService()) |
There was a problem hiding this comment.
This 'lambda' is just a simple wrapper around a callable object. Use that object directly.
finish all unit test