feat: add comprehensive backend testing infrastructure and CI improvements #524
Conversation
…ments Testing Infrastructure: - Add pytest configuration with asyncio support (pytest.ini) - Add test dependencies to pyproject.toml (pytest, pytest-asyncio, pytest-cov, httpx) - Create test fixtures for async database sessions and mock users (conftest.py) Backend Tests (95 tests): - test_validators.py: Input validation for search_space_id, document_ids, connectors, research_mode, search_mode, top_k, messages, email, url, uuid - test_connector_config.py: Connector configuration validation for all connector types (Tavily, Notion, GitHub, Jira, Slack, Discord, etc.) - test_rbac.py: Security-critical RBAC tests for access control and permissions - test_connector_service.py: ConnectorService resilience and search result transformation - test_llm_service.py: LLM role constants and configuration lookup - test_blocknote_converter.py: Markdown/BlockNote conversion error handling CI/CD Improvements: - Add test.yml workflow for automated backend testing on PRs - Add dependabot.yml for automated dependency updates (pip, npm, github-actions, docker) - Fix docker-publish.yml with proper metadata-action, caching, and release triggers Code Quality: - Improve exception handling with specific exception types (SQLAlchemyError, IntegrityError) - Add structured logging to replace print statements - Tests use pytest.importorskip for graceful skipping when deps unavailable
|
@ankitpasayat is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 9bbea0b..bcda9bf
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (17)
• .github/dependabot.yml
• .github/workflows/docker-publish.yml
• .github/workflows/test.yml
• surfsense_backend/app/routes/chats_routes.py
• surfsense_backend/app/routes/podcasts_routes.py
• surfsense_backend/app/services/connector_service.py
• surfsense_backend/app/tasks/document_processors/file_processors.py
• surfsense_backend/pyproject.toml
• surfsense_backend/pytest.ini
• surfsense_backend/tests/__init__.py
• surfsense_backend/tests/conftest.py
• surfsense_backend/tests/test_blocknote_converter.py
• surfsense_backend/tests/test_connector_config.py
• surfsense_backend/tests/test_connector_service.py
• surfsense_backend/tests/test_llm_service.py
• surfsense_backend/tests/test_rbac.py
• surfsense_backend/tests/test_validators.py
There was a problem hiding this comment.
Pull request overview
This PR establishes a comprehensive testing infrastructure for the backend with 95 automated tests, improves error handling and logging throughout the codebase, and enhances CI/CD workflows with automated testing and dependency management.
Key Changes:
- Added complete pytest test suite covering validators, RBAC security, LLM service, connector service, connector configuration, and BlockNote conversion utilities
- Replaced bare
exceptclauses and print statements with structured logging and specific exception types (SQLAlchemyError,IntegrityError,OSError) - Introduced automated CI/CD workflows for testing, Docker publishing, and Dependabot dependency updates
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| surfsense_backend/tests/test_validators.py | Comprehensive validation tests for search_space_id, document_ids, connectors, research_mode, search_mode, top_k, messages, email, url, and uuid |
| surfsense_backend/tests/test_rbac.py | Security-critical tests for RBAC access control, permissions, ownership checks, and invite code generation |
| surfsense_backend/tests/test_llm_service.py | Tests for LLM role constants, global/user-space config lookup, and role-to-LLM mapping |
| surfsense_backend/tests/test_connector_service.py | Tests for connector service resilience, search result transformation, and search mode behavior |
| surfsense_backend/tests/test_connector_config.py | Validation tests for all connector type configurations (Tavily, Notion, GitHub, Jira, Slack, etc.) |
| surfsense_backend/tests/test_blocknote_converter.py | Error handling tests for markdown/BlockNote conversion with network resilience |
| surfsense_backend/tests/conftest.py | Shared test fixtures for mock sessions, users, search spaces, and sample data |
| surfsense_backend/tests/init.py | Test package initialization |
| surfsense_backend/pytest.ini | Pytest configuration with asyncio support and custom markers |
| surfsense_backend/pyproject.toml | Added test dependencies (pytest, pytest-asyncio, pytest-cov, httpx) |
| surfsense_backend/app/tasks/document_processors/file_processors.py | Improved exception handling with OSError and structured logging for temp file cleanup |
| surfsense_backend/app/services/connector_service.py | Added structured logging and specific SQLAlchemyError handling for counter initialization |
| surfsense_backend/app/routes/podcasts_routes.py | Enhanced error handling with specific exception types and structured logging |
| surfsense_backend/app/routes/chats_routes.py | Improved exception handling with specific types and structured logging across all endpoints |
| .github/workflows/test.yml | New automated testing workflow for backend with coverage reporting |
| .github/workflows/docker-publish.yml | Fixed Docker publishing with proper metadata-action, caching, and release triggers |
| .github/dependabot.yml | Automated dependency updates for pip, npm, GitHub Actions, and Docker |
Comments suppressed due to low confidence (5)
surfsense_backend/app/routes/chats_routes.py:206
- The exception type was changed from generic
ExceptiontoSQLAlchemyError, but this narrows the error handling too much. The original code was handling any unexpected error during chat creation. Non-database errors (e.g., Pydantic validation errors, KeyError, AttributeError, TypeError, etc.) will now bubble up uncaught. Consider keeping a catch-all exception handler afterSQLAlchemyErrorto ensure all errors are properly handled.
except SQLAlchemyError as e:
await session.rollback()
logger.error("Database error during chat creation: %s", e, exc_info=True)
raise HTTPException(
status_code=500,
detail="An unexpected error occurred while creating the chat.",
) from None
surfsense_backend/app/routes/chats_routes.py:390
- The exception type was changed from generic
ExceptiontoSQLAlchemyError, but this narrows the error handling too much. Non-database errors (e.g., Pydantic validation errors, attribute errors, etc.) that could occur during chat updates will now bubble up uncaught. Consider keeping a catch-all exception handler afterSQLAlchemyErrorto ensure all errors are properly handled.
except SQLAlchemyError as e:
await session.rollback()
logger.error("Database error while updating chat %d: %s", chat_id, e, exc_info=True)
raise HTTPException(
status_code=500,
detail="An unexpected error occurred while updating the chat.",
) from None
surfsense_backend/app/routes/chats_routes.py:330
- The exception type was changed from generic
ExceptiontoSQLAlchemyError, but this narrows the error handling too much. Non-database errors (e.g., serialization errors, attribute errors, etc.) that could occur during chat fetching will now bubble up uncaught. Consider keeping a catch-all exception handler afterSQLAlchemyErrorto ensure all errors are properly handled.
except SQLAlchemyError as e:
logger.error("Database error while fetching chat %d: %s", chat_id, e, exc_info=True)
raise HTTPException(
status_code=500,
detail="An unexpected error occurred while fetching the chat.",
) from None
surfsense_backend/app/routes/chats_routes.py:442
- The exception type was changed from generic
ExceptiontoSQLAlchemyError, but this narrows the error handling too much. Non-database errors that could occur during chat deletion will now bubble up uncaught. Consider keeping a catch-all exception handler afterSQLAlchemyErrorto ensure all errors are properly handled.
except SQLAlchemyError as e:
await session.rollback()
logger.error("Database error while deleting chat %d: %s", chat_id, e, exc_info=True)
raise HTTPException(
status_code=500,
detail="An unexpected error occurred while deleting the chat.",
) from None
surfsense_backend/app/routes/podcasts_routes.py:263
- This import of module logging is redundant, as it was previously imported on line 1.
import logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except SQLAlchemyError as e: | ||
| await session.rollback() | ||
| logger.error("Database error while creating podcast: %s", e, exc_info=True) | ||
| raise HTTPException( | ||
| status_code=500, detail="Database error occurred while creating podcast" | ||
| ) from None |
There was a problem hiding this comment.
The removal of the generic except Exception block creates a gap in error handling. If an error occurs that is neither an HTTPException, IntegrityError, nor SQLAlchemyError (e.g., validation errors from Pydantic, KeyError, AttributeError, etc.), it will now bubble up uncaught. This could expose internal error details to clients and cause unhandled 500 errors. Consider keeping a catch-all exception handler at the end to ensure all errors are properly handled and logged.
| except SQLAlchemyError as e: | ||
| logger.error("Database error while fetching chats: %s", e, exc_info=True) | ||
| raise HTTPException( | ||
| status_code=500, detail="An unexpected error occurred while fetching chats." | ||
| ) from None |
There was a problem hiding this comment.
The exception type was changed from generic Exception to SQLAlchemyError, but this narrows the error handling too much. Non-database errors (e.g., serialization errors, attribute errors, etc.) that could occur during chat fetching will now bubble up uncaught. Consider keeping a catch-all exception handler after SQLAlchemyError to ensure all errors are properly handled.
surfsense_backend/app/tasks/document_processors/file_processors.py
Outdated
Show resolved
Hide resolved
- Add vitest configuration with React and jsdom support - Add test setup with Next.js component mocks (Image, Link, Router) - Add utility tests: cn() function, getChatTitleFromMessages() - Add pagination tests: normalizeListResponse() API normalization - Add auth utility tests: token management, authentication state - Add hook tests: useMediaQuery, useIsMobile responsive hooks - Add component tests: Button variants/sizes, Logo accessibility - Update test.yml workflow to run frontend tests on surfsense_web changes - Add test scripts to package.json (test, test:watch, test:coverage) Tests validate: - Tailwind class merging edge cases - API response format normalization - Authentication flow and token handling - Responsive design breakpoints - UI component behavior and accessibility 87 frontend tests passing
- Remove unused imports (AsyncGenerator, HTTPException, rbac functions) - Move logger initialization after all imports per PEP 8
- Add 517 tests covering schemas, routes, services, and utilities - Improve coverage from 14% to 25% - Test files added: - test_schemas.py: Pydantic schema validation tests - test_permissions.py: Permission system tests - test_document_converters.py: Document converter tests - test_page_limit_service.py: Page limit service tests - test_rbac_schemas.py: RBAC schema tests - test_rbac_utils.py: RBAC utility tests - test_routes_search_spaces.py: Search spaces route tests - test_routes_llm_config.py: LLM config route tests - test_routes_documents.py: Documents route tests - test_connector_service_extended.py: Extended connector service tests - test_llm_service_extended.py: Extended LLM service tests - test_agent_configuration.py: Agent configuration tests - test_retrievers.py: Hybrid search retriever tests - test_config.py: Config module tests - test_db_models.py: Database model tests
…d CI
- Add comprehensive unit tests for Celery tasks and connector indexers
- test_celery_tasks.py: Tests for connector task functions
- test_celery_tasks_comprehensive.py: Extended tests for schedule checker,
blocknote migration, and document reindex tasks
- test_connector_indexers_comprehensive.py: Tests for all connector indexers
(Slack, Notion, GitHub, Jira, Confluence, Linear, Discord, etc.)
- Refactor airtable authentication
- Extract refresh_airtable_token to app/utils/connector_auth.py to avoid
circular imports between routes and indexers
- Add re-export in airtable_add_connector_route.py for backward compatibility
- Update CI/CD pipeline for Docker-based testing
- Modify .github/workflows/test.yml to use Docker Compose for tests
- Add docker layer caching for faster CI builds
- Run tests against real PostgreSQL and Redis containers
- Enhance Docker configuration
- Add pytest dependencies to backend Dockerfile
- Add backend-test service profile in docker-compose.yml
- Mount tests directory in backend service for development
- Update .gitignore with comprehensive Python patterns
- Add coverage, cache, and IDE file patterns
- Exclude uv.lock from version control
- Add backend README with testing instructions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 53 changed files in this pull request and generated 28 comments.
Comments suppressed due to low confidence (1)
surfsense_backend/app/routes/podcasts_routes.py:263
- This import of module logging is redundant, as it was previously imported on line 1.
import logging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * 4. Auth headers are correctly constructed | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach } from "vitest"; |
There was a problem hiding this comment.
Unused import vi.
|
|
||
| def test_can_compare_modes(self): | ||
| """Test enum comparison.""" | ||
| assert SearchMode.CHUNKS == SearchMode.CHUNKS |
There was a problem hiding this comment.
Comparison of identical values; use cmath.isnan() if testing for not-a-number.
| assert SearchMode.CHUNKS == SearchMode.CHUNKS |
| mock_config.name = "Test LLM" | ||
| MockLLMConfig.return_value = mock_config | ||
|
|
||
| result = await create_llm_config( |
There was a problem hiding this comment.
Variable result is not used.
| mock_search_space.name = "Test Space" | ||
| MockSearchSpace.return_value = mock_search_space | ||
|
|
||
| result = await create_search_space( |
There was a problem hiding this comment.
Variable result is not used.
| with patch("app.routes.documents_routes.check_permission") as mock_check: | ||
| mock_check.return_value = None | ||
|
|
||
| result = await update_document( |
There was a problem hiding this comment.
Variable result is not used.
| This module tests the permission checking functions used in RBAC. | ||
| """ | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
|
|
||
| def test_all_document_types_are_strings(self): | ||
| """Test all document types have string values.""" | ||
| for doc_type in DocumentType: |
There was a problem hiding this comment.
This for-loop may attempt to iterate over a non-iterable instance of class type.
|
|
||
| def test_all_providers_are_strings(self): | ||
| """Test all providers have string values.""" | ||
| for provider in LiteLLMProvider: |
There was a problem hiding this comment.
This for-loop may attempt to iterate over a non-iterable instance of class type.
|
|
||
| def test_all_connector_types_are_strings(self): | ||
| """Test all connector types have string values.""" | ||
| for conn_type in SearchSourceConnectorType: |
There was a problem hiding this comment.
This for-loop may attempt to iterate over a non-iterable instance of class type.
|
|
||
| def test_permission_values_are_strings(self): | ||
| """Test all permission values are strings.""" | ||
| for perm in Permission: |
There was a problem hiding this comment.
This for-loop may attempt to iterate over a non-iterable instance of class type.
…d remove unused imports - Add catch-all Exception handlers to chats_routes.py and podcasts_routes.py - Remove redundant 'import logging' in podcasts_routes.py - Remove unused imports from test files: - test_celery_tasks.py: asyncio, datetime - test_celery_tasks_comprehensive.py: asyncio, call - test_config.py: pytest, patch, MagicMock, os - test_connector_indexers_comprehensive.py: datetime, timedelta, db imports - test_connector_service_extended.py: patch - test_db_models.py: pytest, mock imports, datetime, uuid4 - test_document_converters.py: AsyncMock, patch - test_page_limit_service.py: patch - test_permissions.py: pytest - Rename unused 'result' variables to '_result' in route tests - Remove unused 'vi' import from auth-utils.test.ts
|
Added a new PR #527 which included these changes as well. Closing this. |
1 participant
Testing Infrastructure:
pytest.ini)pyproject.toml(pytest, pytest-asyncio, pytest-cov, httpx)conftest.py)Backend Tests:
test_validators.py: Input validation for search_space_id, document_ids, connectors, research_mode, search_mode, top_k, messages, email, url, uuidtest_connector_config.py: Connector configuration validation for all connector types (Tavily, Notion, GitHub, Jira, Slack, Discord, etc.)test_rbac.py: Security-critical RBAC tests for access control and permissionstest_connector_service.py: ConnectorService resilience and search result transformationtest_llm_service.py: LLM role constants and configuration lookuptest_blocknote_converter.py: Markdown/BlockNote conversion error handlingCI/CD Improvements:
test.ymlworkflow for automated backend/frontend testing on PRs tomain/devdependabot.ymlfor automated dependency updates (pip, npm, GitHub Actions, Docker)docker-publish.ymlwith proper metadata-action, caching, and release triggersCode Quality:
SQLAlchemyError,IntegrityError,OperationalError)pytest.importorskipfor graceful skipping when dependencies unavailableMotivation and Context
The SurfSense backend previously lacked a formal testing infrastructure, making it difficult to:
This PR addresses these gaps by:
Screenshots
API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR establishes a comprehensive testing infrastructure for the backend with 95 tests covering input validation, connector configuration, RBAC security, service resilience, and LLM configuration. It includes CI/CD improvements with automated test workflows triggered on pull requests, Dependabot configuration for dependency management across Python, npm, GitHub Actions, and Docker, plus enhanced Docker publishing with proper metadata handling, caching, and release triggers. The PR also improves code quality by replacing bare
Exceptionhandlers with specific SQLAlchemy exception types (IntegrityError,OperationalError,SQLAlchemyError) and adding structured logging to replace print statements throughout the backend routes and services.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
.github/workflows/test.yml.github/dependabot.ymlsurfsense_backend/pytest.inisurfsense_backend/pyproject.tomlsurfsense_backend/tests/__init__.pysurfsense_backend/tests/conftest.pysurfsense_backend/tests/test_validators.pysurfsense_backend/tests/test_connector_config.pysurfsense_backend/tests/test_rbac.pysurfsense_backend/tests/test_connector_service.pysurfsense_backend/tests/test_llm_service.pysurfsense_backend/tests/test_blocknote_converter.py.github/workflows/docker-publish.ymlsurfsense_backend/app/routes/chats_routes.pysurfsense_backend/app/routes/podcasts_routes.pysurfsense_backend/app/services/connector_service.pysurfsense_backend/app/tasks/document_processors/file_processors.py