feat: add testing infrastructure for backend and frontend #532
Conversation
Backend: - Add pytest.ini with test configuration - Add conftest.py with shared fixtures - Add test dependencies (pytest, pytest-asyncio, pytest-cov, httpx) Frontend: - Add vitest.config.ts for test configuration - Add tests/setup.tsx with testing-library setup - Add test dependencies (vitest, testing-library, jsdom) - Add test scripts (test, test:watch, test:coverage)
![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 601489b..9c300db
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (7)
• surfsense_backend/pyproject.toml
• surfsense_backend/pytest.ini
• surfsense_backend/tests/__init__.py
• surfsense_backend/tests/conftest.py
• surfsense_web/package.json
• surfsense_web/tests/setup.tsx
• surfsense_web/vitest.config.ts
There was a problem hiding this comment.
Pull request overview
This PR establishes comprehensive testing infrastructure for both the backend (Python/pytest) and frontend (TypeScript/Vitest) portions of the SurfSense application. The changes lay the groundwork for writing unit and integration tests across the codebase.
Key Changes
- Backend testing setup: pytest configuration with async support, shared test fixtures for database mocking, user objects, and sample data
- Frontend testing setup: Vitest configuration with jsdom environment, comprehensive mocking of Next.js features (router, Image, Link), and browser APIs (localStorage, ResizeObserver, etc.)
- Test dependencies: Added all necessary testing libraries including pytest-asyncio, pytest-cov, httpx for backend; vitest, testing-library, jsdom for frontend
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| surfsense_backend/pytest.ini | Configures pytest with async mode, test discovery patterns, and custom markers |
| surfsense_backend/tests/conftest.py | Provides shared fixtures for database sessions, user objects, and sample test data |
| surfsense_backend/tests/init.py | Marks the tests directory as a Python package |
| surfsense_backend/pyproject.toml | Adds pytest, pytest-asyncio, pytest-cov, and httpx to dev dependencies |
| surfsense_web/vitest.config.ts | Configures Vitest with jsdom, coverage settings, and path aliases |
| surfsense_web/tests/setup.tsx | Sets up test environment with mocks for Next.js components, browser APIs, and testing-library integration |
| surfsense_web/package.json | Adds testing-library packages, vitest, jsdom, and test scripts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }), | ||
| clear: vi.fn(() => { | ||
| localStorageMock.store = {}; | ||
| }), |
There was a problem hiding this comment.
The localStorage mock is missing the length property and key(index) method, which are part of the Storage interface. This could cause tests to fail if code attempts to iterate over localStorage keys using these standard methods. Consider adding:
length: 0,
key: vi.fn((index: number) => {
const keys = Object.keys(localStorageMock.store);
return keys[index] ?? null;
}),And update the length in setItem/removeItem/clear methods.
| }), | |
| }), | |
| get length() { | |
| return Object.keys(localStorageMock.store).length; | |
| }, | |
| key: vi.fn((index: number) => { | |
| const keys = Object.keys(localStorageMock.store); | |
| return keys[index] ?? null; | |
| }), |
| vi.mock("next/navigation", () => ({ | ||
| useRouter: () => ({ | ||
| push: vi.fn(), | ||
| replace: vi.fn(), | ||
| prefetch: vi.fn(), | ||
| back: vi.fn(), | ||
| forward: vi.fn(), | ||
| }), | ||
| usePathname: () => "/", | ||
| useSearchParams: () => new URLSearchParams(), | ||
| useParams: () => ({}), | ||
| })); |
There was a problem hiding this comment.
The Next.js router mock returns static mock functions, but in the afterEach cleanup (line 124), only vi.clearAllMocks() is called without resetting the router state. Consider documenting that tests requiring specific router behavior should use vi.mocked() to access and configure these mocks, or add explicit router mock reset logic if tests need isolated router state.
| session.execute = AsyncMock() | ||
| session.commit = AsyncMock() | ||
| session.rollback = AsyncMock() | ||
| session.refresh = AsyncMock() |
There was a problem hiding this comment.
The session.add method is mocked as a synchronous MagicMock() while other session methods are AsyncMock(). In SQLAlchemy, session.add() is synchronous, but for consistency and to avoid confusion, consider documenting why this is different or using MagicMock() explicitly with a comment explaining the synchronous nature.
| session.refresh = AsyncMock() | |
| session.refresh = AsyncMock() | |
| # Note: session.add() is synchronous even on AsyncSession, so we use MagicMock() here. |
| return [ | ||
| {"role": "user", "content": "Hello, how are you?"}, | ||
| {"role": "assistant", "content": "I'm doing well, thank you!"}, | ||
| {"role": "user", "content": "What is the weather today?"}, | ||
| ] |
There was a problem hiding this comment.
The sample_messages fixture returns a mutable list that could be modified by tests, potentially affecting subsequent tests. Consider using @pytest.fixture(scope="function") explicitly or returning a new copy of the list each time to ensure test isolation. Alternatively, document that tests should not mutate this fixture data.
| return [ | |
| {"role": "user", "content": "Hello, how are you?"}, | |
| {"role": "assistant", "content": "I'm doing well, thank you!"}, | |
| {"role": "user", "content": "What is the weather today?"}, | |
| ] | |
| messages = [ | |
| {"role": "user", "content": "Hello, how are you?"}, | |
| {"role": "assistant", "content": "I'm doing well, thank you!"}, | |
| {"role": "user", "content": "What is the weather today?"}, | |
| ] | |
| return messages.copy() |
| "search_space_id": 1, | ||
| "initial_connectors": [], | ||
| "messages": [], | ||
| } |
There was a problem hiding this comment.
Similar to sample_messages, the sample_chat_create_data fixture returns a mutable dictionary. Consider returning a copy (return {...}) or documenting that tests should not mutate the returned data to prevent potential test interference.
| } | |
| }.copy() |
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_messages() -> list[dict]: |
There was a problem hiding this comment.
Consider using more specific type hints. Instead of list[dict], use list[dict[str, str]] to better specify the structure of chat messages with role and content fields.
| def sample_messages() -> list[dict]: | |
| def sample_messages() -> list[dict[str, str]]: |
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_chat_create_data() -> dict: |
There was a problem hiding this comment.
Consider using a more specific type hint like dict[str, Any] or creating a TypedDict for the chat creation data structure to provide better type safety and documentation.
| [pytest] | ||
| testpaths = tests | ||
| asyncio_mode = auto | ||
| asyncio_default_fixture_loop_scope = function |
There was a problem hiding this comment.
The asyncio_default_fixture_loop_scope configuration option is deprecated in pytest-asyncio 0.23.0+. With pytest-asyncio>=0.24.0 specified in dependencies, consider using asyncio_fixture_scope instead, or rely on the default function scope which is already the standard behavior.
| asyncio_default_fixture_loop_scope = function |
| coverage: { | ||
| provider: "v8", | ||
| reporter: ["text", "json", "html", "lcov"], | ||
| include: ["lib/**/*.{ts,tsx}", "hooks/**/*.{ts,tsx}"], |
There was a problem hiding this comment.
The coverage configuration only includes lib/** and hooks/** but excludes components/**. This may result in incomplete test coverage visibility for the frontend codebase. Consider adding "components/**/*.{ts,tsx}" to the include array to track coverage for UI components as well.
| include: ["lib/**/*.{ts,tsx}", "hooks/**/*.{ts,tsx}"], | |
| include: ["lib/**/*.{ts,tsx}", "hooks/**/*.{ts,tsx}", "components/**/*.{ts,tsx}"], |
| }; | ||
|
|
||
| Object.defineProperty(window, "localStorage", { | ||
| value: localStorageMock, |
There was a problem hiding this comment.
The localStorage mock is missing the writable property in the Object.defineProperty configuration. Consider adding writable: true to allow tests to potentially reassign the mock if needed, maintaining consistency with the window.location mock pattern (line 37).
| value: localStorageMock, | |
| value: localStorageMock, | |
| writable: true, |
1 participant
Backend:
Frontend:
Description
Motivation and Context
FIX #
Screenshots
API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR establishes a comprehensive testing infrastructure for both the backend and frontend. For the backend, it adds
pytestconfiguration with test markers, shared fixtures for mocking database sessions and common objects, and test dependencies. For the frontend, it sets upvitestwith React Testing Library, configures test coverage reporting, adds mock implementations for Next.js routing and components, and includes test scripts for running tests in different modes.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_backend/pyproject.tomlsurfsense_backend/pytest.inisurfsense_backend/tests/__init__.pysurfsense_backend/tests/conftest.pysurfsense_web/package.jsonsurfsense_web/vitest.config.tssurfsense_web/tests/setup.tsx