Kaapi v1.0: Enhancing the test suite #488
Conversation
📝 WalkthroughWalkthroughMain app seeding was simplified to only create organizations, projects, users, and API keys; credentials, assistants, and documents were removed from the app seeder. A separate comprehensive test seeder and test seed JSON were added. Many tests received typing, import, and minor-coverage adjustments. Changes
Sequence Diagram(s)(Skipped — changes are seeding and test-suite updates and do not introduce a new multi-component runtime control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nishika26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/app/tests/seed_data/seed_data.py (2)
86-86: Remove unnecessary mode argument.The
"r"mode is the default foropen()and can be omitted.🔎 Proposed fix
- with open(json_path, "r") as f: + with open(json_path) as f:
24-80: Consider extracting shared Pydantic models to reduce duplication.The data models (
OrgData,ProjectData,UserData,APIKeyData) and several helper functions are nearly identical between this file andbackend/app/seed_data/seed_data.py. Consider extracting common models to a shared module to improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/seed_data/seed_data.json(1 hunks)backend/app/seed_data/seed_data.py(2 hunks)backend/app/tests/conftest.py(1 hunks)backend/app/tests/seed_data/seed_data.json(1 hunks)backend/app/tests/seed_data/seed_data.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/conftest.pybackend/app/seed_data/seed_data.pybackend/app/tests/seed_data/seed_data.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/conftest.pybackend/app/tests/seed_data/seed_data.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.458Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.457Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.457Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Applied to files:
backend/app/tests/conftest.py
📚 Learning: 2025-12-17T15:39:30.458Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.458Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
Applied to files:
backend/app/tests/conftest.pybackend/app/seed_data/seed_data.pybackend/app/tests/seed_data/seed_data.py
🧬 Code graph analysis (3)
backend/app/tests/conftest.py (2)
backend/app/seed_data/seed_data.py (1)
seed_database(200-268)backend/app/tests/seed_data/seed_data.py (1)
seed_database(376-471)
backend/app/seed_data/seed_data.py (1)
backend/app/core/security.py (1)
get_password_hash(100-110)
backend/app/tests/seed_data/seed_data.py (9)
backend/app/tests/conftest.py (1)
db(28-45)backend/app/core/security.py (2)
get_password_hash(100-110)encrypt_credentials(151-168)backend/app/models/api_key.py (1)
APIKey(48-89)backend/app/models/organization.py (1)
Organization(44-82)backend/app/models/project.py (1)
Project(51-107)backend/app/models/user.py (1)
User(65-75)backend/app/models/credentials.py (1)
Credential(67-131)backend/app/models/assistants.py (1)
Assistant(34-116)backend/app/models/document.py (1)
Document(28-69)
🪛 Ruff (0.14.8)
backend/app/tests/seed_data/seed_data.py
86-86: Unnecessary mode argument
Remove mode argument
(UP015)
🔇 Additional comments (5)
backend/app/seed_data/seed_data.json (1)
1-56: LGTM!The JSON structure is valid and the simplification to core entities (organization, projects, users, apikeys) for production seeding is appropriate.
backend/app/tests/conftest.py (1)
24-24: LGTM!The import path correctly points to the new test-specific seed module, which includes credentials, assistants, and documents needed for comprehensive test coverage.
backend/app/tests/seed_data/seed_data.json (1)
1-129: LGTM!The test seed data file is well-structured and comprehensive, providing all entity types needed for thorough test coverage. The use of clearly fake credentials (e.g.,
sk-proj-GlificI3i5SCxN,sk-lf-test-*) is appropriate for test isolation.backend/app/seed_data/seed_data.py (1)
200-268: LGTM!The simplified seed function correctly focuses on core entities (organizations, users, projects, API keys) for production seeding. Exception handling with rollback is properly implemented.
backend/app/tests/seed_data/seed_data.py (1)
376-471: LGTM!The test seed function is comprehensive and correctly seeds all entity types needed for thorough test coverage. The exception handling with rollback is properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/tests/services/response/response/test_process_response.py (1)
56-62: Add return type hint to comply with coding guidelines.The
make_requesthelper function is missing a return type hint. As per coding guidelines, all function parameters and return values must have type hints.🔎 Proposed fix
-def make_request(assistant_id: str, previous_response_id: str | None = None): +def make_request(assistant_id: str, previous_response_id: str | None = None) -> ResponsesAPIRequest: return ResponsesAPIRequest( assistant_id=assistant_id, question="What is the capital of France?", callback_url="http://example.com/callback", response_id=previous_response_id, )Based on coding guidelines, all functions must include return type hints.
backend/app/tests/crud/config/test_config.py (1)
19-20: Add return type hint to fixture function.The
example_config_blobfixture is missing a return type hint. Per the coding guidelines, all function parameters and return values in Python code should have type hints.🔎 Proposed fix
@pytest.fixture -def example_config_blob(): +def example_config_blob() -> ConfigBlob: return ConfigBlob(As per coding guidelines, type hints are required for all functions in
**/*.pyfiles.backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (1)
13-27: Add return type hint to helper function.Per coding guidelines, all function parameters and return values must have type hints. This function returns a project ID (integer).
🔎 Proposed fix
-def create_collections(db: Session, n: int): +def create_collections(db: Session, n: int) -> int:As per coding guidelines, Python functions require type hints throughout the codebase.
♻️ Duplicate comments (1)
backend/app/tests/seed_data/seed_data.py (1)
346-357: Document deletion is already included.The previous review comment about missing
Documentdeletion is outdated - line 350 already includessession.exec(delete(Document)).
🧹 Nitpick comments (15)
backend/app/tests/api/routes/test_threads.py (1)
26-34: Consider adding type hints to test function parameters.Per the coding guidelines, all function parameters and return values should have type hints. While the import changes look good, the test functions throughout this file would benefit from type annotations for better IDE support and maintainability.
For example:
def test_threads_endpoint( mock_process_run: MagicMock, mock_send_callback: MagicMock, mock_get_provider_credential: MagicMock, mock_configure_openai: MagicMock, client: TestClient, db: Session, user_api_key_header: dict[str, str], ) -> None:This applies to most test functions in the file.
As per coding guidelines, type hints should be added to all function parameters and return values.
backend/app/tests/core/test_callback_ssrf.py (1)
13-317: Consider adding type hints to test methods.The coding guidelines require type hints for all function parameters and return values. Test methods should include return type hints (
-> None) and parameter type hints for mocked objects (e.g.,mock_getaddrinfo: MagicMock).Example for Line 118:
def test_reject_localhost_by_name(self, mock_getaddrinfo: MagicMock) -> None:As per coding guidelines, type hints are required for
**/*.pyfiles.backend/app/tests/crud/evaluations/test_dataset.py (4)
388-420: Add return type hint and consider factory pattern.The test logic is excellent and correctly validates successful deletion. However, per coding guidelines:
- Add
-> Nonereturn type hint to the test method- Consider using factory pattern for test fixtures instead of direct CRUD calls (though existing tests in this file don't follow this pattern either)
🔎 Proposed fix for return type hint
- def test_delete_dataset_success(self, db: Session): + def test_delete_dataset_success(self, db: Session) -> None: """Test successfully deleting a dataset."""As per coding guidelines, all functions should have type hints for parameters and return values.
422-437: Add return type hint.Test logic correctly validates the not-found scenario.
🔎 Proposed fix
- def test_delete_dataset_not_found(self, db: Session): + def test_delete_dataset_not_found(self, db: Session) -> None: """Test deleting a non-existent dataset."""As per coding guidelines, all functions should have type hints for return values.
439-470: Add return type hint.Test logic correctly validates organization-level access control for deletion.
🔎 Proposed fix
- def test_delete_dataset_wrong_org(self, db: Session): + def test_delete_dataset_wrong_org(self, db: Session) -> None: """Test deleting a dataset with wrong organization."""As per coding guidelines, all functions should have type hints for return values.
472-519: Add return type hint.Excellent test that validates referential integrity - datasets cannot be deleted when referenced by evaluation runs. The test correctly creates a dependent EvaluationRun and verifies deletion is blocked.
🔎 Proposed fix
- def test_delete_dataset_with_evaluation_runs(self, db: Session): + def test_delete_dataset_with_evaluation_runs(self, db: Session) -> None: """Test that dataset cannot be deleted if it has evaluation runs."""As per coding guidelines, all functions should have type hints for return values.
backend/app/tests/services/doctransformer/test_job/test_execute_job.py (1)
150-160: Consider using a more specific exception type.Catching the broad
Exceptionclass makes the test less precise. If you know the expected exception type(s), using them would make the test more robust and self-documenting.🔎 Suggested improvement
If the expected exception is known (e.g.,
ClientErrorfrom boto3 for missing S3 objects):- with pytest.raises(Exception): + with pytest.raises((ClientError, RetryError)):backend/app/tests/api/routes/test_assistants.py (1)
14-28: Add return type hints to fixtures.As per coding guidelines, all function parameters and return values should have type hints.
🔎 Suggested fix
@pytest.fixture -def assistant_create_payload(): +def assistant_create_payload() -> dict[str, str | float | int | list[str]]: return { "name": "Test Assistant", "instructions": "This is a test instruction.", "model": "gpt-4o", "vector_store_ids": ["vs_test_1", "vs_test_2"], "temperature": 0.5, "max_num_results": 10, } @pytest.fixture -def assistant_id(): +def assistant_id() -> str: return str(uuid4())backend/app/tests/crud/test_assistants.py (1)
465-472: Remove unusedprojectvariable.The
projectvariable is assigned but never used. The test only usesget_non_existent_id(db, Project).🔎 Suggested fix
def test_get_assistants_by_project_empty(self, db: Session): """Returns empty list when project has no assistants""" - project = get_project(db) non_existent_project_id = get_non_existent_id(db, Project) result = get_assistants_by_project(db, non_existent_project_id) assert result == []backend/pyproject.toml (1)
49-49: Approve pytest-asyncio addition. Consider consolidating pytest to dev-dependencies only.pytest-asyncio provides asyncio support for pytest, appropriately supporting the async test infrastructure in this PR. However, pytest appears in both main dependencies (line 27) and dev-dependencies (line 43). Testing frameworks like pytest need to be installed for development but aren't needed for the application to function in production—it should be moved to dev-dependencies only for proper dependency organization.
backend/app/tests/api/routes/test_evaluation.py (3)
699-709: Consider adding explicit return type hint.While pytest fixtures provide implicit typing, the coding guidelines require type hints on all function parameters and return values. Consider adding
-> EvaluationDatasetto the fixture:@pytest.fixture def create_test_dataset(self, db, user_api_key): """Create a test dataset for evaluation runs.""" - return create_test_evaluation_dataset( + return create_test_evaluation_dataset( # -> EvaluationDatasetOr more explicitly:
@pytest.fixture def create_test_dataset(self, db, user_api_key) -> EvaluationDataset:As per coding guidelines, all functions should have type hints for return values.
869-879: Consider adding explicit return type hint to fixture.Similar to the earlier fixture, consider adding
-> EvaluationDatasetreturn type annotation for consistency with coding guidelines.
928-938: Consider adding explicit return type hint to fixture.For consistency with coding guidelines, add
-> EvaluationDatasetreturn type annotation to this fixture as well.backend/app/tests/api/routes/test_cron.py (1)
9-184: Consider adding type hints to test functions.While pytest conventions typically omit type hints on test functions, the coding guidelines specify that all function parameters and return values should have type hints. Consider adding explicit annotations:
def test_evaluation_cron_job_success( client: TestClient, superuser_api_key: TestAuthContext, ) -> None: """Test successful cron job execution.""" ...Apply this pattern to all test functions in the file for consistency with coding guidelines.
backend/app/tests/services/doctransformer/test_zerox_transformer.py (1)
43-66: Add return type hints to pytest fixtures.For consistency with coding guidelines, add explicit return type annotations to the fixtures:
+from pathlib import Path +from unittest.mock import Mock + @pytest.fixture -def temp_input_file(self, tmp_path): +def temp_input_file(self, tmp_path) -> Path: """Create a temporary input file.""" ... @pytest.fixture -def temp_output_file(self, tmp_path): +def temp_output_file(self, tmp_path) -> Path: """Create a temporary output file path.""" ... @pytest.fixture -def mock_zerox_result(self): +def mock_zerox_result(self) -> Mock: """Create a mock zerox result with pages.""" ...As per coding guidelines, all function return values should have type hints.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py(1 hunks)backend/app/tests/api/routes/documents/test_route_document_remove.py(1 hunks)backend/app/tests/api/routes/test_assistants.py(1 hunks)backend/app/tests/api/routes/test_creds.py(2 hunks)backend/app/tests/api/routes/test_cron.py(1 hunks)backend/app/tests/api/routes/test_evaluation.py(9 hunks)backend/app/tests/api/routes/test_fine_tuning.py(1 hunks)backend/app/tests/api/routes/test_llm.py(1 hunks)backend/app/tests/api/routes/test_login.py(0 hunks)backend/app/tests/api/routes/test_responses.py(1 hunks)backend/app/tests/api/routes/test_threads.py(1 hunks)backend/app/tests/api/routes/test_users.py(0 hunks)backend/app/tests/api/test_deps.py(1 hunks)backend/app/tests/core/batch/test_openai.py(1 hunks)backend/app/tests/core/test_callback_ssrf.py(1 hunks)backend/app/tests/core/test_providers.py(1 hunks)backend/app/tests/core/test_security.py(1 hunks)backend/app/tests/crud/collections/collection/test_crud_collection_delete.py(0 hunks)backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py(2 hunks)backend/app/tests/crud/collections/collection/test_crud_collection_read_one.py(0 hunks)backend/app/tests/crud/collections/collection_job/test_collection_jobs.py(1 hunks)backend/app/tests/crud/config/test_config.py(1 hunks)backend/app/tests/crud/config/test_version.py(1 hunks)backend/app/tests/crud/documents/documents/test_crud_document_delete.py(0 hunks)backend/app/tests/crud/documents/documents/test_crud_document_read_many.py(0 hunks)backend/app/tests/crud/documents/documents/test_crud_document_read_one.py(0 hunks)backend/app/tests/crud/documents/documents/test_crud_document_update.py(0 hunks)backend/app/tests/crud/documents/test_doc_transformation_job.py(0 hunks)backend/app/tests/crud/evaluations/test_batch.py(1 hunks)backend/app/tests/crud/evaluations/test_core.py(1 hunks)backend/app/tests/crud/evaluations/test_cron.py(1 hunks)backend/app/tests/crud/evaluations/test_dataset.py(3 hunks)backend/app/tests/crud/evaluations/test_embeddings.py(3 hunks)backend/app/tests/crud/evaluations/test_langfuse.py(5 hunks)backend/app/tests/crud/evaluations/test_processing.py(1 hunks)backend/app/tests/crud/test_api_key.py(1 hunks)backend/app/tests/crud/test_assistants.py(1 hunks)backend/app/tests/crud/test_credentials.py(0 hunks)backend/app/tests/crud/test_jobs.py(1 hunks)backend/app/tests/crud/test_openai_conversation.py(1 hunks)backend/app/tests/crud/test_thread_result.py(1 hunks)backend/app/tests/seed_data/seed_data.py(1 hunks)backend/app/tests/services/collections/test_create_collection.py(1 hunks)backend/app/tests/services/doctransformer/test_job/test_execute_job.py(1 hunks)backend/app/tests/services/doctransformer/test_job/test_execute_job_errors.py(0 hunks)backend/app/tests/services/doctransformer/test_job/test_integration.py(0 hunks)backend/app/tests/services/doctransformer/test_job/test_start_job.py(1 hunks)backend/app/tests/services/doctransformer/test_zerox_transformer.py(1 hunks)backend/app/tests/services/llm/test_jobs.py(0 hunks)backend/app/tests/services/response/response/test_process_response.py(1 hunks)backend/app/tests/services/response/test_jobs_response.py(1 hunks)backend/app/tests/utils/document.py(1 hunks)backend/app/tests/utils/openai.py(2 hunks)backend/app/tests/utils/test_data.py(2 hunks)backend/app/tests/utils/utils.py(0 hunks)backend/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (14)
- backend/app/tests/crud/documents/test_doc_transformation_job.py
- backend/app/tests/crud/collections/collection/test_crud_collection_delete.py
- backend/app/tests/crud/documents/documents/test_crud_document_delete.py
- backend/app/tests/services/doctransformer/test_job/test_execute_job_errors.py
- backend/app/tests/services/llm/test_jobs.py
- backend/app/tests/services/doctransformer/test_job/test_integration.py
- backend/app/tests/crud/collections/collection/test_crud_collection_read_one.py
- backend/app/tests/crud/test_credentials.py
- backend/app/tests/crud/documents/documents/test_crud_document_update.py
- backend/app/tests/api/routes/test_login.py
- backend/app/tests/crud/documents/documents/test_crud_document_read_one.py
- backend/app/tests/api/routes/test_users.py
- backend/app/tests/utils/utils.py
- backend/app/tests/crud/documents/documents/test_crud_document_read_many.py
✅ Files skipped from review due to trivial changes (14)
- backend/app/tests/crud/test_jobs.py
- backend/app/tests/crud/collections/collection_job/test_collection_jobs.py
- backend/app/tests/services/collections/test_create_collection.py
- backend/app/tests/api/routes/test_llm.py
- backend/app/tests/crud/test_api_key.py
- backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py
- backend/app/tests/crud/test_thread_result.py
- backend/app/tests/crud/evaluations/test_embeddings.py
- backend/app/tests/api/routes/documents/test_route_document_remove.py
- backend/app/tests/api/routes/test_fine_tuning.py
- backend/app/tests/crud/config/test_version.py
- backend/app/tests/api/test_deps.py
- backend/app/tests/services/doctransformer/test_job/test_start_job.py
- backend/app/tests/crud/test_openai_conversation.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/api/routes/test_assistants.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/services/doctransformer/test_zerox_transformer.pybackend/app/tests/crud/evaluations/test_core.pybackend/app/tests/crud/config/test_config.pybackend/app/tests/core/batch/test_openai.pybackend/app/tests/services/doctransformer/test_job/test_execute_job.pybackend/app/tests/utils/test_data.pybackend/app/tests/utils/openai.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/api/routes/test_cron.pybackend/app/tests/crud/evaluations/test_cron.pybackend/app/tests/core/test_callback_ssrf.pybackend/app/tests/api/routes/test_responses.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/crud/evaluations/test_batch.pybackend/app/tests/core/test_security.pybackend/app/tests/api/routes/test_threads.pybackend/app/tests/core/test_providers.pybackend/app/tests/crud/test_assistants.pybackend/app/tests/services/response/test_jobs_response.pybackend/app/tests/seed_data/seed_data.pybackend/app/tests/crud/evaluations/test_dataset.pybackend/app/tests/utils/document.pybackend/app/tests/services/response/response/test_process_response.pybackend/app/tests/api/routes/test_creds.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/test_assistants.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/services/doctransformer/test_zerox_transformer.pybackend/app/tests/crud/evaluations/test_core.pybackend/app/tests/crud/config/test_config.pybackend/app/tests/core/batch/test_openai.pybackend/app/tests/services/doctransformer/test_job/test_execute_job.pybackend/app/tests/utils/test_data.pybackend/app/tests/utils/openai.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/api/routes/test_cron.pybackend/app/tests/crud/evaluations/test_cron.pybackend/app/tests/core/test_callback_ssrf.pybackend/app/tests/api/routes/test_responses.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/crud/evaluations/test_batch.pybackend/app/tests/core/test_security.pybackend/app/tests/api/routes/test_threads.pybackend/app/tests/core/test_providers.pybackend/app/tests/crud/test_assistants.pybackend/app/tests/services/response/test_jobs_response.pybackend/app/tests/seed_data/seed_data.pybackend/app/tests/crud/evaluations/test_dataset.pybackend/app/tests/utils/document.pybackend/app/tests/services/response/response/test_process_response.pybackend/app/tests/api/routes/test_creds.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.458Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.457Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.457Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Applied to files:
backend/app/tests/api/routes/test_assistants.py
📚 Learning: 2025-12-17T15:39:30.458Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.458Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
Applied to files:
backend/app/tests/seed_data/seed_data.py
🧬 Code graph analysis (13)
backend/app/tests/crud/evaluations/test_processing.py (1)
backend/app/crud/evaluations/processing.py (5)
check_and_process_evaluation(443-648)parse_evaluation_output(45-176)process_completed_embedding_batch(319-440)process_completed_evaluation(179-316)poll_all_pending_evaluations(651-812)
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (2)
backend/app/crud/collection/collection.py (1)
delete(103-111)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/services/doctransformer/test_zerox_transformer.py (2)
backend/app/services/doctransform/zerox_transformer.py (1)
ZeroxTransformer(12-70)backend/app/services/doctransform/transformer.py (1)
Transformer(5-14)
backend/app/tests/core/batch/test_openai.py (2)
backend/app/core/batch/openai.py (1)
OpenAIBatchProvider(14-254)backend/app/tests/utils/openai.py (1)
create_mock_batch(141-177)
backend/app/tests/utils/test_data.py (2)
backend/app/models/evaluation.py (1)
EvaluationDataset(73-167)backend/app/tests/utils/utils.py (1)
random_lower_string(17-18)
backend/app/tests/crud/evaluations/test_langfuse.py (1)
backend/app/core/langfuse/langfuse.py (1)
flush(110-111)
backend/app/tests/api/routes/test_cron.py (3)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-22)backend/app/tests/conftest.py (2)
superuser_api_key(96-98)user_api_key(102-104)backend/app/tests/utils/document.py (1)
get(117-121)
backend/app/tests/crud/evaluations/test_cron.py (3)
backend/app/crud/evaluations/cron.py (2)
process_all_pending_evaluations(20-143)process_all_pending_evaluations_sync(146-158)backend/app/crud/evaluations/core.py (1)
create_evaluation_run(14-61)backend/app/tests/utils/test_data.py (1)
create_test_evaluation_dataset(341-376)
backend/app/tests/api/routes/test_evaluation.py (3)
backend/app/tests/utils/test_data.py (1)
create_test_evaluation_dataset(341-376)backend/app/models/evaluation.py (1)
EvaluationRun(170-319)backend/app/tests/crud/evaluations/test_dataset.py (3)
TestDeleteDataset(385-519)test_delete_dataset_success(388-420)test_delete_dataset_not_found(422-437)
backend/app/tests/crud/evaluations/test_batch.py (1)
backend/app/crud/evaluations/batch.py (3)
build_evaluation_jsonl(62-115)fetch_dataset_items(24-59)start_evaluation_batch(118-212)
backend/app/tests/crud/evaluations/test_dataset.py (4)
backend/app/core/util.py (1)
now(11-12)backend/app/models/evaluation.py (1)
EvaluationRun(170-319)backend/app/api/routes/evaluation.py (1)
delete_dataset(390-418)backend/app/crud/evaluations/dataset.py (2)
delete_dataset(323-387)get_dataset_by_id(107-140)
backend/app/tests/utils/document.py (1)
backend/app/tests/utils/utils.py (1)
SequentialUuidGenerator(147-160)
backend/app/tests/api/routes/test_creds.py (4)
backend/app/core/providers.py (1)
Provider(9-14)backend/app/models/credentials.py (1)
Credential(67-131)backend/app/core/security.py (1)
decrypt_credentials(171-188)backend/app/tests/utils/utils.py (1)
generate_random_string(21-22)
🪛 Ruff (0.14.8)
backend/app/tests/services/doctransformer/test_zerox_transformer.py
13-13: Unused function argument: args
(ARG001)
13-13: Unused function argument: kwargs
(ARG001)
122-122: Unused function argument: args
(ARG001)
122-122: Unused function argument: kwargs
(ARG001)
426-426: Unused function argument: args
(ARG001)
426-426: Unused function argument: kwargs
(ARG001)
backend/app/tests/crud/evaluations/test_cron.py
13-13: Redefinition of unused Organization from line 12
Remove definition: Organization
(F811)
13-13: Redefinition of unused Project from line 12
Remove definition: Project
(F811)
backend/app/tests/seed_data/seed_data.py
85-85: Unnecessary mode argument
Remove mode argument
(UP015)
325-325: Local variable users is assigned to but never used
Remove assignment to unused variable users
(F841)
backend/app/tests/api/routes/test_creds.py
170-170: Avoid equality comparisons to True; use Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (33)
backend/app/tests/api/routes/test_threads.py (1)
5-6: LGTM! Import consolidation improves code organization.Moving the OpenAI imports to the top-level follows Python best practices and eliminates redundant inline imports.
backend/app/tests/services/response/response/test_process_response.py (6)
4-6: LGTM: Import additions support enhanced test structure.The new imports enable direct OpenAI client instantiation and explicit Session typing, which align well with the test refactoring objectives.
24-53: LGTM: Well-structured test fixture.The fixture properly sets up test data with appropriate type hints. Using a hardcoded API key on Line 39 is acceptable since all OpenAI calls are mocked in the tests.
65-101: LGTM: Comprehensive success case test.The test properly mocks external dependencies (OpenAI, Langfuse) and verifies both the API response and job status updates. Type hints are correctly applied.
104-123: LGTM: Proper error handling test.The test correctly verifies the behavior when an assistant is not found, checking both the API response and job status update.
126-158: LGTM: Proper failure case test.The test correctly verifies error handling when
generate_responsefails, ensuring the error propagates to the API response and job status.
161-186: LGTM: Robust exception handling test.The test verifies that unexpected exceptions are caught and handled gracefully, with appropriate error messages and job status updates.
backend/app/tests/core/test_callback_ssrf.py (1)
3-5: LGTM: Import reordering improves organization.The import reordering is clean and maintains proper grouping (standard library, third-party, local imports).
backend/app/tests/crud/config/test_config.py (1)
33-494: Excellent test coverage!The test suite provides comprehensive coverage of Config CRUD operations including:
- Creation with various scenarios (duplicate names, different projects)
- Reading (single, all, pagination, ordering)
- Updates (various fields, duplicate name handling)
- Soft deletion
- Edge cases and error conditions
The tests are well-structured, properly documented, and follow pytest best practices.
backend/app/tests/crud/evaluations/test_dataset.py (2)
17-19: LGTM!The new imports are appropriate for the deletion tests and properly organized.
385-519: Excellent test coverage for dataset deletion!The
TestDeleteDatasetclass provides comprehensive coverage of deletion scenarios:
- ✅ Successful deletion
- ✅ Non-existent dataset handling
- ✅ Organization-level access control
- ✅ Referential integrity with evaluation runs
The test assertions are robust with flexible message matching, and the logic correctly validates the expected behavior from the
delete_datasetfunction.backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (2)
4-4: LGTM: Import addition supports the new deletion pattern.The addition of
deleteto the SQLModel imports is necessary and correct for the updated deletion approach used in the fixture and test method.
40-40: LGTM: Deletion pattern updated to use SQLModel expression.The change from ORM-based deletion to SQLModel's
delete()expression is an improvement and aligns with the broader pattern being adopted across the test suite. The absence of an explicit commit is correct here, as thedbfixture from conftest.py manages transactions with automatic rollback.backend/app/tests/services/doctransformer/test_job/test_execute_job.py (1)
1-18: LGTM on import cleanup.The removal of the unused
UUIDimport is appropriate since onlyuuid4is used throughout the file.backend/app/tests/api/routes/test_assistants.py (1)
1-8: LGTM on import reorganization.The import ordering now follows the standard convention: stdlib (
unittest.mock,uuid) before third-party packages (pytest,sqlmodel,fastapi).backend/app/tests/crud/test_assistants.py (1)
1-6: LGTM on import reorganization.The import ordering is consistent with other test files in this PR, placing stdlib imports before third-party packages.
backend/app/tests/api/routes/test_responses.py (1)
1-5: LGTM on import grouping.The blank line correctly separates third-party imports (
unittest.mock,fastapi) from local application imports (app.models), following PEP 8 conventions.backend/app/tests/utils/document.py (1)
19-19: LGTM! Import path made more explicit.The change from relative to absolute import improves clarity and consistency across the test suite.
backend/app/tests/core/test_security.py (1)
3-9: LGTM! Unused imports removed.Cleaning up the unused
get_password_hashandverify_passwordimports improves code maintainability.backend/app/tests/core/test_providers.py (1)
2-6: LGTM! Unused import removed.Removing the unused
Providerimport keeps the test module clean.backend/app/tests/services/response/test_jobs_response.py (1)
2-8: LGTM! Unused import removed.The
Jobmodel import was not needed since jobs are accessed viaJobCrud.backend/app/tests/utils/test_data.py (1)
341-376: LGTM! Well-structured test data factory.The new
create_test_evaluation_datasethelper follows the established factory pattern for test fixtures and includes proper type hints for all parameters and return type, as required by the coding guidelines.backend/app/tests/api/routes/test_evaluation.py (1)
780-984: Excellent test coverage additions!The new test cases significantly improve coverage by validating authentication requirements, error handling for non-existent resources, and edge cases. The tests follow consistent patterns and will help catch regressions.
backend/app/tests/api/routes/test_cron.py (1)
9-184: Excellent comprehensive test coverage!The test suite thoroughly covers the cron evaluation endpoint including success scenarios, error handling, authentication, authorization, and OpenAPI schema validation. The tests are well-structured and will effectively catch regressions.
backend/app/tests/services/doctransformer/test_zerox_transformer.py (1)
19-438: Outstanding comprehensive test coverage!This test suite provides exceptional coverage of the
ZeroxTransformerincluding:
- Initialization scenarios
- Successful transformations with various content types
- Error handling (timeouts, missing files, Poppler errors)
- Edge cases (empty pages, None content, mixed content)
- Content formatting and encoding (unicode, multiline)
- Large document handling
- Integration-style end-to-end flows
The tests are well-organized, use appropriate mocking strategies, and will effectively prevent regressions.
backend/app/tests/api/routes/test_creds.py (2)
2-2: LGTM!The addition of
Sessionandselectimports is appropriate for the new encryption verification tests that require direct database access.
161-181: Add type hints to function parameters.Per coding guidelines, all function parameters should have type hints. Additionally, the static analysis hint about using
Credential.is_activeinstead of== Trueon line 170 is valid for more idiomatic code.🔎 Proposed fix
def test_credential_encryption( - db: Session, - user_api_key: TestAuthContext, + db: Session, + user_api_key: TestAuthContext, ): """Verify credentials are encrypted in database.""" db_credential = db.exec( select(Credential).where( Credential.organization_id == user_api_key.organization_id, Credential.project_id == user_api_key.project_id, - Credential.is_active == True, + Credential.is_active, Credential.provider == Provider.OPENAI.value, ) ).first()As per coding guidelines, type hints are required for all function parameters.
Likely an incorrect or invalid review comment.
backend/app/tests/crud/evaluations/test_processing.py (1)
1-805: LGTM! Comprehensive test coverage.The test suite is well-structured with:
- Clear test organization into logical classes
- Appropriate use of fixtures and mocking
- Good coverage of success, error, and edge cases
- Proper async test handling with
@pytest.mark.asyncio- Clear, descriptive test names and docstrings
backend/app/tests/utils/openai.py (1)
141-177: LGTM! Well-designed test utility.The
create_mock_batchfunction is well-implemented with:
- Clear type hints on all parameters and return type
- Comprehensive docstring explaining usage
- Sensible default values for common scenarios
- Proper MagicMock configuration
backend/app/tests/crud/evaluations/test_core.py (1)
1-647: LGTM! Comprehensive CRUD test coverage.The test suite provides excellent coverage of evaluation run CRUD operations:
- Creation (minimal and complete configurations)
- Listing (empty, multiple, pagination, org/project scoping)
- Retrieval (success, not-found, cross-org scenarios)
- Updates (individual and multiple fields)
- Score operations (caching, fetching, force-refetch)
All tests are well-structured with appropriate fixtures, clear names, and good assertions.
backend/app/tests/core/batch/test_openai.py (1)
1-441: LGTM! Thorough batch provider testing.The test suite provides excellent coverage of the OpenAI batch provider:
- Initialization and basic operations
- Batch creation (success, defaults, errors)
- Status retrieval (completed, in_progress, failed, expired, cancelled)
- Result downloading (success, errors, malformed JSON, empty files)
- File operations (upload/download with various scenarios)
- JSONL formatting validation
All tests use appropriate mocking and have clear assertions.
backend/app/tests/crud/evaluations/test_langfuse.py (1)
1-471: LGTM! Comprehensive Langfuse integration tests.The test suite provides thorough coverage of Langfuse integration:
- Dataset run creation (success, missing items, errors, empty results, cost tracking)
- Trace updates with cosine scores (success, missing trace_id, error handling)
- Dataset uploads (success, duplication, metadata, empty items, errors)
All tests properly mock external dependencies and verify expected behaviors.
backend/app/tests/crud/evaluations/test_batch.py (1)
18-488: Excellent test coverage and organization.The test suite comprehensively covers:
- Success scenarios with various configurations
- Error handling (dataset not found, empty datasets, fetch failures)
- Edge cases (missing metadata, empty questions, minimal config)
- Integration with tools configuration
The tests appropriately use mocking to isolate units under test while maintaining realistic database interactions where needed.
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py
Outdated
Show resolved
Hide resolved
backend/app/tests/services/doctransformer/test_zerox_transformer.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
backend/app/tests/seed_data/seed_data.py (4)
319-327: Critical: IndexError risk remains unresolved.Despite being marked as addressed in a previous review, accessing
users[1]without validating list length will raiseIndexErrorif fewer than 2 users exist. The check on line 326 is unreachable because the error occurs first.🔎 Proposed fix
users = session.exec( select(User) .join(APIKey, APIKey.user_id == User.id) .where(APIKey.organization_id == organization.id) ).all() - user = users[1] - if not user: + if len(users) < 2: raise ValueError(f"No user found in organization '{organization.name}'") + user = users[1]
357-418: Correct log prefixes.🔎 Proposed fix
def seed_database(session: Session) -> None: """ Seed the database with initial test data. This function creates a complete test environment including: - Organizations (Project Tech4dev) - Projects (Glific, Dalgo) - Users (superuser, test user) - API Keys for both users - OpenAI Credentials for both projects (ensures all tests have credentials) - Langfuse Credentials for both projects (for tracing and observability tests) - Test Assistants for both projects - Sample Documents This seed data is used by the test suite and ensures that all tests can rely on both OpenAI and Langfuse credentials being available without manual setup. """ - logging.info("[tests.seed_data] Starting database seeding") + logging.info("[seed_database] Starting database seeding") try: clear_database(session) seed_data = load_seed_data() for org_data in seed_data["organization"]: create_organization(session, org_data) for user_data in seed_data["users"]: if user_data["email"] == "{{SUPERUSER_EMAIL}}": user_data["email"] = settings.FIRST_SUPERUSER elif user_data["email"] == "{{ADMIN_EMAIL}}": user_data["email"] = settings.EMAIL_TEST_USER for user_data in seed_data["users"]: create_user(session, user_data) for project_data in seed_data["projects"]: create_project(session, project_data) for api_key_data in seed_data["apikeys"]: if api_key_data["user_email"] == "{{SUPERUSER_EMAIL}}": api_key_data["user_email"] = settings.FIRST_SUPERUSER elif api_key_data["user_email"] == "{{ADMIN_EMAIL}}": api_key_data["user_email"] = settings.EMAIL_TEST_USER for api_key_data in seed_data["apikeys"]: create_api_key(session, api_key_data) for credential_data in seed_data["credentials"]: create_credential(session, credential_data) for assistant_data in seed_data.get("assistants", []): create_assistant(session, assistant_data) for document_data in seed_data.get("documents", []): create_document(session, document_data) session.commit() - logging.info("[tests.seed_data] Database seeded successfully") + logging.info("[seed_database] Database seeded successfully") except Exception as e: - logging.error(f"[tests.seed_data] Error seeding database: {e}") + logging.error(f"[seed_database] Error seeding database: {e}") session.rollback() raiseAs per coding guidelines, log messages must be prefixed with the function name.
99-109: Add complete type hints and correct log prefix.The parameter type hint and log prefix need updates per coding guidelines.
🔎 Proposed fix
-def create_organization(session: Session, org_data_raw: dict) -> Organization: +def create_organization(session: Session, org_data_raw: dict[str, Any]) -> Organization: """Create an organization from data.""" try: org_data = OrgData.model_validate(org_data_raw) organization = Organization(name=org_data.name, is_active=org_data.is_active) session.add(organization) session.flush() # Ensure ID is assigned return organization except Exception as e: - logging.error(f"[tests.seed_data]Error creating organization: {e}") + logging.error(f"[create_organization] Error creating organization: {e}") raiseAs per coding guidelines.
81-96: Fix type hints, log prefixes, and file mode.Multiple issues need attention:
- Return type should be
dict[str, Any]instead ofdict- Log messages must use
[load_seed_data]prefix (not[tests.seed_data])- The
"r"mode argument is unnecessary (it's the default)🔎 Proposed fix
+from typing import Any + -def load_seed_data() -> dict: +def load_seed_data() -> dict[str, Any]: """Load seed data from JSON file.""" json_path = Path(__file__).parent / "seed_data.json" try: - with open(json_path, "r") as f: + with open(json_path) as f: return json.load(f) except FileNotFoundError: logging.error( - f"[tests.seed_data]Error: Seed data file not found at {json_path}" + f"[load_seed_data] Seed data file not found at {json_path}" ) raise except json.JSONDecodeError as e: logging.error( - f"[tests.seed_data]Error: Failed to decode JSON from {json_path}: {e}" + f"[load_seed_data] Failed to decode JSON from {json_path}: {e}" ) raiseAs per coding guidelines, all functions must have complete type hints and log messages must be prefixed with the function name.
🧹 Nitpick comments (8)
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (3)
13-27: Add return type hint tocreate_collectionsfunction.Per coding guidelines, all function parameters and return values require type hints. The function returns
crud.project_idwhich is anint.🔎 Proposed fix
-def create_collections(db: Session, n: int): +def create_collections(db: Session, n: int) -> int: crud = None project = get_project(db)
33-40: Add return type hints to test methods.Per coding guidelines, all functions require return type hints. Test methods should have
-> Nonereturn type.🔎 Proposed fix
- def test_number_read_is_expected(self, db: Session): + def test_number_read_is_expected(self, db: Session) -> None: db.exec(delete(Collection))
42-45: Add return type hint.🔎 Proposed fix
- def test_deleted_docs_are_excluded(self, db: Session): + def test_deleted_docs_are_excluded(self, db: Session) -> None: owner = create_collections(db, self._ncollections)backend/app/tests/crud/evaluations/test_cron.py (1)
43-52: Avoid mutating the db session'sexecmethod directly.Replacing
db.execwith a MagicMock modifies the session object in a way that could leak to other tests if the session is reused. Consider usingpatch.objectfor safer, scoped mocking.🔎 Proposed fix
@pytest.mark.asyncio async def test_process_all_pending_evaluations_no_orgs(self, db: Session): """Test processing when there are no organizations.""" - # This is unlikely in practice but tests the edge case - # We can't actually remove all orgs due to seed data, so we'll just check - # that the function handles it gracefully by mocking - with patch("app.crud.evaluations.cron.select") as mock_select: - mock_stmt = MagicMock() - mock_select.return_value = mock_stmt - db.exec = MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))) - - result = await process_all_pending_evaluations(session=db) + with patch.object(db, 'exec', return_value=MagicMock(all=MagicMock(return_value=[]))): + result = await process_all_pending_evaluations(session=db) assert result["status"] == "success" assert result["organizations_processed"] == 0 assert result["total_processed"] == 0backend/app/seed_data/seed_data.py (2)
49-60: Add return type hint toload_seed_data.Per coding guidelines, all functions require return type hints.
🔎 Proposed fix
The function signature already has
-> dicton line 49, so this is correct. No change needed.
56-60: Consider using function name in log prefixes for consistency.The coding guidelines specify prefixing log messages with the function name in square brackets. Currently using
[seed_data](module name) instead of function-specific prefixes like[load_seed_data].🔎 Example
- logging.error(f"[seed_data] Seed data file not found at {json_path}") + logging.error(f"[load_seed_data] Seed data file not found at {json_path}")As per coding guidelines, log messages should use
[function_name]prefix.backend/app/tests/seed_data/seed_data.py (2)
5-5: Use Python 3.11+ union syntax instead ofOptional.Per coding guidelines, Python 3.11+ should use the
|operator for optional types instead of importingOptional.🔎 Proposed fix
-from typing import OptionalThen update lines 50 and 59 to use
str | Noneinstead ofOptional[str].As per coding guidelines, use Python 3.11+ features throughout the codebase.
383-402: Consider consolidating email placeholder replacement.The current two-pass approach (lines 383-387 for mutation, 389-390 for creation) works but mutates the seed data and loops twice. A single-pass approach would be more efficient.
🔎 Alternative approach
- for user_data in seed_data["users"]: - if user_data["email"] == "{{SUPERUSER_EMAIL}}": - user_data["email"] = settings.FIRST_SUPERUSER - elif user_data["email"] == "{{ADMIN_EMAIL}}": - user_data["email"] = settings.EMAIL_TEST_USER - for user_data in seed_data["users"]: + if user_data["email"] == "{{SUPERUSER_EMAIL}}": + user_data["email"] = settings.FIRST_SUPERUSER + elif user_data["email"] == "{{ADMIN_EMAIL}}": + user_data["email"] = settings.EMAIL_TEST_USER create_user(session, user_data) for project_data in seed_data["projects"]: create_project(session, project_data) - for api_key_data in seed_data["apikeys"]: - if api_key_data["user_email"] == "{{SUPERUSER_EMAIL}}": - api_key_data["user_email"] = settings.FIRST_SUPERUSER - elif api_key_data["user_email"] == "{{ADMIN_EMAIL}}": - api_key_data["user_email"] = settings.EMAIL_TEST_USER - for api_key_data in seed_data["apikeys"]: + if api_key_data["user_email"] == "{{SUPERUSER_EMAIL}}": + api_key_data["user_email"] = settings.FIRST_SUPERUSER + elif api_key_data["user_email"] == "{{ADMIN_EMAIL}}": + api_key_data["user_email"] = settings.EMAIL_TEST_USER create_api_key(session, api_key_data)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/seed_data/seed_data.pybackend/app/tests/conftest.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/evaluations/test_core.pybackend/app/tests/crud/evaluations/test_cron.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/seed_data/seed_data.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/conftest.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/crud/evaluations/test_core.pybackend/app/seed_data/seed_data.pybackend/app/tests/crud/evaluations/test_cron.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/seed_data/seed_data.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/crud/evaluations/test_core.pybackend/app/tests/crud/evaluations/test_cron.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/seed_data/seed_data.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
Applied to files:
backend/app/seed_data/seed_data.pybackend/app/tests/seed_data/seed_data.py
🧬 Code graph analysis (3)
backend/app/tests/crud/evaluations/test_core.py (1)
backend/app/crud/evaluations/core.py (5)
create_evaluation_run(14-61)get_evaluation_run_by_id(103-141)get_or_fetch_score(199-256)save_score(259-295)update_evaluation_run(144-196)
backend/app/tests/crud/evaluations/test_cron.py (3)
backend/app/crud/evaluations/cron.py (2)
process_all_pending_evaluations(20-143)process_all_pending_evaluations_sync(146-158)backend/app/crud/evaluations/core.py (1)
create_evaluation_run(14-61)backend/app/models/batch_job.py (1)
BatchJob(15-136)
backend/app/tests/seed_data/seed_data.py (3)
backend/app/core/security.py (2)
get_password_hash(100-110)encrypt_credentials(151-168)backend/app/models/api_key.py (1)
APIKey(48-89)backend/app/models/credentials.py (1)
Credential(67-131)
🪛 Ruff (0.14.8)
backend/app/tests/crud/evaluations/test_core.py
27-27: Undefined name test_dataset
(F821)
28-28: Undefined name test_dataset
(F821)
30-30: Undefined name test_dataset
(F821)
31-31: Undefined name test_dataset
(F821)
36-36: Undefined name test_dataset
(F821)
37-37: Undefined name test_dataset
(F821)
40-40: Undefined name test_dataset
(F821)
41-41: Undefined name test_dataset
(F821)
backend/app/tests/seed_data/seed_data.py
85-85: Unnecessary mode argument
Remove mode argument
(UP015)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.11.7, 6)
🔇 Additional comments (13)
backend/app/tests/crud/evaluations/test_langfuse.py (5)
311-321: LGTM!The test correctly validates that items without
trace_idare skipped. The assertionmock_langfuse.score.call_count == 2confirms only valid items are processed.
378-385: LGTM!The assertions correctly reflect the duplication logic: 3 items × 5 duplication factor = 15 total items.
409-411: LGTM!The duplicate number distribution is correct: with 3 original items and duplication_factor=3, each duplicate_number (1, 2, 3) appears exactly 3 times.
446-448: LGTM!Correctly asserts 3 total items when duplication_factor=1.
457-470: LGTM!The test correctly validates error handling during item creation. The side_effect list simulates one success, one failure, and one success, resulting in
total_items == 2.backend/app/tests/crud/evaluations/test_core.py (3)
71-90: LGTM!The
test_datasetfixture is properly defined with all required parameters and uses the factory pattern appropriately.
576-611: LGTM!The test appropriately documents the limitation with
save_scorecreating its own session, and usesupdate_evaluation_runas a surrogate to test the core functionality.
613-627:save_scorecorrectly handles non-existent evaluation runs without interfering with test sessions.The implementation creates its own database session via
Session(engine), ensuring complete isolation from the test's transactional session. When the evaluation run doesn't exist,get_evaluation_run_by_idreturnsNone, which the function correctly returns as specified by its-> EvaluationRun | Nonereturn type. The test expectation ofresult is Noneis properly satisfied.backend/app/tests/crud/evaluations/test_cron.py (3)
106-134: LGTM!The test properly creates a second organization and project to validate multi-org processing. The fixture setup is correct and the assertion
organizations_processed >= 2accounts for seed data orgs.
275-328: LGTM!The integration test properly sets up a complete scenario with a batch job and evaluation run, then verifies cron processing handles it correctly. The mock setup for OpenAI and Langfuse clients is appropriate.
330-348: LGTM!Good edge case coverage - verifying that completed evaluations are not reprocessed by the cron job.
backend/app/seed_data/seed_data.py (2)
191-238: LGTM!The seed flow is simplified by removing unnecessary list accumulation. The function properly handles transactions with commit on success and rollback on error.
241-244: LGTM!The main guard is simplified appropriately. Based on learnings, this seed module is invoked via
uv run python -m app.seed_data.seed_data.
There was a problem hiding this comment.
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 (3)
backend/app/tests/utils/document.py (3)
25-26: Add missing return type annotation.The
httpx_to_standardfunction is missing a return type annotation.As per coding guidelines: Always add type hints to all function parameters and return values in Python code.
🔎 Proposed fix
-def httpx_to_standard(response: Response): +def httpx_to_standard(response: Response) -> APIResponse: return APIResponse(**response.json())
30-52: Add missing type annotations to DocumentMaker methods.The
DocumentMakerclass methods are missing return type annotations.As per coding guidelines: Always add type hints to all function parameters and return values in Python code.
🔎 Proposed fix
- def __init__(self, project_id: int, session: Session): + def __init__(self, project_id: int, session: Session) -> None: self.project_id = project_id self.session = session self.project: Project = get_project_by_id( session=self.session, project_id=self.project_id ) self.index = SequentialUuidGenerator() - def __iter__(self): + def __iter__(self) -> "DocumentMaker": return self - def __next__(self): + def __next__(self) -> Document: doc_id = next(self.index)
158-167: Add missing return type annotation.The
to_public_dictmethod is missing a return type annotation.As per coding guidelines: Always add type hints to all function parameters and return values in Python code.
🔎 Proposed fix
- def to_public_dict(self) -> dict: + def to_public_dict(self) -> dict[str, Any]: """Convert Document to dict matching DocumentPublic schema."""
🧹 Nitpick comments (1)
backend/app/tests/api/routes/test_evaluation.py (1)
868-879: Consider consolidating duplicate fixtures.The
create_test_datasetfixture is duplicated acrossTestGetEvaluationRunStatus(lines 698-709),TestGetDataset(lines 868-879), andTestDeleteDataset(lines 927-938). The only differences are thenameanddescriptionvalues.You could consolidate these into a single shared fixture (e.g., in
conftest.py) or parameterize them, reducing duplication.🔎 Example consolidation approach
Move to
conftest.pywith a more generic name:@pytest.fixture def test_evaluation_dataset(db, user_api_key): """Create a test evaluation dataset for evaluation-related tests.""" return create_test_evaluation_dataset( db=db, organization_id=user_api_key.organization_id, project_id=user_api_key.project_id, )Then use this shared fixture across all test classes.
Also applies to: 927-938
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/app/tests/api/routes/test_evaluation.pybackend/app/tests/conftest.pybackend/app/tests/utils/auth.pybackend/app/tests/utils/collection.pybackend/app/tests/utils/document.pybackend/app/tests/utils/openai.pybackend/app/tests/utils/test_data.pybackend/app/tests/utils/utils.py
💤 Files with no reviewable changes (1)
- backend/app/tests/utils/auth.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/conftest.py
- backend/app/tests/utils/openai.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/utils/collection.pybackend/app/tests/utils/test_data.pybackend/app/tests/utils/document.pybackend/app/tests/utils/utils.pybackend/app/tests/api/routes/test_evaluation.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/utils/collection.pybackend/app/tests/utils/test_data.pybackend/app/tests/utils/document.pybackend/app/tests/utils/utils.pybackend/app/tests/api/routes/test_evaluation.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
🧬 Code graph analysis (4)
backend/app/tests/utils/collection.py (1)
backend/app/models/project.py (1)
Project(51-107)
backend/app/tests/utils/test_data.py (2)
backend/app/models/evaluation.py (1)
EvaluationDataset(73-167)backend/app/tests/utils/utils.py (1)
random_lower_string(17-18)
backend/app/tests/utils/document.py (7)
backend/app/tests/crud/documents/documents/test_crud_document_update.py (1)
documents(11-14)backend/app/tests/api/routes/documents/test_route_document_upload.py (2)
put(25-46)route(75-76)backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py (1)
route(28-29)backend/app/tests/api/routes/documents/test_route_document_remove.py (1)
route(20-21)backend/app/tests/api/routes/documents/test_route_document_info.py (1)
route(16-17)backend/app/tests/api/routes/documents/test_route_document_list.py (1)
route(23-24)backend/app/tests/crud/documents/documents/test_crud_document_delete.py (1)
document(13-22)
backend/app/tests/api/routes/test_evaluation.py (1)
backend/app/tests/utils/test_data.py (1)
create_test_evaluation_dataset(341-376)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (12)
backend/app/tests/utils/collection.py (1)
11-11: LGTM! Type hints correctly added throughout.The addition of the
Projectimport and explicit type annotations enhances type safety and aligns with the coding guidelines. All function parameters and return values now have complete type hints.Also applies to: 21-21, 28-28, 53-53, 77-77
backend/app/tests/api/routes/test_evaluation.py (4)
9-9: Good adoption of the factory pattern.The import of
create_test_evaluation_datasetfrom the test utilities aligns with the factory pattern for test fixtures. Based on learnings, this is the preferred approach forbackend/app/tests/.
698-709: Fixture correctly uses the shared factory.The fixture now delegates dataset creation to the centralized
create_test_evaluation_datasetfactory, which improves maintainability and consistency.
780-862: Comprehensive test coverage for evaluation run edge cases.The new tests cover important scenarios:
- 404 for non-existent evaluation runs
- Successful retrieval without trace info
- Validation that
resync_scorerequiresget_trace_info- 401 for unauthenticated access
940-962: Good test design with verification of deletion.The delete test properly verifies that the dataset is actually removed by making a follow-up GET request and asserting 404. This is a thorough approach to testing delete operations.
backend/app/tests/utils/test_data.py (2)
21-21: Import addition for new model usage.The
EvaluationDatasetimport is correctly added to support the new factory function.
341-376: Well-structured factory function following established patterns.The
create_test_evaluation_datasetfunction:
- Has complete type hints on all parameters and return value (per coding guidelines)
- Follows the factory pattern (per learnings for
backend/app/tests/)- Uses sensible defaults for optional parameters
- Properly handles name generation with
random_lower_string()- Follows the same db persistence pattern (add → commit → refresh) as other factories in this file
backend/app/tests/utils/utils.py (4)
21-22: LGTM!Type annotations added correctly for both parameter and return type.
148-152: LGTM!Type annotations added correctly for
__init__and__iter__methods.
154-157: Add missing return type annotation.The
__next__method is missing a return type annotation.As per coding guidelines: Always add type hints to all function parameters and return values in Python code.
🔎 Proposed fix
- def __next__(self) -> UUID: + def __next__(self) -> UUID:Wait, actually looking at the code more carefully, it appears the return type
-> UUIDis already present on line 154. Let me reconsider.Actually, I need to look at the annotated code more carefully. The lines shown are:
154 def __next__(self) -> UUID: 155 uu_id = UUID(int=self.start) 156 self.start += 1 157 return uu_idThis already has the return type. So this is fine. Let me skip this comment.
159-160: Add missing return type annotation.The
peekmethod is missing a return type annotation.As per coding guidelines: Always add type hints to all function parameters and return values in Python code.
🔎 Proposed fix
- def peek(self) -> UUID: + def peek(self) -> UUID:Wait, I see
-> UUIDis already there on line 159. Let me reconsider the annotated code.Looking at the code:
159 def peek(self) -> UUID: 160 return UUID(int=self.start)This already has the return type annotation. So this is actually fine. Let me skip this comment.
backend/app/tests/utils/document.py (1)
3-4: LGTM!Type annotations have been added correctly throughout the file, improving type safety and code clarity.
Also applies to: 56-82, 89-112, 120-130, 138-149, 151-156, 171-172
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/tests/seed_data/seed_data.py (1)
321-329: Fix IndexError risk with direct list access.Accessing
users[1]without first checking the list length will raise anIndexErrorif fewer than 2 users exist. Theif not user:check on line 328 is unreachable because the IndexError occurs first.🔎 Proposed fix
users = session.exec( select(User) .join(APIKey, APIKey.user_id == User.id) .where(APIKey.organization_id == organization.id) ).all() - user = users[1] - if not user: + if len(users) < 2: raise ValueError(f"No user found in organization '{organization.name}'") + user = users[1] document = Document(
🧹 Nitpick comments (13)
backend/app/tests/utils/document.py (3)
3-3: Consider importingGeneratorfromcollections.abcfor Python 3.11+.Per static analysis and PEP 585,
Generatorshould be imported fromcollections.abcrather thantypingin Python 3.9+. Since the codebase uses Python 3.11+, this aligns with modern Python best practices.🔎 Proposed fix
-from typing import Any, Generator +from typing import Any +from collections.abc import Generator
157-166: Add return type hint toto_public_dictmethod.The
to_public_dictmethod is missing a return type annotation. Per coding guidelines, all function parameters and return values should have type hints.🔎 Proposed fix
- def to_public_dict(self) -> dict: + def to_public_dict(self) -> dict[str, Any]:Note: You'll need to ensure
Anyis imported fromtypingat the top of the file (it already is on line 3).As per coding guidelines.
29-51: Consider adding type hints to DocumentMaker class for consistency.While the rest of the file received type hint enhancements, the
DocumentMakerclass methods (__init__,__iter__, and__next__) remain untyped. For consistency with the coding guidelines and the rest of this file, consider adding type annotations.🔎 Proposed fix
- def __init__(self, project_id: int, session: Session): + def __init__(self, project_id: int, session: Session) -> None: self.project_id = project_id self.session = session self.project: Project = get_project_by_id( session=self.session, project_id=self.project_id ) self.index = SequentialUuidGenerator() - def __iter__(self): + def __iter__(self) -> "DocumentMaker": return self - def __next__(self): + def __next__(self) -> Document: doc_id = next(self.index)As per coding guidelines.
backend/app/tests/conftest.py (1)
10-10: Consider usingcollections.abc.Generatorfor Python 3.9+ compatibility.The import works correctly, but importing from
collections.abcis preferred in modern Python versions.🔎 Proposed fix
-from typing import Any, Generator +from typing import Any +from collections.abc import Generatorbackend/app/seed_data/seed_data.py (1)
49-60: Remove unnecessary mode argument.The
"r"mode is the default foropen()and can be omitted for cleaner code.🔎 Proposed fix
- with open(json_path, "r") as f: + with open(json_path) as f: return json.load(f)backend/app/tests/seed_data/seed_data.py (8)
85-85: Remove unnecessary mode argument.The
"r"mode is the default foropen()and can be omitted.🔎 Proposed fix
- with open(json_path, "r") as f: + with open(json_path) as f: return json.load(f)
88-95: Add space after log prefix for consistency.The log messages are missing a space after the closing bracket of the prefix, making them harder to read.
🔎 Proposed fix
logging.error( - f"[tests.seed_data]Error: Seed data file not found at {json_path}" + f"[tests.seed_data] Seed data file not found at {json_path}" ) raise except json.JSONDecodeError as e: logging.error( - f"[tests.seed_data]Error: Failed to decode JSON from {json_path}: {e}" + f"[tests.seed_data] Failed to decode JSON from {json_path}: {e}" )Based on learnings, the file-based prefix
[tests.seed_data]is correct for this test seed data module.
108-108: Add space after log prefix.- logging.error(f"[tests.seed_data]Error creating organization: {e}") + logging.error(f"[tests.seed_data] Error creating organization: {e}")Based on learnings, the file-based prefix is correct for test seed data modules.
135-135: Add space after log prefix.- logging.error(f"[tests.seed_data]Error creating project: {e}") + logging.error(f"[tests.seed_data] Error creating project: {e}") raise- logging.error(f"[tests.seed_data]Error creating user: {e}") + logging.error(f"[tests.seed_data] Error creating user: {e}") raiseBased on learnings.
Also applies to: 155-155
194-195: Initialize CryptContext as a module-level constant.Creating a new
CryptContextinstance on every call is inefficient. Define it once at the module level.🔎 Proposed fix
Add at module level (after imports, around line 22):
# Module-level password context for API key hashing _pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")Then update the function:
key_prefix = key_portion[:12] - pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") - key_hash = pwd_context.hash(key_portion[12:]) + key_hash = _pwd_context.hash(key_portion[12:]) api_key = APIKey(
210-210: Add space after log prefix.- logging.error(f"[tests.seed_data]Error creating API key: {e}") + logging.error(f"[tests.seed_data] Error creating API key: {e}")
252-252: Add space after log prefix.- logging.error(f"[tests.seed_data]Error creating credential: {e}") + logging.error(f"[tests.seed_data] Error creating credential: {e}") raise- logging.error(f"[tests.seed_data]Error creating assistant: {e}") + logging.error(f"[tests.seed_data] Error creating assistant: {e}") raiseAlso applies to: 291-291
342-342: Add space after log prefix.- logging.error(f"[tests.seed_data]Error creating document: {e}") + logging.error(f"[tests.seed_data] Error creating document: {e}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/seed_data/seed_data.pybackend/app/tests/conftest.pybackend/app/tests/seed_data/seed_data.pybackend/app/tests/utils/document.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/seed_data/seed_data.pybackend/app/tests/seed_data/seed_data.pybackend/app/tests/utils/document.pybackend/app/tests/conftest.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/seed_data/seed_data.pybackend/app/tests/utils/document.pybackend/app/tests/conftest.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
Learnt from: nishika26
Repo: ProjectTech4DevAI/kaapi-backend PR: 488
File: backend/app/tests/seed_data/seed_data.py:344-354
Timestamp: 2025-12-23T03:57:00.894Z
Learning: In backend/app/tests/seed_data/seed_data.py, log messages should use the file name as the prefix (e.g., `[tests.seed_data]`) rather than the function name prefix. This file-based logging prefix convention is preferred for test seed data modules.
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
Applied to files:
backend/app/seed_data/seed_data.pybackend/app/tests/seed_data/seed_data.pybackend/app/tests/conftest.py
📚 Learning: 2025-12-23T03:57:00.894Z
Learnt from: nishika26
Repo: ProjectTech4DevAI/kaapi-backend PR: 488
File: backend/app/tests/seed_data/seed_data.py:344-354
Timestamp: 2025-12-23T03:57:00.894Z
Learning: In backend/app/tests/seed_data/seed_data.py, log messages should use the file name as the prefix (e.g., `[tests.seed_data]`) rather than the function name prefix. This file-based logging prefix convention is preferred for test seed data modules.
Applied to files:
backend/app/tests/seed_data/seed_data.pybackend/app/tests/conftest.py
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Applied to files:
backend/app/tests/seed_data/seed_data.pybackend/app/tests/conftest.py
🧬 Code graph analysis (3)
backend/app/tests/seed_data/seed_data.py (6)
backend/app/core/security.py (2)
get_password_hash(100-110)encrypt_credentials(151-168)backend/app/models/api_key.py (1)
APIKey(48-89)backend/app/models/organization.py (1)
Organization(44-82)backend/app/models/project.py (1)
Project(51-107)backend/app/models/credentials.py (1)
Credential(67-131)backend/app/models/document.py (1)
Document(28-69)
backend/app/tests/utils/document.py (2)
backend/app/models/document.py (1)
Document(28-69)backend/app/models/project.py (1)
Project(51-107)
backend/app/tests/conftest.py (2)
backend/app/seed_data/seed_data.py (1)
seed_database(191-238)backend/app/tests/seed_data/seed_data.py (1)
seed_database(359-420)
🪛 Ruff (0.14.10)
backend/app/seed_data/seed_data.py
53-53: Unnecessary mode argument
Remove mode argument
(UP015)
backend/app/tests/seed_data/seed_data.py
85-85: Unnecessary mode argument
Remove mode argument
(UP015)
backend/app/tests/utils/document.py
3-3: Import from collections.abc instead: Generator
Import from collections.abc
(UP035)
backend/app/tests/conftest.py
10-10: Import from collections.abc instead: Generator
Import from collections.abc
(UP035)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (11)
backend/app/tests/utils/document.py (5)
55-81: Excellent type hint additions to DocumentStore class.All methods now have proper type annotations:
__init__andclearcorrectly typed as returningNoneprojectproperty returns concreteProjecttypeputreturnsDocumentextendproperly typed asGenerator[Document, None, None]fillreturnslist[Document]The type hints are accurate and follow Python 3.11+ conventions. As per coding guidelines.
88-111: Well-typed Route class methods.The type annotations are accurate:
__init__properly typesendpointasstr | Pathunion and usesAnyfor**qs_args__str__andto_urlreturn correct typesappenduses forward reference"Route"appropriately for self-referencing return typeAs per coding guidelines.
119-129: LGTM - WebCrawler methods properly typed.Both
getanddeletemethods correctly returnResponsetype from httpx library.
137-153: Proper type hints for DocumentComparator methods.The singledispatch pattern is correctly typed:
- Base
to_stringmethod usesAny -> Any- Overloads for
UUIDanddatetimecorrectly returnstr__init__and__eq__have accurate type annotationsAs per coding guidelines.
170-171: Fixture properly typed.The
crawlerfixture now has an explicitWebCrawlerreturn type, improving IDE support and type checking.backend/app/tests/conftest.py (1)
24-24: LGTM!The import path correctly points to the test-specific seed module, and the updated type annotations properly reflect the generator-based fixture patterns. The docstrings accurately describe the comprehensive test seed data including credentials, assistants, and documents.
Also applies to: 49-61, 65-68
backend/app/seed_data/seed_data.py (3)
1-17: LGTM!The imports correctly reflect the simplified scope, removing credentials, assistants, and documents support. Type hints are properly imported.
63-178: LGTM!The creator functions have proper type hints (
dict[str, Any]) and consistent log prefixes. The logic correctly handles organization, project, user, and API key creation with appropriate error handling.
181-244: LGTM!The simplified seed workflow correctly handles only core entities (organizations, users, projects, API keys). The docstring accurately reflects the reduced scope, and logging is consistent throughout.
backend/app/tests/seed_data/seed_data.py (2)
1-79: LGTM!The imports and data models are well-structured for the comprehensive test seed module. All necessary entities are included with proper type annotations.
346-420: LGTM!The
clear_databaseandseed_databasefunctions correctly handle the comprehensive test seed workflow, including credentials, assistants, and documents. The logic properly sequences entity creation with appropriate email placeholder substitution and error handling.
| assert len(data) >= 1 | ||
|
|
||
|
|
||
| def test_read_credentials_not_found(client: TestClient, user_api_key: TestAuthContext): |
There was a problem hiding this comment.
we can skip not_found credential status safely I think.
There was a problem hiding this comment.
I have learned that the point of test cases is to cover as many situations as possible, even if either the possibility of that situation is really low, or even if API testing will reveal it to us as well
| assert "host" in data | ||
|
|
||
|
|
||
| def test_read_provider_credential_not_found( |
There was a problem hiding this comment.
same as above. Not found credential can be tested with APIs directly to see if 404 appears. Can reduce test cases from here.
There was a problem hiding this comment.
do test cases have to be only for situations we can't test through API directly, because we have been writing test cases covering everything, errors that you can cover by testing through api directly, and errors that you can't directly test through APIs
| user_api_key: TestAuthContext, | ||
| ): | ||
| # Ensure none | ||
| """Test reading credentials for non-existent provider returns 404.""" |
|
|
||
| def test_delete_provider_credential_not_found( | ||
| client: TestClient, user_api_key: TestAuthContext | ||
| def test_credential_encryption( |
There was a problem hiding this comment.
This would be self evident. This will never break imo.
There was a problem hiding this comment.
does not hurt to cover it by test case
|
|
||
| def test_delete_all_credentials( | ||
|
|
||
| def test_update_nonexistent_provider_returns_404( |
There was a problem hiding this comment.
this test is guaranteed to give 404 I think. Test cases can be reduced
| assert data["status"] == "completed" | ||
| assert "traces" in data["score"] | ||
|
|
||
| def test_get_evaluation_run_not_found(self, client, user_api_key_header): |
There was a problem hiding this comment.
this we can test with APIs directly
There was a problem hiding this comment.
as mentioned before just because something can be tested with api does not mean we won't cover it by test cases
| assert data["langfuse_dataset_id"].startswith("langfuse") | ||
| assert data["object_store_url"].startswith("s3://test/") | ||
|
|
||
| def test_get_dataset_not_found(self, client, user_api_key_header): |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/tests/core/test_security.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/core/test_security.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/core/test_security.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
🪛 GitHub Actions: Kaapi CI
backend/app/tests/core/test_security.py
[error] end-of-file-fixer hook modified this file during pre-commit check.
🪛 Ruff (0.14.10)
backend/app/tests/core/test_security.py
195-195: No newline at end of file
Add trailing newline
(W292)
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/tests/api/routes/test_creds.py (2)
12-65: Add return type hints to test functions.Per coding guidelines, all functions should include return type annotations. Test functions should be annotated with
-> None.🔎 Proposed fix for missing return type hints
-def test_read_credentials( +def test_read_credentials( client: TestClient, user_api_key: TestAuthContext, -): +) -> None: -def test_read_provider_credential_langfuse( +def test_read_provider_credential_langfuse( client: TestClient, user_api_key: TestAuthContext, -): +) -> None: -def test_read_provider_credential_not_found( +def test_read_provider_credential_not_found( client: TestClient, user_api_key: TestAuthContext, -): +) -> None:Note: As per coding guidelines, all function parameters and return values require type hints.
68-141: Add return type hints to test functions.These test functions are missing return type annotations.
🔎 Proposed fix for missing return type hints
-def test_update_credentials( +def test_update_credentials( client: TestClient, user_api_key: TestAuthContext, -): +) -> None: -def test_create_credential( +def test_create_credential( client: TestClient, user_api_key: TestAuthContext, -): +) -> None:Note: As per coding guidelines, all function parameters and return values require type hints.
backend/app/tests/api/routes/test_evaluation.py (2)
489-551: Add type hints to test methods and fixtures.The test methods in
TestBatchEvaluationare missing type hints on parameters. Per coding guidelines, all function parameters and return values should have type hints.🔎 Proposed fix with type hints
+from typing import Any +from fastapi.testclient import TestClient @pytest.fixture -def sample_evaluation_config(self): +def sample_evaluation_config(self) -> dict[str, Any]: """Sample evaluation configuration.""" return { "model": "gpt-4o", "temperature": 0.2, "instructions": "You are a helpful assistant", } def test_start_batch_evaluation_invalid_dataset_id( - self, client, user_api_key_header, sample_evaluation_config + self, client: TestClient, user_api_key_header: dict[str, str], sample_evaluation_config: dict[str, Any] -): +) -> None: """Test batch evaluation fails with invalid/non-existent dataset_id.""" -def test_start_batch_evaluation_missing_model(self, client, user_api_key_header): +def test_start_batch_evaluation_missing_model(self, client: TestClient, user_api_key_header: dict[str, str]) -> None: """Test batch evaluation fails when model is missing from config.""" def test_start_batch_evaluation_without_authentication( - self, client, sample_evaluation_config + self, client: TestClient, sample_evaluation_config: dict[str, Any] -): +) -> None: """Test batch evaluation requires authentication."""As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"
698-856: Add type hints to fixture and test methods.The
create_test_datasetfixture and all test methods inTestGetEvaluationRunStatusare missing type hints on parameters and return values.🔎 Proposed fix with type hints
+from sqlmodel import Session +from fastapi.testclient import TestClient +from app.models import EvaluationDataset @pytest.fixture -def create_test_dataset(self, db, user_api_key): +def create_test_dataset(self, db: Session, user_api_key: Any) -> EvaluationDataset: """Create a test dataset for evaluation runs.""" return create_test_evaluation_dataset( db=db, organization_id=user_api_key.organization_id, project_id=user_api_key.project_id, name="test_dataset_for_runs", description="Test dataset", original_items_count=3, duplication_factor=1, ) def test_get_evaluation_run_trace_info_not_completed( - self, client, user_api_key_header, db, user_api_key, create_test_dataset + self, client: TestClient, user_api_key_header: dict[str, str], db: Session, user_api_key: Any, create_test_dataset: EvaluationDataset -): +) -> None: def test_get_evaluation_run_trace_info_completed( - self, client, user_api_key_header, db, user_api_key, create_test_dataset + self, client: TestClient, user_api_key_header: dict[str, str], db: Session, user_api_key: Any, create_test_dataset: EvaluationDataset -): +) -> None: -def test_get_evaluation_run_not_found(self, client, user_api_key_header): +def test_get_evaluation_run_not_found(self, client: TestClient, user_api_key_header: dict[str, str]) -> None: def test_get_evaluation_run_without_trace_info( - self, client, user_api_key_header, db, user_api_key, create_test_dataset + self, client: TestClient, user_api_key_header: dict[str, str], db: Session, user_api_key: Any, create_test_dataset: EvaluationDataset -): +) -> None: def test_get_evaluation_run_resync_without_trace_info_fails( - self, client, user_api_key_header, db, user_api_key, create_test_dataset + self, client: TestClient, user_api_key_header: dict[str, str], db: Session, user_api_key: Any, create_test_dataset: EvaluationDataset -): +) -> None:As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"
♻️ Duplicate comments (1)
backend/app/tests/core/test_security.py (1)
192-192: Add trailing newline at end of file.The file is missing a trailing newline, which causes the
end-of-file-fixerpre-commit hook to fail. This is a required formatting convention for Python files.🔎 Proposed fix
assert auth_context.user.id == api_key_response.user_id +
🧹 Nitpick comments (2)
backend/app/tests/api/test_auth_failures.py (1)
30-98: Consider reducing code duplication.All three test functions share nearly identical logic, differing only in the authentication headers. Consider extracting the common request logic into a helper function or using pytest's
@pytest.mark.parametrizedecorator.🔎 Example refactor using a helper function
def _test_endpoint_with_auth( client: TestClient, headers: dict[str, str] | None, expected_message: str ) -> None: """Helper to test all endpoints with given auth headers.""" for endpoint, method in PROTECTED_ENDPOINTS: if method == "GET": response = client.get(endpoint, headers=headers) elif method == "POST": response = client.post(endpoint, json={"name": "test"}, headers=headers) elif method == "DELETE": response = client.delete(endpoint, headers=headers) elif method == "PUT": response = client.put(endpoint, json={"name": "test"}, headers=headers) elif method == "PATCH": response = client.patch(endpoint, json={"name": "test"}, headers=headers) assert response.status_code == 401, ( f"Expected 401 for {method} {endpoint} {expected_message}, " f"got {response.status_code}" ) def test_all_endpoints_reject_missing_auth_header(client: TestClient) -> None: """Test that all protected endpoints return 401 when no auth header is provided.""" _test_endpoint_with_auth(client, None, "without auth") def test_all_endpoints_reject_invalid_auth_format(client: TestClient) -> None: """Test that all protected endpoints return 401 when auth header has invalid format.""" _test_endpoint_with_auth( client, {"Authorization": "InvalidFormat"}, "with invalid format" ) def test_all_endpoints_reject_nonexistent_api_key(client: TestClient) -> None: """Test that all protected endpoints return 401 when API key doesn't exist.""" _test_endpoint_with_auth( client, {"Authorization": "ApiKey FakeKeyThatDoesNotExist123456789"}, "with fake key" )backend/app/tests/core/test_security.py (1)
11-192: Consider adding return type hints to test functions.All test functions in this file lack return type hints. While not critical for test code, adding
-> Noneto each test function would improve type safety and align with the coding guidelines for Python files.Example
-def test_get_encryption_key(): +def test_get_encryption_key() -> None: """Test that encryption key generation works correctly."""This pattern would apply to all test methods in the file.
Note: This is pre-existing technical debt, not introduced by this PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/app/tests/api/routes/configs/test_config.pybackend/app/tests/api/routes/configs/test_version.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/api/routes/test_cron.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/core/test_security.py
💤 Files with no reviewable changes (2)
- backend/app/tests/api/routes/configs/test_version.py
- backend/app/tests/api/routes/configs/test_config.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/api/routes/test_cron.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/core/test_security.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/test_cron.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/core/test_security.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
🧬 Code graph analysis (2)
backend/app/tests/api/routes/test_cron.py (3)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)backend/app/tests/conftest.py (2)
superuser_api_key(96-98)user_api_key(102-104)backend/app/tests/utils/document.py (1)
get(119-123)
backend/app/tests/api/routes/test_evaluation.py (1)
backend/app/tests/utils/test_data.py (1)
create_test_evaluation_dataset(341-376)
🪛 Ruff (0.14.10)
backend/app/tests/api/routes/test_creds.py
153-153: Avoid equality comparisons to True; use Credential.is_active: for truth checks
Replace with Credential.is_active
(E712)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (5)
backend/app/tests/api/routes/test_cron.py (1)
1-150: Well-structured test coverage for the cron endpoint.The test suite provides comprehensive coverage with appropriate scenarios:
- Success and failure cases with proper assertion validation
- Authentication and authorization boundaries (superuser requirement)
- OpenAPI schema exclusion verification
The mock usage properly isolates endpoint behavior, and the assertions validate all relevant response fields.
backend/app/tests/api/routes/test_creds.py (1)
1-9: LGTM!Imports are well-organized and support the test functionality. The addition of
selectfrom sqlmodel enables database query operations in the encryption test.backend/app/tests/api/routes/test_evaluation.py (2)
9-9: LGTM: Factory pattern import.The import of
create_test_evaluation_datasetaligns with the factory pattern guideline for test fixtures and supports the refactoring toward shared test-data utilities.
780-978: LGTM: Comprehensive test coverage for evaluation endpoints.The new test methods provide thorough coverage of the evaluation run and dataset endpoints, including:
- Success paths with proper response validation
- 404 cases for non-existent resources
- 401 authentication failures
- Verification of side effects (e.g., confirming deletion with follow-up GET)
The tests properly handle flexible error response formats and use the factory pattern for test data creation.
backend/app/tests/core/test_security.py (1)
1-9: Import cleanup is correct and verified—no further action needed.The removed imports (
pytest,uuid4,get_password_hash,verify_password) are confirmed unused in this file. Thedbfixture parameter works without explicitpytestimport because pytest is already imported inconftest.py, which is the standard pattern for pytest fixture injection. The file also properly ends with a newline.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/tests/api/routes/test_evaluation.py (1)
474-493: Add type hints to test method parameters and return value.The test method is missing type hints on all parameters and the return value.
As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"🔎 Proposed fix with type hints
+from typing import Any +from fastapi.testclient import TestClient + def test_start_batch_evaluation_invalid_dataset_id( - self, client, user_api_key_header, sample_evaluation_config -): + self, client: TestClient, user_api_key_header: dict[str, str], sample_evaluation_config: dict[str, Any] +) -> None: """Test batch evaluation fails with invalid/non-existent dataset_id."""
♻️ Duplicate comments (7)
backend/app/tests/api/test_auth_failures.py (4)
7-38: Add type annotation to the module-level constant.As per coding guidelines, type hints should be used throughout the codebase. The
PROTECTED_ENDPOINTSconstant should be annotated withlist[tuple[str, str]].🔎 Proposed fix
-PROTECTED_ENDPOINTS = [ +PROTECTED_ENDPOINTS: list[tuple[str, str]] = [ (f"{settings.API_V1_STR}/collections/", "GET"),
41-52: Add return type annotation.As per coding guidelines, all function parameters and return values should have type hints. The test function should be annotated with
-> None.🔎 Proposed fix
@pytest.mark.parametrize("endpoint,method", PROTECTED_ENDPOINTS) -def test_endpoints_reject_missing_auth_header( +def test_endpoints_reject_missing_auth_header( client: TestClient, endpoint: str, method: str -): +) -> None:
54-67: Add return type annotation.As per coding guidelines, all function parameters and return values should have type hints. The test function should be annotated with
-> None.🔎 Proposed fix
@pytest.mark.parametrize("endpoint,method", PROTECTED_ENDPOINTS) -def test_endpoints_reject_invalid_auth_format( +def test_endpoints_reject_invalid_auth_format( client: TestClient, endpoint: str, method: str -): +) -> None:
69-84: Add return type annotation.As per coding guidelines, all function parameters and return values should have type hints. The test function should be annotated with
-> None.🔎 Proposed fix
@pytest.mark.parametrize("endpoint,method", PROTECTED_ENDPOINTS) -def test_endpoints_reject_nonexistent_api_key( +def test_endpoints_reject_nonexistent_api_key( client: TestClient, endpoint: str, method: str -): +) -> None:backend/app/tests/api/routes/test_evaluation.py (3)
829-880: Add type hints to TestGetDataset fixture and test methods.The
TestGetDatasetclass has missing type hints on the fixture and all test methods, including:
create_test_datasetfixture (lines 833-843)test_get_dataset_success(lines 845-865)test_get_dataset_not_found(lines 867-879)As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"
882-934: Add type hints to TestDeleteDataset fixture and test methods.The
TestDeleteDatasetclass has missing type hints on the fixture and all test methods, including:
create_test_datasetfixture (lines 886-896)test_delete_dataset_success(lines 898-920)test_delete_dataset_not_found(lines 922-933)As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"
669-679: Add type hints to fixture parameters and return value.The fixture is missing type hints on all parameters and the return value.
As per coding guidelines for
**/*.py: "Always add type hints to all function parameters and return values in Python code"🔎 Proposed fix with type hints
+from typing import Any +from sqlmodel import Session +from app.models import EvaluationDataset + @pytest.fixture -def create_test_dataset(self, db, user_api_key): +def create_test_dataset(self, db: Session, user_api_key: Any) -> EvaluationDataset: """Create a test dataset for evaluation runs.""" return create_test_evaluation_dataset( db=db, organization_id=user_api_key.organization_id, project_id=user_api_key.project_id, name="test_dataset_for_runs", description="Test dataset", original_items_count=3, duplication_factor=1, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/tests/api/routes/test_cron.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/routes/test_openai_conversation.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/services/collections/providers/openai.pybackend/app/tests/services/collections/providers/registry.py
💤 Files with no reviewable changes (1)
- backend/app/tests/api/routes/test_openai_conversation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/tests/api/routes/test_cron.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/test_auth_failures.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/test_auth_failures.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (4)
backend/app/tests/api/test_auth_failures.py (2)
1-4: LGTM!The imports are clean, minimal, and follow standard Python conventions.
41-84: Excellent test coverage and structure.The test suite comprehensively covers authentication failure scenarios across all protected endpoints:
- Missing authentication header
- Invalid authorization format
- Non-existent API key
The use of
pytest.mark.parametrizewith descriptive assertion messages makes these tests maintainable and easy to debug. The conditional kwargs handling for POST/PATCH methods is a clean approach.backend/app/tests/api/routes/test_evaluation.py (2)
9-9: LGTM: Factory pattern helper import.The import of
create_test_evaluation_datasetaligns with the factory pattern requirement for test fixtures.
204-216: LGTM: Clarified default duplication behavior.The comment and assertion correctly reflect that when
duplication_factoris not provided, it defaults to 1, resulting intotal_itemsequalingoriginal_items.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
backend/app/tests/services/collections/test_helpers.py (2)
42-53: Add type hints toFakeDocumentCrudhelper class methods.The
FakeDocumentCrudclass methods are missing type hints, which is inconsistent with the type hint additions made to the test functions and violates the coding guideline requiring type hints on all function parameters and return values.🔎 Proposed type hints for FakeDocumentCrud
+from typing import Any + class FakeDocumentCrud: - def __init__(self): + def __init__(self) -> None: self.calls = [] - def read_each(self, ids): + def read_each(self, ids: list[Any]) -> list[SimpleNamespace]: self.calls.append(list(ids)) return [ SimpleNamespace( id=i, fname=f"{i}.txt", object_store_url=f"s3://bucket/{i}.txt" ) for i in ids ]As per coding guidelines, all function parameters and return values should have type hints.
91-109: Add type hints to nested test helper classes.The nested
CrudandFailingCrudclasses lack type hints on their methods, which is inconsistent with the type hint additions throughout this file.🔎 Proposed type hints for nested classes
class Crud: - def __init__(self): + def __init__(self) -> None: self.calls = 0 - def delete(self, resource_id: str): + def delete(self, resource_id: str) -> None: self.calls += 1 crud = Crud() helpers._backout(crud, "rsrc_1") assert crud.calls == 1 class DummyOpenAIError(Exception): pass monkeypatch.setattr(helpers, "OpenAIError", DummyOpenAIError) class FailingCrud: - def delete(self, resource_id: str): + def delete(self, resource_id: str) -> None: raise DummyOpenAIError("nope")As per coding guidelines, all function parameters and return values should have type hints.
backend/app/tests/crud/test_openai_conversation.py (1)
686-690: Fix the project retrieval logic.The conditional logic is flawed because
get_projectraises aValueErrorwhen no project is found (never returningNone), so theis not Nonecheck will never be evaluated if "Dalgo" doesn't exist. Additionally, if "Dalgo" exists,get_project(db, "Dalgo")is called twice unnecessarily.🔎 Proposed fix using try-except
- # Get another project (assuming there are multiple projects in test data) - project2 = ( - get_project(db, "Dalgo") - if get_project(db, "Dalgo") is not None - else get_project(db) - ) + # Get another project (assuming there are multiple projects in test data) + try: + project2 = get_project(db, "Dalgo") + except ValueError: + # If "Dalgo" doesn't exist, create a new test project + project2 = Project( + name=f"test_project_{uuid4()}", + description="Test project for count isolation", + is_active=True, + organization_id=organization.id, + ) + db.add(project2) + db.commit() + db.refresh(project2)backend/app/tests/services/collections/test_delete_collection.py (3)
77-80: Add missing type hints for consistency with PR objectives.This function is missing type hints on both parameters and the return type. Since the PR objectives mention "adding type hints" and the coding guidelines require type hints on all function parameters and return values, this function should be updated for consistency.
🔎 Proposed fix
@patch("app.services.collections.delete_collection.get_openai_client") def test_execute_job_delete_success_updates_job_and_calls_delete( - mock_get_openai_client, db -): + mock_get_openai_client: Any, db: Session +) -> None:As per coding guidelines, all function parameters and return values must have type hints.
212-216: Add missing type hints for consistency with PR objectives.This function is missing type hints on both parameters and the return type.
🔎 Proposed fix
@patch("app.services.collections.delete_collection.get_openai_client") def test_execute_job_delete_success_with_callback_sends_success_payload( - mock_get_openai_client, - db, -): + mock_get_openai_client: Any, + db: Session, +) -> None:As per coding guidelines, all function parameters and return values must have type hints.
292-296: Add missing type hints for consistency with PR objectives.This function is missing type hints on both parameters and the return type.
🔎 Proposed fix
@patch("app.services.collections.delete_collection.get_openai_client") def test_execute_job_delete_remote_failure_with_callback_sends_failure_payload( - mock_get_openai_client, - db, -): + mock_get_openai_client: Any, + db: Session, +) -> None:As per coding guidelines, all function parameters and return values must have type hints.
backend/app/tests/api/routes/test_openai_conversation.py (1)
1-8: Add openai-conversation endpoints to centralized authorization tests.The openai-conversation endpoints lack coverage in the centralized auth test suite. The
backend/app/tests/api/test_auth_failures.pyfile tests missing auth headers and invalid auth formats across protected endpoints, but the/api/v1/openai-conversation/*routes are not included in thePROTECTED_ENDPOINTSlist.Add the following endpoints to the
PROTECTED_ENDPOINTSlist inbackend/app/tests/api/test_auth_failures.py:
GET /api/v1/openai-conversationGET /api/v1/openai-conversation/{id}GET /api/v1/openai-conversation/response/{response_id}GET /api/v1/openai-conversation/ancestor/{ancestor_id}DELETE /api/v1/openai-conversation/{id}This ensures that unauthorized access attempts (missing API key, invalid API key, invalid format) are tested consistently across all protected routes.
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (1)
12-26: Incorrect return type annotation: returnsint, notCollection.The function is annotated with return type
-> Collectionon line 12, but it actually returnscrud.project_idon line 26, which is anint(specifically, a project ID). This type mismatch will cause type checking failures.🔎 Proposed fix
-def create_collections(db: Session, n: int) -> Collection: +def create_collections(db: Session, n: int) -> int: crud = None project = get_project(db) openai_mock = OpenAIMock() with openai_mock.router: client = OpenAI(api_key="sk-test-key") for _ in range(n): collection = get_collection(db, project=project) store = DocumentStore(db, project_id=collection.project_id) documents = store.fill(1) if crud is None: crud = CollectionCrud(db, collection.project_id) crud.create(collection, documents) return crud.project_idAs per coding guidelines, type hints must be accurate for all function return values.
♻️ Duplicate comments (1)
backend/app/tests/api/test_auth_failures.py (1)
7-38: Add type annotation to the module-level constant.The
PROTECTED_ENDPOINTSconstant is missing a type annotation.As per coding guidelines, type hints should be used throughout the codebase.
🔎 Proposed fix
-PROTECTED_ENDPOINTS = [ +PROTECTED_ENDPOINTS: list[tuple[str, str]] = [ # Collections (f"{settings.API_V1_STR}/collections/", "GET"),
🧹 Nitpick comments (18)
backend/app/tests/api/routes/collections/test_collection_job_info.py (1)
1-1: Consider removing unused import.The
Anyimport fromtypingis not used anywhere in this file.🔎 Proposed fix
-from typing import Any - from fastapi.testclient import TestClientbackend/app/tests/api/routes/test_project.py (1)
53-53: Type hint added correctly; consider removing unused parameter.The
-> Nonereturn type correctly follows the coding guidelines. However, thedbparameter appears unused in the function body—you may consider removing it for cleaner test signatures.🔎 Optional cleanup
-def test_read_projects(db: Session, superuser_token_headers: dict[str, str]) -> None: +def test_read_projects(superuser_token_headers: dict[str, str]) -> None:Based on static analysis hints.
backend/app/tests/api/routes/test_org.py (1)
42-44: Remove unuseddbparameter.The
dbparameter is not used in this test function. Consider removing it to improve code clarity.🔎 Proposed fix
-def test_read_organizations( - db: Session, superuser_token_headers: dict[str, str] -) -> None: +def test_read_organizations(superuser_token_headers: dict[str, str]) -> None:backend/app/tests/api/routes/test_doc_transformation_job.py (1)
40-40: Consider extracting the fake UUID as a module-level constant.The UUID string
"00000000-0000-0000-0000-000000000001"is used in multiple test methods (lines 40 and 180). While this works fine, extracting it as a module-level constant would improve maintainability and make it easier to update if needed.🔎 Proposed refactor
Add at the top of the file after imports:
+# Test constants +FAKE_UUID = "00000000-0000-0000-0000-000000000001" +Then update the usage:
- fake_uuid = "00000000-0000-0000-0000-000000000001" + fake_uuid = FAKE_UUIDAlso applies to: 180-180
backend/app/tests/crud/test_assistants.py (1)
136-138: Consider adding type hints to mock parameters.While return type annotations have been consistently added, the mock parameters injected by
@patchdecorators still lack type hints. AddingMagicMocktype hints would fully comply with the coding guideline requiring type hints on all function parameters.As per coding guidelines, type hints should be added to all parameters.
💡 Example refactor for adding mock parameter type hints
+from unittest.mock import patch, MagicMock -from unittest.mock import patch @patch("app.crud.assistants.verify_vector_store_ids_exist") def test_create_assistant_success( - self, mock_vector_store_ids_exist, db: Session + self, mock_vector_store_ids_exist: MagicMock, db: Session ) -> None:Apply similar changes to all test methods with
@patchdecorators (lines 164, 193, 232, 260, 301, 352, 428).Also applies to: 163-165, 192-194, 231-233, 259-261, 300-302, 351-353, 427-429
backend/app/tests/crud/documents/test_doc_transformation_job.py (1)
18-20: Consider extracting duplicated fixture to conftest.py.The
storefixture here is identical to the one intest_crud_document_read_one.py. Consider extracting this common fixture toconftest.pyto reduce duplication and make it available across multiple test modules. However, note thattest_crud_document_read_many.pyhas a specialized version that pre-fills documents, so you may want to provide both a base fixture and a specialized one.As per coding guidelines, consider using a factory pattern for test fixtures.
backend/app/tests/crud/documents/documents/test_crud_document_delete.py (1)
12-22: Consider renaming fixture for clarity.The fixture is named
documentbut specifically returns a soft-deleted document. Consider renaming todeleted_documentto make the fixture's purpose more explicit, especially if this fixture is used in other test files.🔎 Proposed rename
@pytest.fixture -def document(db: Session) -> Document: +def deleted_document(db: Session) -> Document: project = get_project(db) store = DocumentStore(db, project.id) document = store.put()And update all test method signatures:
- def test_delete_is_soft(self, document: Document) -> None: + def test_delete_is_soft(self, deleted_document: Document) -> None:backend/app/tests/crud/evaluations/test_processing.py (9)
297-361: Add type hints to mock parameters.The mock parameters (
mock_upload,mock_start_embedding, etc.) lack type hints. While they are mocks, adding type hints improves code clarity.🔎 Proposed fix
+ from unittest.mock import MagicMock + @pytest.mark.asyncio @patch("app.crud.evaluations.processing.download_batch_results") @patch("app.crud.evaluations.processing.fetch_dataset_items") @patch("app.crud.evaluations.processing.create_langfuse_dataset_run") @patch("app.crud.evaluations.processing.start_embedding_batch") @patch("app.crud.evaluations.processing.upload_batch_results_to_object_store") async def test_process_completed_evaluation_success( self, - mock_upload, - mock_start_embedding, - mock_create_langfuse, - mock_fetch_dataset, - mock_download, + mock_upload: MagicMock, + mock_start_embedding: MagicMock, + mock_create_langfuse: MagicMock, + mock_fetch_dataset: MagicMock, + mock_download: MagicMock, db: Session, - eval_run_with_batch, - ): + eval_run_with_batch: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
362-394: Add type hints to parameters.Parameters lack type hints for mocks and fixtures.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.download_batch_results") @patch("app.crud.evaluations.processing.fetch_dataset_items") async def test_process_completed_evaluation_no_results( self, - mock_fetch_dataset, - mock_download, + mock_fetch_dataset: MagicMock, + mock_download: MagicMock, db: Session, - eval_run_with_batch, - ): + eval_run_with_batch: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
396-423: Add type hints to parameters.Parameters
test_datasetlacks a type hint.🔎 Proposed fix
@pytest.mark.asyncio async def test_process_completed_evaluation_no_batch_job_id( - self, db: Session, test_dataset - ): + self, db: Session, test_dataset: Any + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
485-533: Add type hints to parameters.Mock and fixture parameters lack type hints.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.download_batch_results") @patch("app.crud.evaluations.processing.parse_embedding_results") @patch("app.crud.evaluations.processing.calculate_average_similarity") @patch("app.crud.evaluations.processing.update_traces_with_cosine_scores") async def test_process_completed_embedding_batch_success( self, - mock_update_traces, - mock_calculate, - mock_parse, - mock_download, + mock_update_traces: MagicMock, + mock_calculate: MagicMock, + mock_parse: MagicMock, + mock_download: MagicMock, db: Session, - eval_run_with_embedding_batch, - ): + eval_run_with_embedding_batch: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
535-561: Add type hints to parameters.Mock and fixture parameters lack type hints.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.download_batch_results") @patch("app.crud.evaluations.processing.parse_embedding_results") async def test_process_completed_embedding_batch_no_results( self, - mock_parse, - mock_download, + mock_parse: MagicMock, + mock_download: MagicMock, db: Session, - eval_run_with_embedding_batch, - ): + eval_run_with_embedding_batch: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
585-646: Add type hints to parameters.Mock and fixture parameters lack type hints.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.get_batch_job") @patch("app.crud.batch_operations.poll_batch_status") @patch("app.crud.evaluations.processing.process_completed_evaluation") async def test_check_and_process_evaluation_completed( self, - mock_process, - mock_poll, - mock_get_batch, + mock_process: MagicMock, + mock_poll: MagicMock, + mock_get_batch: MagicMock, db: Session, - test_dataset, - ): + test_dataset: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
648-708: Add type hints to parameters.Mock and fixture parameters lack type hints.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.get_batch_job") @patch("app.crud.batch_operations.poll_batch_status") async def test_check_and_process_evaluation_failed( self, - mock_poll, - mock_get_batch, + mock_poll: MagicMock, + mock_get_batch: MagicMock, db: Session, - test_dataset, - ): + test_dataset: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
732-744: Add type hint to parameter.The
test_datasetparameter lacks a type hint.🔎 Proposed fix
@pytest.mark.asyncio async def test_poll_all_pending_evaluations_no_pending( - self, db: Session, test_dataset - ): + self, db: Session, test_dataset: Any + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
746-806: Add type hints to parameters.Mock and fixture parameters lack type hints.
🔎 Proposed fix
@pytest.mark.asyncio @patch("app.crud.evaluations.processing.check_and_process_evaluation") @patch("app.crud.evaluations.processing.get_openai_client") @patch("app.crud.evaluations.processing.get_langfuse_client") async def test_poll_all_pending_evaluations_with_runs( self, - mock_langfuse_client, - mock_openai_client, - mock_check, + mock_langfuse_client: MagicMock, + mock_openai_client: MagicMock, + mock_check: MagicMock, db: Session, - test_dataset, - ): + test_dataset: Any, + ) -> None:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
backend/app/tests/crud/evaluations/test_dataset.py (1)
388-486: Consider a factory fixture for dataset creation to reduce duplication.The pattern of creating test datasets (fetch org → fetch project → create_evaluation_dataset) is repeated across three test methods. Per the coding guidelines, consider extracting this into a factory fixture to improve maintainability.
💡 Example factory fixture approach
@pytest.fixture def dataset_factory(db: Session) -> Callable[[str, int], EvaluationDataset]: """Factory fixture for creating test datasets.""" def _create_dataset( name: str, original_items_count: int = 5 ) -> EvaluationDataset: org = db.exec(select(Organization)).first() project = db.exec( select(Project).where(Project.organization_id == org.id) ).first() return create_evaluation_dataset( session=db, name=name, dataset_metadata={"original_items_count": original_items_count}, organization_id=org.id, project_id=project.id, ) return _create_datasetThen use it in tests:
def test_delete_dataset_success(self, db: Session, dataset_factory) -> None: dataset = dataset_factory("dataset_to_delete", 5) # ... rest of testbackend/app/tests/crud/collections/collection_job/test_collection_jobs.py (1)
1-1: Remove unused import.The
Anytype is imported fromtypingbut never used in this file.🔎 Proposed fix
-from typing import Any from uuid import uuid4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
backend/app/tests/api/routes/collections/test_collection_job_info.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/api/routes/test_cron.pybackend/app/tests/api/routes/test_doc_transformation_job.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/api/routes/test_llm.pybackend/app/tests/api/routes/test_openai_conversation.pybackend/app/tests/api/routes/test_org.pybackend/app/tests/api/routes/test_project.pybackend/app/tests/api/routes/test_responses.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/api/test_deps.pybackend/app/tests/api/test_permissions.pybackend/app/tests/crud/collections/collection/test_crud_collection_create.pybackend/app/tests/crud/collections/collection/test_crud_collection_delete.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_one.pybackend/app/tests/crud/collections/collection_job/test_collection_jobs.pybackend/app/tests/crud/documents/documents/test_crud_document_delete.pybackend/app/tests/crud/documents/documents/test_crud_document_read_many.pybackend/app/tests/crud/documents/documents/test_crud_document_read_one.pybackend/app/tests/crud/documents/documents/test_crud_document_update.pybackend/app/tests/crud/documents/test_doc_transformation_job.pybackend/app/tests/crud/evaluations/test_dataset.pybackend/app/tests/crud/evaluations/test_embeddings.pybackend/app/tests/crud/evaluations/test_langfuse.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/crud/test_assistants.pybackend/app/tests/crud/test_credentials.pybackend/app/tests/crud/test_fine_tuning.pybackend/app/tests/crud/test_jobs.pybackend/app/tests/crud/test_model_evaluation.pybackend/app/tests/crud/test_openai_conversation.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/services/collections/test_helpers.py
💤 Files with no reviewable changes (1)
- backend/app/tests/crud/test_credentials.py
✅ Files skipped from review due to trivial changes (2)
- backend/app/tests/crud/collections/collection/test_crud_collection_create.py
- backend/app/tests/crud/test_model_evaluation.py
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/app/tests/api/routes/test_cron.py
- backend/app/tests/crud/collections/collection/test_crud_collection_delete.py
- backend/app/tests/api/routes/test_responses.py
- backend/app/tests/api/test_deps.py
- backend/app/tests/crud/evaluations/test_langfuse.py
- backend/app/tests/crud/documents/documents/test_crud_document_update.py
- backend/app/tests/crud/test_jobs.py
- backend/app/tests/services/collections/test_create_collection.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/api/routes/test_llm.pybackend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/crud/evaluations/test_dataset.pybackend/app/tests/api/routes/test_org.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/api/routes/collections/test_collection_job_info.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_one.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/crud/documents/documents/test_crud_document_delete.pybackend/app/tests/crud/test_fine_tuning.pybackend/app/tests/api/routes/test_openai_conversation.pybackend/app/tests/crud/test_assistants.pybackend/app/tests/api/routes/test_doc_transformation_job.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/documents/test_doc_transformation_job.pybackend/app/tests/api/routes/test_project.pybackend/app/tests/crud/test_openai_conversation.pybackend/app/tests/api/test_permissions.pybackend/app/tests/services/collections/test_helpers.pybackend/app/tests/crud/documents/documents/test_crud_document_read_many.pybackend/app/tests/crud/collections/collection_job/test_collection_jobs.pybackend/app/tests/crud/evaluations/test_embeddings.pybackend/app/tests/crud/documents/documents/test_crud_document_read_one.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/api/routes/test_creds.pybackend/app/tests/api/routes/test_llm.pybackend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/crud/evaluations/test_dataset.pybackend/app/tests/api/routes/test_org.pybackend/app/tests/api/test_auth_failures.pybackend/app/tests/api/routes/collections/test_collection_job_info.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_one.pybackend/app/tests/api/routes/test_evaluation.pybackend/app/tests/crud/documents/documents/test_crud_document_delete.pybackend/app/tests/crud/test_fine_tuning.pybackend/app/tests/api/routes/test_openai_conversation.pybackend/app/tests/crud/test_assistants.pybackend/app/tests/api/routes/test_doc_transformation_job.pybackend/app/tests/crud/collections/collection/test_crud_collection_read_all.pybackend/app/tests/crud/documents/test_doc_transformation_job.pybackend/app/tests/api/routes/test_project.pybackend/app/tests/crud/test_openai_conversation.pybackend/app/tests/api/test_permissions.pybackend/app/tests/services/collections/test_helpers.pybackend/app/tests/crud/documents/documents/test_crud_document_read_many.pybackend/app/tests/crud/collections/collection_job/test_collection_jobs.pybackend/app/tests/crud/evaluations/test_embeddings.pybackend/app/tests/crud/documents/documents/test_crud_document_read_one.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to **/*.py : Always add type hints to all function parameters and return values in Python code
Applied to files:
backend/app/tests/api/routes/test_evaluation.py
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to **/*.py : Use Python 3.11+ with type hints throughout the codebase
Applied to files:
backend/app/tests/api/routes/test_evaluation.py
🧬 Code graph analysis (20)
backend/app/tests/api/routes/test_creds.py (5)
backend/app/core/providers.py (1)
Provider(9-14)backend/app/models/credentials.py (1)
Credential(67-131)backend/app/core/security.py (1)
decrypt_credentials(133-150)backend/app/tests/utils/utils.py (1)
generate_random_string(21-22)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)
backend/app/tests/api/routes/test_llm.py (1)
backend/app/tests/conftest.py (1)
user_api_key_header(90-92)
backend/app/tests/services/collections/test_delete_collection.py (2)
backend/app/tests/services/collections/test_create_collection.py (1)
test_start_job_creates_collection_job_and_schedules_task(51-113)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/crud/evaluations/test_dataset.py (6)
backend/app/models/evaluation.py (1)
EvaluationRun(170-319)backend/app/api/routes/evaluation.py (1)
delete_dataset(404-432)backend/app/crud/evaluations/dataset.py (4)
delete_dataset(323-387)update_dataset_langfuse_id(294-320)create_evaluation_dataset(31-104)get_dataset_by_id(107-140)backend/app/tests/api/routes/test_evaluation.py (3)
TestDeleteDataset(952-1012)test_delete_dataset_success(970-996)test_delete_dataset_not_found(998-1012)backend/app/models/organization.py (1)
Organization(44-82)backend/app/models/project.py (1)
Project(51-107)
backend/app/tests/api/routes/test_org.py (2)
backend/app/tests/crud/test_org.py (1)
test_create_organization(9-17)backend/app/tests/conftest.py (2)
db(28-45)superuser_token_headers(72-73)
backend/app/tests/api/routes/collections/test_collection_job_info.py (1)
backend/app/tests/conftest.py (2)
db(28-45)user_api_key_header(90-92)
backend/app/tests/crud/collections/collection/test_crud_collection_read_one.py (2)
backend/app/models/collection.py (1)
Collection(15-64)backend/app/crud/collection/collection.py (2)
CollectionCrud(18-145)read_one(59-83)
backend/app/tests/api/routes/test_evaluation.py (6)
backend/app/crud/evaluations/batch.py (1)
build_evaluation_jsonl(62-115)backend/app/models/evaluation.py (2)
EvaluationDataset(73-167)EvaluationRun(170-319)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)backend/app/tests/utils/test_data.py (1)
create_test_evaluation_dataset(341-376)backend/app/tests/conftest.py (4)
client(65-68)user_api_key_header(90-92)db(28-45)user_api_key(102-104)backend/app/tests/crud/evaluations/test_dataset.py (3)
TestDeleteDataset(385-519)test_delete_dataset_success(388-420)test_delete_dataset_not_found(422-437)
backend/app/tests/crud/documents/documents/test_crud_document_delete.py (2)
backend/app/tests/conftest.py (1)
db(28-45)backend/app/models/document.py (1)
Document(28-69)
backend/app/tests/crud/test_fine_tuning.py (2)
backend/app/crud/fine_tuning.py (2)
fetch_by_provider_job_id(69-85)fetch_by_id(88-104)backend/app/tests/utils/test_data.py (1)
create_test_fine_tuning_jobs(166-193)
backend/app/tests/crud/test_assistants.py (2)
backend/app/tests/conftest.py (1)
db(28-45)backend/app/tests/api/routes/test_assistants.py (3)
test_create_assistant_success(54-89)test_update_assistant_success(119-148)test_update_assistant_not_found(178-195)
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (2)
backend/app/crud/collection/collection.py (3)
delete(103-111)CollectionCrud(18-145)read_all(85-94)backend/app/models/collection.py (1)
Collection(15-64)
backend/app/tests/crud/documents/test_doc_transformation_job.py (5)
backend/app/tests/crud/documents/documents/test_crud_document_read_many.py (1)
store(10-15)backend/app/tests/crud/documents/documents/test_crud_document_read_one.py (1)
store(12-14)backend/app/tests/utils/document.py (2)
DocumentStore(54-81)project(66-67)backend/app/tests/utils/utils.py (1)
get_project(51-70)backend/app/crud/document/doc_transformation_job.py (2)
DocTransformationJobCrud(21-95)read_each(57-71)
backend/app/tests/api/routes/test_project.py (1)
backend/app/tests/conftest.py (2)
db(28-45)superuser_token_headers(72-73)
backend/app/tests/crud/test_openai_conversation.py (5)
backend/app/models/openai_conversation.py (1)
OpenAIConversationCreate(124-150)backend/app/models/project.py (1)
Project(51-107)backend/app/tests/utils/utils.py (2)
get_project(51-70)get_organization(123-144)backend/app/tests/utils/openai.py (1)
generate_openai_id(14-19)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/api/test_permissions.py (1)
backend/app/api/permissions.py (3)
permission_checker(55-68)require_permission(45-70)Permission(10-15)
backend/app/tests/services/collections/test_helpers.py (1)
backend/app/services/collections/helpers.py (1)
extract_error_message(21-47)
backend/app/tests/crud/documents/documents/test_crud_document_read_many.py (2)
backend/app/tests/utils/document.py (2)
DocumentStore(54-81)project(66-67)backend/app/crud/document/document.py (2)
DocumentCrud(13-134)read_many(36-70)
backend/app/tests/crud/evaluations/test_embeddings.py (1)
backend/app/crud/evaluations/embeddings.py (1)
calculate_cosine_similarity(221-252)
backend/app/tests/crud/documents/documents/test_crud_document_read_one.py (3)
backend/app/tests/utils/document.py (3)
DocumentStore(54-81)project(66-67)put(69-74)backend/app/tests/utils/utils.py (1)
get_project(51-70)backend/app/crud/document/document.py (2)
DocumentCrud(13-134)read_one(18-34)
🪛 Ruff (0.14.10)
backend/app/tests/api/routes/test_org.py
43-43: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/test_project.py
53-53: Unused function argument: db
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (48)
backend/app/tests/api/routes/collections/test_collection_job_info.py (1)
15-17: Excellent type hint additions!The type annotations for all five test functions are accurate and consistent:
client: TestClientcorrectly types the FastAPI test clientuser_api_key_header: dict[str, str]matches the fixture return type fromconftest.py-> Noneproperly indicates test functions return nothingThese changes align perfectly with the coding guideline to add type hints throughout the codebase and are consistent with the broader typing improvements across the test suite.
As per coding guidelines, type hints have been properly added to all function parameters and return values.
Also applies to: 38-40, 68-70, 98-100, 130-132
backend/app/tests/api/test_permissions.py (1)
14-154: LGTM! Type hints properly applied across all test methods.All test methods now have explicit
-> Nonereturn type annotations, which aligns perfectly with the coding guidelines and PR objectives. The multiline formatting applied to longer method signatures improves readability. No logic changes were made, and the test coverage remains solid.backend/app/tests/api/routes/test_project.py (4)
14-16: LGTM: Well-typed fixture.The fixture properly declares its return type and parameter types, aligning with the coding guidelines.
20-22: LGTM: Type hints added correctly.The explicit
-> Nonereturn type and multi-line parameter formatting improve code clarity and satisfy the coding guidelines requirement for type hints on all function parameters and return values.
64-66: LGTM: Type hints added correctly.The explicit
-> Nonereturn type properly satisfies the coding guidelines requirement for type hints on all function return values.
84-86: LGTM: Type hints added correctly.The explicit
-> Nonereturn type properly satisfies the coding guidelines requirement for type hints on all function return values.backend/app/tests/api/routes/test_llm.py (3)
1-5: LGTM: Import formatting improved.The blank lines added after imports follow PEP 8 conventions for grouping standard library, third-party, and local imports.
17-19: LGTM: Type hints properly added.The return type annotation
-> Nonecorrectly reflects that test functions don't return values. This aligns with the coding guidelines requiring type hints for all function parameters and return values.
55-166: LGTM: Consistent type hints across all test functions.All remaining test functions have been updated with proper return type annotations (
-> None). The changes are consistent and comply with the coding guidelines. The test logic, assertions, and mocking remain unchanged and correct.backend/app/tests/services/collections/test_helpers.py (1)
4-4: LGTM! Type hints added correctly to test functions.The addition of explicit return type annotations (
-> None) and themonkeypatch: Anyparameter type align well with the coding guidelines and PR objectives.As per coding guidelines, type hints should be added to all function parameters and return values.
Also applies to: 11-11, 18-18, 25-25, 33-33, 56-56, 69-69, 80-80, 90-90
backend/app/tests/api/test_auth_failures.py (1)
41-84: LGTM! Comprehensive authentication failure testing.The test functions now have proper type annotations and provide excellent coverage of authentication failure scenarios. The parametrized approach efficiently validates that all protected endpoints consistently enforce authentication requirements.
backend/app/tests/api/routes/test_org.py (1)
20-22: LGTM! Type annotations properly added.The function signatures have been correctly updated with explicit return type annotations, fully complying with the coding guidelines.
Also applies to: 55-59, 78-82
backend/app/tests/crud/test_openai_conversation.py (2)
1-18: LGTM! Import organization is clean.The import additions and reorganization follow PEP 8 conventions. The
Projectimport is correctly used later in the test suite for project scoping tests.
21-823: Excellent type safety improvements!All test functions now have explicit
-> Nonereturn type annotations, which improves type checking and follows the project's coding guidelines. The type hints are consistently applied across all 20+ test functions.backend/app/tests/api/routes/test_doc_transformation_job.py (3)
1-13: LGTM! Imports are clean and well-organized.The imports are appropriate for the test scenarios covered in this file, with proper typing support from TestClient, Session, and custom test utilities.
15-138: Excellent type hint coverage and comprehensive test scenarios.All test methods in this class now have explicit
-> Nonereturn type hints, which aligns perfectly with the coding guidelines. The test coverage is thorough, including success cases, error handling (404, 422), project isolation, and different job statuses.
140-366: Excellent type hint coverage and thorough batch operation testing.All test methods now have explicit
-> Nonereturn type hints, fully complying with the coding guidelines. The test suite comprehensively covers batch job retrieval scenarios including:
- Success cases with multiple jobs
- Mixed existing/non-existing jobs
- Edge cases (empty, whitespace, invalid UUIDs)
- Project isolation
- Various job statuses
The flexible error message assertions (e.g., lines 211-212, 246-247, 271-275) using
orconditions are appropriate for handling potential variations in validation error formats from FastAPI/Pydantic.backend/app/tests/crud/test_fine_tuning.py (1)
18-18: LGTM! Proper type hints added.The addition of
-> Nonereturn type annotations to all test functions correctly completes the type hint coverage and aligns with the coding guidelines requirement to add type hints to all function parameters and return values.Also applies to: 45-45, 55-55, 61-61, 69-69, 75-75, 90-90, 101-101
backend/app/tests/api/routes/test_evaluation.py (2)
16-19: Add type hints to helper function.The
create_csv_filehelper function is missing type hints on its parameter and return value.🔎 Proposed fix with type hints
-def create_csv_file(content: str) -> tuple[str, io.BytesIO]: +def create_csv_file(content: str) -> tuple[str, io.BytesIO]: """Create a CSV file-like object for testing."""Wait, I see the signature already shows
content: strbut let me verify the actual code:-def create_csv_file(content: str) -> tuple[str, io.BytesIO]: +def create_csv_file(content: str) -> tuple[str, io.BytesIO]:Actually, looking at line 16, the function signature in the diff doesn't show type hints. Here's the correct fix:
-def create_csv_file(content: str) -> tuple[str, io.BytesIO]: +def create_csv_file(content: str) -> tuple[str, io.BytesIO]:Let me re-read the line more carefully. Line 16 shows:
def create_csv_file(content: str) -> tuple[str, io.BytesIO]:This already has type hints! So this is actually correct. Let me skip this comment.
1-1012: Excellent type hint coverage!All test methods, fixtures, and helper functions now have comprehensive type hints that align with the coding guidelines. The additions properly use concrete types (TestClient, Session, TestAuthContext, EvaluationDataset) and appropriate generics (dict[str, Any], dict[str, str]).
The type annotations significantly improve code clarity and type safety across:
- CSV validation and upload tests
- Duplication factor tests
- Error handling tests
- Batch evaluation and JSONL building tests
- Evaluation run status tests
- Dataset CRUD operation tests
This addresses all previous review comments regarding missing type hints.
As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
backend/app/tests/api/routes/test_creds.py (1)
1-290: Excellent work addressing all previous review feedback!All the issues from prior reviews have been properly resolved:
- Return type hints (
-> None) added to all test functions ✓- Line 153 now uses
Credential.is_activeinstead of== Truecomparison ✓- Import of
selectfrom sqlmodel added for the new encryption test ✓The test suite is comprehensive and covers:
- CRUD operations for credentials
- Provider-specific endpoints (OPENAI, LANGFUSE)
- Error scenarios (404, 400 cases)
- Database-level encryption verification (new test at lines 144-163)
- API key context precedence over request body IDs
The code follows all coding guidelines and demonstrates good testing practices with proper assertions and verification steps.
backend/app/tests/crud/test_assistants.py (2)
1-3: LGTM! Import ordering improved.The import reordering follows Python conventions by grouping standard library imports before third-party imports.
27-27: Excellent addition of return type annotations!All test methods now include explicit
-> Nonereturn type annotations, which significantly improves code clarity and aligns with the coding guidelines requiring type hints throughout the codebase.As per coding guidelines, this change ensures consistency with the project's type safety standards.
Also applies to: 49-49, 63-63, 76-76, 91-91, 105-105, 118-118, 136-138, 165-165, 194-194, 233-233, 259-261, 286-286, 302-302, 324-324, 353-353, 373-373, 385-385, 396-396, 407-407, 416-416, 429-429, 469-469
backend/app/tests/crud/collections/collection/test_crud_collection_read_one.py (4)
8-8: LGTM! Type hints properly supported.The
Collectionimport is correctly added to support the return type annotation in themk_collectionhelper function, following the coding guideline for comprehensive type hints.
14-14: LGTM! Return type annotation correctly applied.The return type
-> Collectionaccurately reflects the function's behavior and adheres to the coding guideline requiring type hints on all function parameters and return values.
27-33: LGTM! Test properly typed.The return type annotation
-> Noneis correctly applied, following the coding guideline for comprehensive type hints in test methods.
35-42: LGTM! Enhanced test coverage with proper assertions.The return type annotation is correctly added, and the expanded assertions significantly improve test quality by:
- Validating the specific HTTP status code (404)
- Verifying the exact error message ("Collection not found")
This ensures the authorization boundary is properly enforced when attempting to access collections from unauthorized projects, matching the behavior defined in
CollectionCrud.read_one.backend/app/tests/crud/documents/test_doc_transformation_job.py (3)
18-20: LGTM! Fixture properly typed.The
storefixture now has an explicit return type annotation that correctly identifies it returns aDocumentStore. This aligns with the coding guidelines requiring type hints throughout the codebase.
24-25: LGTM! Fixture properly typed with dependency.The
crudfixture correctly uses thestorefixture as a dependency and has the proper return type annotation.
29-51: LGTM! All test methods properly typed.All test methods in this file now have explicit
-> Nonereturn type annotations, which aligns with the coding guidelines and improves type safety across the test suite.Also applies to: 53-70, 74-95, 97-127, 131-181, 185-250, 252-271
backend/app/tests/crud/documents/documents/test_crud_document_read_one.py (2)
12-14: LGTM! Fixture properly typed.The
storefixture now has an explicit-> DocumentStorereturn type annotation, improving type safety.
18-48: LGTM! All test methods properly typed.All test methods now include explicit
-> Nonereturn type annotations. The multiline formatting fortest_cannot_read_others_documentsimproves readability while maintaining proper type hints.Also good cleanup removing the unused
NoResultFoundimport mentioned in the summary.backend/app/tests/crud/documents/documents/test_crud_document_read_many.py (2)
10-15: LGTM! Fixture properly typed with pre-filled data.The
storefixture correctly returnsDocumentStoreand appropriately pre-fills it with test documents for the read_many tests. The type annotation is correct.
21-114: LGTM! All test methods properly typed.All ten test methods now have explicit
-> Nonereturn type annotations, completing the type safety improvements across this test suite.backend/app/tests/api/routes/test_openai_conversation.py (1)
10-14: Type annotations look good.The explicit
-> Nonereturn type annotations across all test functions align with the coding guidelines for Python 3.11+ type hints. This improves code clarity and enables better static analysis tooling.backend/app/tests/crud/evaluations/test_embeddings.py (1)
16-16: Excellent type hint additions!The
-> Nonereturn type annotations have been consistently added to all test methods, which properly aligns with the coding guidelines. The test suite demonstrates good coverage with well-structured test cases for:
- Basic functionality and edge cases
- Error handling scenarios
- Mathematical edge cases (zero vectors, orthogonal vectors, opposite vectors)
- Statistical calculations
The tests are clear, well-documented, and align with the production code implementation.
Also applies to: 49-49, 69-69, 103-103, 130-130, 169-169, 194-194, 230-230, 239-239, 248-248, 257-257, 266-266, 279-279, 307-307, 318-318, 341-341
backend/app/tests/crud/documents/documents/test_crud_document_delete.py (4)
13-13: Type hint addition looks good.The return type annotation
-> Documentcorrectly follows the coding guideline to add type hints to all function parameters and return values.
26-33: Excellent type hints and soft delete verification tests.The return type annotations
-> Noneare correctly applied to all test methods, following the coding guidelines. The test assertions appropriately verify soft delete behavior: document persistence, is_deleted flag, and timestamp ordering.
35-46: Excellent cross-project security test coverage.The expanded test properly verifies that documents cannot be deleted across project boundaries. The test creates isolated test data (separate project), attempts deletion from the wrong project context, and validates both the status code and error message. This is a valuable addition to security testing.
The return type annotation
-> Nonecorrectly follows coding guidelines.
9-9: No changes needed. The importfrom app.core.exception_handlers import HTTPExceptionis correct—it re-exports FastAPI'sHTTPExceptionand is fully compatible with the test code usage of.status_codeand.detailattributes (lines 45-46). This import pattern is intentional and used consistently throughout the codebase for centralized exception handling.Likely an incorrect or invalid review comment.
backend/app/tests/crud/evaluations/test_processing.py (1)
21-236: Excellent test coverage for parsing evaluation outputs.The test class comprehensively covers various scenarios including basic parsing, simple strings, errors, missing data, JSON strings, and multiple items. All methods have proper type hints.
backend/app/tests/crud/evaluations/test_dataset.py (3)
17-19: LGTM! Imports properly support the new deletion tests.The new imports (
now,EvaluationRun,delete_dataset) are correctly organized and necessary for the comprehensive deletion test coverage added in theTestDeleteDatasetclass.
25-383: LGTM! Type hints correctly applied throughout existing tests.All test methods now have proper
-> Nonereturn type annotations, fully complying with the Python 3.11+ type hints requirement specified in the coding guidelines.
385-519: LGTM! Comprehensive deletion test coverage.The
TestDeleteDatasetclass provides excellent test coverage for dataset deletion scenarios:
- ✓ Successful deletion and verification
- ✓ Not-found error handling (404 case)
- ✓ Organization-level authorization validation
- ✓ Referential integrity constraint (datasets with evaluation runs)
All test logic aligns correctly with the
delete_datasetCRUD function behavior, and assertions properly validate both success and failure paths.backend/app/tests/crud/collections/collection_job/test_collection_jobs.py (3)
14-32: LGTM! Type hints added correctly.The function signature now includes proper type hints for all parameters and the return type, which aligns with the coding guidelines requiring type hints throughout the codebase.
35-38: LGTM! Fixture properly typed.The
sample_projectfixture now has an explicit return type annotation-> Project, which improves type safety and IDE support for tests using this fixture.
41-60: LGTM! Test functions properly typed.All test functions now include:
- Explicit parameter types (
db: Session,sample_project: Project)- Return type annotations (
-> None)This follows the coding guidelines and improves code maintainability.
Also applies to: 63-76, 78-93, 95-110, 112-128
backend/app/tests/crud/collections/collection/test_crud_collection_read_all.py (1)
33-33: Remove the explicitdb.exec(delete(Collection))call as it is redundant.The function-scoped
dbfixture inconftest.pyalready provides test isolation through transaction rollback (lines 27-45). Each test runs in its own transaction that is automatically rolled back, ensuring collections created in one test don't affect others. No parallel test execution is configured in the project, and other CRUD test files in the same directory do not use explicit global deletes. This pattern is unnecessary and creates confusion about the actual isolation mechanism.Likely an incorrect or invalid review comment.
backend/app/tests/services/collections/test_delete_collection.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
backend/app/tests/crud/evaluations/test_processing.py (2)
260-260: Add missing type hint to fixture parameter.The
test_datasetparameter lacks a type hint, violating the coding guideline requirement.🔎 Proposed fix
- def eval_run_with_batch(self, db: Session, test_dataset) -> EvaluationRun: + def eval_run_with_batch(self, db: Session, test_dataset: EvaluationDataset) -> EvaluationRun:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
447-447: Add missing type hint to fixture parameter.The
test_datasetparameter lacks a type hint, violating the coding guideline requirement.🔎 Proposed fix
- def eval_run_with_embedding_batch(self, db: Session, test_dataset) -> EvaluationRun: + def eval_run_with_embedding_batch(self, db: Session, test_dataset: EvaluationDataset) -> EvaluationRun:As per coding guidelines: "Always add type hints to all function parameters and return values in Python code"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/tests/crud/evaluations/test_embeddings.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/services/collections/test_delete_collection.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/crud/evaluations/test_embeddings.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/crud/evaluations/test_embeddings.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
🔇 Additional comments (13)
backend/app/tests/services/collections/test_delete_collection.py (3)
1-1: LGTM! Type hint imports properly added.The
AnyandSessionimports support the type hints added to test function signatures throughout this file.Also applies to: 5-5
18-18: LGTM! Type hints are complete.The function signature now has proper type hints for all parameters and return type.
144-146: LGTM! Type hints are complete.All function parameters have proper type hints, including the mock parameter typed as
Any.backend/app/tests/crud/evaluations/test_processing.py (6)
1-20: LGTM!Imports are well-organized with proper typing support and all necessary test dependencies.
21-236: LGTM!Comprehensive test coverage for
parse_evaluation_outputwith proper type annotations and various edge cases (errors, missing data, JSON parsing, multiple items).
296-423: LGTM!The test methods provide excellent coverage for completed evaluation processing, including success scenarios, edge cases (no results, missing batch_job_id), and proper use of mocking for external dependencies.
484-561: LGTM!The tests thoroughly validate embedding batch processing with comprehensive mocking and verification of success and no-results scenarios.
563-708: LGTM!Excellent test coverage for the check-and-process workflow, validating both completed and failed batch scenarios with proper status transitions.
731-806: LGTM!The tests validate polling behavior for both empty and active evaluation scenarios with appropriate mocking of client dependencies.
backend/app/tests/crud/evaluations/test_embeddings.py (4)
11-123: LGTM! Comprehensive test coverage for JSONL building.The test class properly validates the
build_embedding_jsonlfunction with return type hints correctly applied. Coverage includes basic functionality, custom models, empty value filtering, and missing ID handling.
125-223: LGTM! Robust error handling in parsing tests.The parsing tests correctly validate conversion from OpenAI batch API format (custom_id) to internal format (trace_id), with proper handling of errors and incomplete data.
225-272: LGTM! Excellent mathematical edge case coverage.The cosine similarity tests comprehensively validate the calculation with proper floating-point comparison tolerances. Edge cases (zero vectors, orthogonal, opposite) are correctly handled.
274-371: LGTM! Statistics calculations properly validated.The average similarity tests verify correct computation of mean, standard deviation, and per-item scores. Edge case (empty list) returns safe zero values.
backend/app/tests/services/collections/test_delete_collection.py
Outdated
Show resolved
Hide resolved
backend/app/tests/services/collections/test_delete_collection.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/app/tests/api/routes/collections/test_create_collections.py (1)
1-3: Missing import forAnytype.The
Anytype is used on lines 18 and 65 but not imported from thetypingmodule, which will cause aNameError.🔎 Proposed fix
from uuid import UUID, uuid4 +from typing import Any from unittest.mock import patchbackend/app/tests/services/collections/test_create_collection.py (1)
264-337: Consolidate duplicate test functions.The tests
test_execute_job_success_flow_callback_job_and_creates_collection(lines 264-337) andtest_execute_job_success_creates_collection_with_callback(lines 344-417) are completely identical—same setup, mocks, execution, and assertions. This violates the DRY principle and increases maintenance burden.🔎 Proposed fix: Remove duplicate test
Remove one of the duplicate tests. If both test names are meaningful, keep the more descriptive one:
-@pytest.mark.usefixtures("aws_credentials") -@mock_aws -@patch("app.services.collections.create_collection.get_openai_client") -@patch("app.services.collections.create_collection.send_callback") -def test_execute_job_success_creates_collection_with_callback( - mock_send_callback: Any, mock_get_openai_client: Any, db: Session -) -> None: - """ - execute_job should: - - set task_id on the CollectionJob - - ingest documents into a vector store - - create an OpenAI assistant - - create a Collection with llm fields filled - - link the CollectionJob -> collection_id, set status=successful - - create DocumentCollection links - """ - project = get_project(db) - - aws = AmazonCloudStorageClient() - aws.create() - - store = DocumentStore(db=db, project_id=project.id) - document = store.put() - s3_key = Path(urlparse(document.object_store_url).path).relative_to("/") - aws.client.put_object(Bucket=settings.AWS_S3_BUCKET, Key=str(s3_key), Body=b"test") - - callback_url = "https://example.com/collections/create-success" - - sample_request = CreationRequest( - model="gpt-4o", - instructions="string", - temperature=0.000001, - documents=[document.id], - batch_size=1, - callback_url=callback_url, - ) - - mock_client = get_mock_openai_client_with_vector_store() - mock_get_openai_client.return_value = mock_client - - job_id = uuid.uuid4() - _ = get_collection_job( - db, - project, - job_id=job_id, - action_type=CollectionActionType.CREATE, - status=CollectionJobStatus.PENDING, - collection_id=None, - ) - - task_id = uuid.uuid4() - - with patch("app.services.collections.create_collection.Session") as SessionCtor: - SessionCtor.return_value.__enter__.return_value = db - SessionCtor.return_value.__exit__.return_value = False - - mock_send_callback.return_value = MagicMock(status_code=403) - - execute_job( - request=sample_request.model_dump(), - project_id=project.id, - organization_id=project.organization_id, - task_id=str(task_id), - with_assistant=True, - job_id=str(job_id), - task_instance=None, - ) - - updated_job = CollectionJobCrud(db, project.id).read_one(job_id) - collection = CollectionCrud(db, project.id).read_one(updated_job.collection_id) - - mock_send_callback.assert_called_once() - cb_url_arg, payload_arg = mock_send_callback.call_args.args - assert str(cb_url_arg) == callback_url - assert payload_arg["success"] is True - assert payload_arg["data"]["status"] == CollectionJobStatus.SUCCESSFUL - assert payload_arg["data"]["collection"]["id"] == str(collection.id) - assert uuid.UUID(payload_arg["data"]["job_id"]) == job_id - -Alternatively, if there were intended differences, use
@pytest.mark.parametrizeto share the test logic while varying inputs.Also applies to: 344-417
🧹 Nitpick comments (5)
backend/app/tests/services/collections/test_delete_collection.py (2)
258-258: Remove redundant import.
execute_jobis already imported at the module level (line 15), making this local import unnecessary.🔎 Proposed fix
- from app.services.collections.delete_collection import execute_job - execute_job(
339-339: Remove redundant import.
execute_jobis already imported at the module level (line 15), making this local import unnecessary.🔎 Proposed fix
- from app.services.collections.delete_collection import execute_job - execute_job(backend/app/tests/api/routes/collections/test_collection_list.py (1)
108-110: Consider removing unuseddbparameter.The
dbparameter is not used in this test function. If it's not needed for setup or future extension, consider removing it for clarity.🔎 Proposed change
def test_list_collections_does_not_error_with_no_collections( - db: Session, client: TestClient, user_api_key_header: dict[str, str] + client: TestClient, user_api_key_header: dict[str, str] ) -> None:backend/app/tests/api/routes/test_fine_tuning.py (1)
22-34: Consider adding type hints to the helper function.While not critical, adding type annotations to
create_file_mockand its inner_side_effectfunction would complete the typing effort in this file and align with the coding guidelines.🔎 Proposed type annotations
+from typing import Any, Callable, Optional + -def create_file_mock(file_type): +def create_file_mock(file_type: str) -> Callable[[Optional[Any], Optional[str]], Optional[MagicMock]]: counter = {"train": 0, "test": 0} - def _side_effect(file=None, purpose=None): + def _side_effect(file: Optional[Any] = None, purpose: Optional[str] = None) -> Optional[MagicMock]: if purpose == "fine-tune": if "train" in file.name: counter["train"] += 1 return MagicMock(id=f"file_{counter['train']}") elif "test" in file.name: counter["test"] += 1 return MagicMock(id=f"file_{counter['test']}") + return None return _side_effectbackend/app/tests/services/collections/test_create_collection.py (1)
26-26: Consider-> Noneinstead of-> Anyfor clarity.The
aws_credentialsfixture sets environment variables and doesn't return a meaningful value. While-> Anyis acceptable for fixtures, using-> Nonewould more accurately reflect the implicit return value and improve type precision.🔎 Proposed fix
-def aws_credentials() -> Any: +def aws_credentials() -> None:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/app/tests/api/routes/collections/test_collection_delete.pybackend/app/tests/api/routes/collections/test_collection_info.pybackend/app/tests/api/routes/collections/test_collection_list.pybackend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/api/routes/documents/test_route_document_info.pybackend/app/tests/api/routes/documents/test_route_document_list.pybackend/app/tests/api/routes/documents/test_route_document_permanent_remove.pybackend/app/tests/api/routes/documents/test_route_document_remove.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/api/routes/test_assistants.pybackend/app/tests/api/routes/test_fine_tuning.pybackend/app/tests/api/routes/test_model_evaluation.pybackend/app/tests/api/test_deps.pybackend/app/tests/core/test_callback_ssrf.pybackend/app/tests/crud/evaluations/test_processing.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/services/collections/test_delete_collection.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py
- backend/app/tests/core/test_callback_ssrf.py
- backend/app/tests/crud/evaluations/test_processing.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/api/routes/documents/test_route_document_list.pybackend/app/tests/api/routes/collections/test_collection_list.pybackend/app/tests/api/routes/collections/test_collection_delete.pybackend/app/tests/api/routes/collections/test_collection_info.pybackend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/api/routes/documents/test_route_document_info.pybackend/app/tests/api/routes/test_fine_tuning.pybackend/app/tests/api/routes/test_model_evaluation.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/api/routes/documents/test_route_document_remove.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/api/test_deps.pybackend/app/tests/api/routes/test_assistants.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/api/routes/documents/test_route_document_list.pybackend/app/tests/api/routes/collections/test_collection_list.pybackend/app/tests/api/routes/collections/test_collection_delete.pybackend/app/tests/api/routes/collections/test_collection_info.pybackend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/api/routes/documents/test_route_document_info.pybackend/app/tests/api/routes/test_fine_tuning.pybackend/app/tests/api/routes/test_model_evaluation.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/api/routes/documents/test_route_document_remove.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/api/test_deps.pybackend/app/tests/api/routes/test_assistants.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Applied to files:
backend/app/tests/api/routes/test_assistants.py
🧬 Code graph analysis (13)
backend/app/tests/api/routes/documents/test_route_document_list.py (5)
backend/app/tests/api/routes/documents/test_route_document_info.py (2)
test_response_is_success(21-30)route(16-17)backend/app/tests/api/routes/documents/test_route_document_remove.py (2)
test_response_is_success(27-42)route(20-21)backend/app/tests/api/routes/documents/test_route_document_permanent_remove.py (1)
route(28-29)backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
route(75-76)backend/app/tests/utils/document.py (3)
crawler(170-171)WebCrawler(115-129)get(119-123)
backend/app/tests/api/routes/collections/test_collection_list.py (1)
backend/app/tests/conftest.py (2)
user_api_key_header(90-92)db(28-45)
backend/app/tests/api/routes/collections/test_collection_delete.py (2)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)backend/app/tests/conftest.py (3)
db(28-45)user_api_key_header(90-92)user_api_key(102-104)
backend/app/tests/api/routes/collections/test_collection_info.py (2)
backend/app/models/document.py (1)
Document(28-69)backend/app/tests/conftest.py (1)
user_api_key_header(90-92)
backend/app/tests/api/routes/collections/test_create_collections.py (2)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)backend/app/tests/conftest.py (3)
client(65-68)user_api_key_header(90-92)user_api_key(102-104)
backend/app/tests/services/collections/test_delete_collection.py (2)
backend/app/tests/services/collections/test_create_collection.py (1)
test_start_job_creates_collection_job_and_schedules_task(51-113)backend/app/tests/conftest.py (1)
db(28-45)
backend/app/tests/api/routes/test_fine_tuning.py (2)
backend/app/core/cloud/storage.py (1)
client(29-41)backend/app/tests/conftest.py (2)
db(28-45)user_api_key_header(90-92)
backend/app/tests/api/routes/test_model_evaluation.py (2)
backend/app/tests/utils/test_data.py (2)
create_test_finetuning_job_with_extra_fields(196-207)create_test_model_evaluation(210-235)backend/app/tests/conftest.py (2)
db(28-45)user_api_key_header(90-92)
backend/app/tests/services/collections/test_create_collection.py (3)
backend/app/models/collection_job.py (4)
CollectionJobStatus(11-15)CollectionJob(23-101)CollectionActionType(18-20)job_id(92-93)backend/app/models/collection.py (1)
CreationRequest(156-165)backend/app/models/project.py (1)
Project(51-107)
backend/app/tests/api/routes/documents/test_route_document_remove.py (1)
backend/app/tests/utils/document.py (2)
Route(84-111)WebCrawler(115-129)
backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
backend/app/tests/utils/document.py (1)
Route(84-111)
backend/app/tests/api/test_deps.py (1)
backend/app/tests/conftest.py (2)
db(28-45)client(65-68)
backend/app/tests/api/routes/test_assistants.py (2)
backend/app/tests/conftest.py (3)
client(65-68)user_api_key(102-104)db(28-45)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)
🪛 Ruff (0.14.10)
backend/app/tests/api/routes/documents/test_route_document_list.py
30-30: Redefinition of unused crawler from line 9
(F811)
70-70: Redefinition of unused crawler from line 9
(F811)
backend/app/tests/api/routes/collections/test_collection_list.py
109-109: Unused function argument: db
(ARG001)
backend/app/tests/api/routes/collections/test_create_collections.py
18-18: Undefined name Any
(F821)
65-65: Undefined name Any
(F821)
backend/app/tests/api/routes/documents/test_route_document_remove.py
29-29: Undefined name Any
(F821)
32-32: Redefinition of unused crawler from line 15
(F811)
48-48: Undefined name Any
(F821)
51-51: Redefinition of unused crawler from line 15
(F811)
72-72: Undefined name Any
(F821)
75-75: Redefinition of unused crawler from line 15
(F811)
backend/app/tests/api/routes/documents/test_route_document_upload.py
157-157: Undefined name Any
(F821)
backend/app/tests/api/routes/test_assistants.py
33-33: Undefined name Any
(F821)
55-55: Undefined name Any
(F821)
94-94: Undefined name Any
(F821)
153-153: Undefined name Any
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (22)
backend/app/tests/api/routes/test_model_evaluation.py (1)
1-121: LGTM! Type hints align perfectly with coding guidelines.The addition of explicit type annotations to all test functions improves type safety and follows the project's coding guidelines. All type hints are accurate and use Python 3.11+ syntax (e.g.,
dict[str, str]instead ofDict[str, str]).backend/app/tests/api/test_deps.py (3)
4-5: LGTM! TestClient import added to support type annotations.The TestClient import is properly added to support the type hint for the
clientparameter intest_get_auth_context_with_inactive_user_via_token. This aligns with the PR's goal of applying strict type hints across test utilities.
22-22: Excellent addition of return type annotations!All test methods now have explicit
-> Nonereturn type annotations, which perfectly aligns with the coding guidelines requiring type hints for all function parameters and return values. This improves code clarity and enables better static analysis.Also applies to: 35-35, 51-51, 64-64, 77-77, 89-89, 111-111, 135-135, 155-155
109-111: Well-typed parameter addition for the client fixture.The
client: TestClientparameter is correctly typed and properly used on line 115 for token authentication. The type hint accurately reflects the yielded value from theclientfixture defined in conftest.py.backend/app/tests/services/collections/test_delete_collection.py (1)
1-5: LGTM! Type hints successfully added.All function parameters and return types now have proper type annotations, addressing the past review comments. The imports of
AnyandSessionare correctly added to support these type hints.Also applies to: 18-18, 79-80, 144-146, 214-216, 294-296
backend/app/tests/api/routes/collections/test_create_collections.py (1)
7-7: Good type hint additions.The explicit type annotations for
TestAuthContext,dict[str, str], and return types improve type safety across the test suite, aligning with the PR's typing enhancement objectives.Also applies to: 18-22, 65-69, 106-106
backend/app/tests/api/routes/collections/test_collection_list.py (1)
14-15: LGTM on type hint additions.The explicit type annotations for test parameters and return types enhance type safety and align with the PR's goal of stricter typing across the test suite.
Also applies to: 39-40, 78-79
backend/app/tests/api/routes/collections/test_collection_delete.py (1)
2-2: LGTM! Clean type hint additions.All necessary imports are present, and the type annotations are correctly applied across all test functions, improving type safety and code clarity.
Also applies to: 9-9, 17-22, 64-69, 108-111
backend/app/tests/api/routes/collections/test_collection_info.py (2)
14-18: Excellent addition of return type to helper function.Adding the
-> Documentreturn type to thelink_document_to_collectionhelper improves type safety and makes the function's contract explicit.
36-40: LGTM on comprehensive type hint additions.All test functions now have explicit type annotations for parameters and return types, improving type safety across the test suite and aligning with the PR's typing enhancement objectives.
Also applies to: 76-80, 106-110, 141-145, 173-176
backend/app/tests/api/routes/test_assistants.py (1)
31-36: Type annotations correctly implement coding guidelines.The addition of
-> Nonereturn type annotations and: Anyfor mock parameters aligns with the coding guidelines. Once the missingAnyimport is added (see previous comment), the type hints will be complete and correct.Also applies to: 53-59, 92-98, 119-123, 151-157, 178-181, 198-202, 220-223, 237-241, 261-264, 288-292, 307-310
backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
102-102: LGTM! Explicit return type annotations added.The
-> Nonereturn type annotations align with the coding guidelines requiring type hints for all function parameters and return values.Also applies to: 119-119, 142-142, 162-162, 195-195, 219-219, 236-236, 258-258, 285-285, 309-309
backend/app/tests/api/routes/documents/test_route_document_info.py (1)
26-26: LGTM! Explicit return type annotations added.The
-> Nonereturn type annotations align with the coding guidelines requiring type hints for all function parameters and return values.Also applies to: 37-37, 48-48
backend/app/tests/api/routes/documents/test_route_document_remove.py (2)
1-2: LGTM! Import statement added.The
unittest.mock.patchimport is correctly placed.
33-33: LGTM! Explicit return type annotations added.The
-> Nonereturn type annotations align with the coding guidelines requiring type hints for all function parameters and return values.Also applies to: 52-52, 76-76
backend/app/tests/api/routes/documents/test_route_document_list.py (1)
30-30: LGTM! Explicit return type annotations added.The
-> Nonereturn type annotations align with the coding guidelines requiring type hints for all function parameters and return values.Also applies to: 39-39, 50-50, 63-63, 71-71, 80-80
backend/app/tests/api/routes/test_fine_tuning.py (5)
2-8: LGTM! Clean import additions for type safety.The added imports (
Any,patch,MagicMock,Session,TestClient) properly support the type annotations throughout the test suite and improve static type checking capabilities.
40-45: LGTM! Proper type annotations applied.The explicit type hints for test parameters and return type improve type safety and enable better IDE support. The annotations correctly match the fixture types defined in
conftest.py.
122-128: LGTM! Appropriate typing for mocked test methods.The type annotations correctly use
Anyfor mock parameters (since mocks are dynamically accessed) while maintaining strict typing for other parameters. This is the right balance between type safety and test flexibility.Also applies to: 155-161
193-195: LGTM! Consistent type annotations.The method signature follows the established typing pattern used throughout the test suite, maintaining consistency and type safety.
224-232: LGTM! Comprehensive type coverage for evaluation tests.All test methods in the
TestAutoEvaluationTriggerclass now have complete type annotations, improving the maintainability of these complex test scenarios. The consistent typing pattern across all methods enhances code readability.Also applies to: 280-288, 347-355, 406-414
backend/app/tests/services/collections/test_create_collection.py (1)
34-51: Excellent type hint additions!The type annotations for
create_collection_job_for_createand the test functions follow the coding guidelines precisely. The use of concrete types (Project,CollectionJob,Session) andAnyfor mocks is appropriate and enhances type safety.Also applies to: 118-121, 198-201
There was a problem hiding this comment.
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/tests/services/collections/test_create_collection.py (1)
25-32: Correct the fixture return type annotation.The
aws_credentialsfixture doesn't return a value (it only sets environment variables), so its return type should be-> Nonerather than-> Any.🔎 Proposed fix
@pytest.fixture(scope="function") -def aws_credentials() -> Any: +def aws_credentials() -> None: os.environ["AWS_ACCESS_KEY_ID"] = "testing"As per coding guidelines, all function return values must have accurate type hints.
🧹 Nitpick comments (8)
backend/app/tests/api/routes/documents/test_route_document_remove.py (1)
16-16: Remove unusedcrawlerimport.The
crawleridentifier is imported but never used directly in this file. It's only referenced as a pytest fixture parameter (lines 33, 52, 76), which is injected by pytest's dependency injection mechanism. The import is redundant.🔎 Proposed fix
from app.tests.utils.document import ( DocumentMaker, DocumentStore, Route, WebCrawler, - crawler, )backend/app/tests/api/routes/documents/test_route_document_upload.py (3)
26-32: Consider adding proper type hints to theputmethod.The method parameters
target_formatandtransformeruse= Nonebut lack explicit optional type hints. The method also lacks a return type annotation. As per the coding guidelines, all function parameters and return values should have type hints.🔎 Proposed fix
class WebUploader(WebCrawler): def put( self, route: Route, scratch: Path, - target_format: str = None, - transformer: str = None, - ): + target_format: str | None = None, + transformer: str | None = None, + ) -> Response:Note: Replace
Responsewith the actual return type fromself.client.post.
189-196: Add missing type hint formock_start_jobparameter.For consistency with
test_upload_with_transformation(line 158), themock_start_jobparameter should also have theAnytype hint here.🔎 Proposed fix
@patch("app.services.doctransform.job.start_job") def test_upload_with_specific_transformer( self, - mock_start_job, + mock_start_job: Any, db: Session, route: Route, pdf_scratch: Path, uploader: WebUploader, ) -> None:
279-286: Add missing type hint formock_start_jobparameter.For consistency with
test_upload_with_transformation(line 158), themock_start_jobparameter should also have theAnytype hint here.🔎 Proposed fix
@patch("app.services.doctransform.job.start_job") def test_transformation_job_created_in_database( self, - mock_start_job, + mock_start_job: Any, db: Session, route: Route, pdf_scratch: Path, uploader: WebUploader, ) -> None:backend/app/tests/api/routes/test_assistants.py (2)
15-29: Add return type annotations to fixtures.Both fixtures are missing return type annotations, which is required by the coding guidelines. As per coding guidelines, all functions including fixtures should have complete type hints.
🔎 Proposed fix
@pytest.fixture -def assistant_create_payload(): +def assistant_create_payload() -> dict[str, Any]: return { "name": "Test Assistant", "instructions": "This is a test instruction.", "model": "gpt-4o", "vector_store_ids": ["vs_test_1", "vs_test_2"], "temperature": 0.5, "max_num_results": 10, } @pytest.fixture -def assistant_id(): +def assistant_id() -> str: return str(uuid4())
54-60: Use specificdict[str, Any]type hints.Several test functions use the generic
dicttype hint instead of the more specificdict[str, Any]form required for Python 3.11+. As per coding guidelines, type hints should be complete and specific.🔎 Proposed fix
def test_create_assistant_success( mock_verify_vector_ids: Any, client: TestClient, - assistant_create_payload: dict, + assistant_create_payload: dict[str, Any], user_api_key: TestAuthContext, ) -> None: def test_create_assistant_invalid_vector_store( mock_verify_vector_ids: Any, client: TestClient, - assistant_create_payload: dict, + assistant_create_payload: dict[str, Any], user_api_key: TestAuthContext, ) -> None: def test_get_assistant_not_found( client: TestClient, - user_api_key_header: dict, + user_api_key_header: dict[str, Any], ) -> None: def test_list_assistants_invalid_pagination( client: TestClient, - user_api_key_header: dict, + user_api_key_header: dict[str, Any], ) -> None: def test_delete_assistant_not_found( client: TestClient, - user_api_key_header: dict, + user_api_key_header: dict[str, Any], ) -> None:Also applies to: 221-224, 262-265, 308-311
backend/app/tests/services/collections/test_delete_collection.py (2)
256-256: Remove redundant import statement.The
execute_jobfunction is already imported at line 13, making this local import unnecessary.🔎 Proposed fix
- from app.services.collections.delete_collection import execute_job - execute_job(
337-337: Remove redundant import statement.The
execute_jobfunction is already imported at line 13, making this local import unnecessary.🔎 Proposed fix
- from app.services.collections.delete_collection import execute_job - execute_job(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/api/routes/documents/test_route_document_remove.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/api/routes/test_assistants.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/services/collections/test_delete_collection.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always add type hints to all function parameters and return values in Python code
Prefix all log messages with the function name in square brackets:logger.info(f"[function_name] Message {mask_string(sensitive_value)}")
Use Python 3.11+ with type hints throughout the codebase
Files:
backend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/api/routes/test_assistants.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/api/routes/documents/test_route_document_remove.py
backend/app/tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use factory pattern for test fixtures in
backend/app/tests/
Files:
backend/app/tests/services/collections/test_delete_collection.pybackend/app/tests/api/routes/collections/test_create_collections.pybackend/app/tests/api/routes/test_assistants.pybackend/app/tests/api/routes/documents/test_route_document_upload.pybackend/app/tests/services/collections/test_create_collection.pybackend/app/tests/api/routes/documents/test_route_document_remove.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Seed database with test data using `uv run python -m app.seed_data.seed_data`
📚 Learning: 2025-12-17T15:39:30.469Z
Learnt from: CR
Repo: ProjectTech4DevAI/kaapi-backend PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T15:39:30.469Z
Learning: Applies to backend/app/tests/**/*.py : Use factory pattern for test fixtures in `backend/app/tests/`
Applied to files:
backend/app/tests/api/routes/test_assistants.py
🧬 Code graph analysis (5)
backend/app/tests/services/collections/test_delete_collection.py (2)
backend/app/models/collection.py (1)
DeletionRequest(168-169)backend/app/services/collections/delete_collection.py (2)
start_job(26-54)execute_job(141-220)
backend/app/tests/api/routes/collections/test_create_collections.py (2)
backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)backend/app/tests/conftest.py (2)
user_api_key_header(90-92)user_api_key(102-104)
backend/app/tests/api/routes/test_assistants.py (2)
backend/app/tests/conftest.py (3)
client(65-68)user_api_key(102-104)db(28-45)backend/app/tests/utils/auth.py (1)
TestAuthContext(9-21)
backend/app/tests/api/routes/documents/test_route_document_upload.py (1)
backend/app/tests/utils/document.py (1)
Route(84-111)
backend/app/tests/services/collections/test_create_collection.py (2)
backend/app/models/collection_job.py (4)
CollectionJobStatus(11-15)CollectionJob(23-101)CollectionActionType(18-20)job_id(92-93)backend/app/models/project.py (1)
Project(51-107)
🪛 Ruff (0.14.10)
backend/app/tests/api/routes/documents/test_route_document_remove.py
33-33: Redefinition of unused crawler from line 16
(F811)
52-52: Redefinition of unused crawler from line 16
(F811)
76-76: Redefinition of unused crawler from line 16
(F811)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.12, 6)
🔇 Additional comments (25)
backend/app/tests/api/routes/documents/test_route_document_remove.py (2)
1-2: Type hint imports correctly added.The addition of
from typing import Anyresolves the previous critical issue whereAnywas used without being imported. This allows the type hints on the mock parameters to function correctly.
28-34: Type annotations properly applied.All three test methods now have complete type hints on parameters (
mock_get_openai_client: Any) and return values (-> None), correctly following the coding guidelines. The use ofAnyfor the mocked OpenAI client is appropriate.Also applies to: 46-53, 70-77
backend/app/tests/api/routes/documents/test_route_document_upload.py (3)
3-3: LGTM!The
Anyimport addition resolves the previously flagged issue whereAnywas used without being imported.
97-163: LGTM!The return type annotations (
-> None) are correctly added to the test methods, and themock_start_job: Anytype hint properly types the mock parameter. This aligns with the coding guidelines for explicit type hints.
214-327: LGTM!The
-> Nonereturn type annotations are correctly added to the remaining test methods, maintaining consistency with the rest of the test class.backend/app/tests/api/routes/collections/test_create_collections.py (4)
3-3: LGTM: Type imports added correctly.The
Anyimport is appropriately used for mock objects, andTestAuthContextprovides structured authentication context with proper type safety.Also applies to: 8-8
18-23: LGTM: Type hints align with coding guidelines.The function signature now includes proper type annotations for all parameters and return value. The
TestAuthContexttype correctly models the structured authentication context used in the test body (lines 52-53).
65-70: LGTM: Consistent typing pattern.Type annotations are consistent with the first test function and correctly reflect fixture types and usage patterns (lines 96-97).
105-107: LGTM: Return type annotation added.The
-> Nonereturn type completes the type hints for this test function, satisfying the coding guidelines requirement.backend/app/tests/api/routes/test_assistants.py (2)
1-2: Import issue resolved.The missing
Anyimport has been correctly added. This resolves the previous review concern about undefined type annotations.
32-37: Excellent type hint additions.The addition of
-> Nonereturn types andAnytype hints for mock parameters across these test functions aligns well with the coding guidelines for complete type annotations.Also applies to: 120-124, 152-158, 179-182, 199-203, 238-242, 289-293
backend/app/tests/services/collections/test_delete_collection.py (6)
1-14: LGTM! Type imports properly added.The type annotations (
AnyandSession) are correctly imported and all necessary imports are in place to support the typed test functions.
16-73: LGTM! Type hints properly applied.The test function has complete type annotations with
db: Sessionand-> Nonereturn type, conforming to the coding guidelines.
75-138: LGTM! Comprehensive test with proper typing.The test function has complete type annotations and thoroughly validates the delete success workflow including job updates and remote/local deletion.
141-207: LGTM! Failure path properly tested with correct typing.The test function has complete type annotations and appropriately validates the failure scenario with job status and error message assertions.
210-287: LGTM! Success callback test properly typed.The test function has complete type annotations and thoroughly validates the success callback workflow with correct payload structure assertions.
290-374: LGTM! Failure callback test properly typed.The test function has complete type annotations and appropriately validates the failure callback workflow including error payload structure.
backend/app/tests/services/collections/test_create_collection.py (8)
1-23: LGTM! Imports properly organized with correct type annotations.The imports include all necessary typing support, and
Projectis correctly imported fromapp.models(its canonical location).
34-48: LGTM! Helper function follows factory pattern.The helper function has complete type annotations and follows the factory pattern guideline for test fixtures in
backend/app/tests/. The return type accurately reflects that it returns aCollectionJobinstance.As per coding guidelines, test fixtures should use the factory pattern.
51-114: LGTM! Test function properly typed.The test function has complete type annotations with
db: Sessionand-> Nonereturn type, conforming to the coding guidelines.
116-194: LGTM! Comprehensive success flow test with proper typing.The test function has complete type annotations and thoroughly validates the collection creation workflow including document ingestion, OpenAI integration, and database operations.
196-258: LGTM! Failure path properly tested with correct typing.The test function has complete type annotations and appropriately validates the assistant creation failure scenario, including verification that the vector store is cleaned up on failure.
260-338: LGTM! Success callback test properly typed.The test function has complete type annotations for all mock parameters and thoroughly validates the success callback workflow with correct payload structure assertions.
340-418: LGTM! Duplicate success callback test properly typed.The test function has complete type annotations and validates the callback workflow. Note that this test appears very similar to the previous one (lines 260-338) - you may want to verify if both are necessary.
420-492: LGTM! Failure callback test properly typed.The test function has complete type annotations for all mock parameters and appropriately validates the failure callback workflow including error payload structure.
nishika26
requested review from
AkhileshNegi
and removed request for
avirajsingh7
Summary
Target issue is #483 and #408
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Test Infrastructure:
Test Coverage:
Code Quality:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.