Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive testing infrastructure for the backend, including unit tests, security tests, and stress/load testing capabilities. The changes eliminate the dependency on pytest-asyncio by implementing a custom async test runner, add detailed testing documentation in Chinese, and refactor the database session management to use lazy initialization.
Key Changes:
- Removed
pytest-asynciodependency and implemented custom async test execution viapytest_pyfunc_callhook - Added new test suites for security (JWT validation), database utilities, and configuration
- Created a standalone stress testing script supporting both ASGI in-process and live HTTP testing
- Refactored database session management to use lazy initialization with
@lru_cachedecorators
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/testing.md | Comprehensive Chinese documentation covering unit, security, and stress testing procedures |
| backend/tests/conftest.py | Removed pytest-asyncio dependency, implemented custom async test runner with manual event loop management |
| backend/tests/test_security.py | New security regression tests for JWT expiration, algorithm manipulation, and missing user validation |
| backend/tests/test_db_utils.py | New unit tests for database utility functions with comprehensive retry/grant logic coverage |
| backend/tests/test_config.py | New configuration tests for settings validation and environment variable handling |
| backend/scripts/stress_test.py | New stress test script with ASGI in-process testing and live HTTP support, includes latency percentile calculations |
| backend/pytest.ini | Added asyncio marker configuration for test categorization |
| backend/app/db/utils.py | Changed early return to break statement to ensure grant/warmup logic always executes |
| backend/app/db/session.py | Refactored to lazy initialization pattern using lru_cache for engine and sessionmaker |
| README.md | Updated test execution commands and added stress testing examples |
Comments suppressed due to low confidence (1)
backend/scripts/stress_test.py:22
- Import of 'Any' is not used.
from typing import Any, AsyncIterator
| client: AsyncClient, | ||
| request_count: int, | ||
| concurrency: int, | ||
| make_request, |
There was a problem hiding this comment.
Missing type annotation for the 'make_request' parameter. Consider adding a type hint such as 'Callable[[AsyncClient], Awaitable[int]]' to improve code clarity and enable better type checking.
| status = await make_request(client) | ||
| except Exception: | ||
| status = 0 | ||
| elapsed_ms = (time.perf_counter() - start) * 1000 |
There was a problem hiding this comment.
The 'lock' is only used to protect 'status_counts' and the 'ok'/'errors' counters, but 'latencies_ms[i]' is written without lock protection on line 134. While this is actually safe because each task writes to a unique index 'i', the inconsistent locking pattern could be confusing. Consider adding a comment explaining why 'latencies_ms[i]' doesn't need lock protection (unique index per task).
| elapsed_ms = (time.perf_counter() - start) * 1000 | |
| elapsed_ms = (time.perf_counter() - start) * 1000 | |
| # This write is safe without the lock: each task gets a unique index i, | |
| # so no two tasks ever write to the same latencies_ms element. |
| import time | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import Any, AsyncIterator |
There was a problem hiding this comment.
The 'Any' import on line 22 is unused. Consider removing it to keep imports clean.
| from typing import Any, AsyncIterator | |
| from typing import AsyncIterator |
| @lru_cache | ||
| def get_engine() -> AsyncEngine: | ||
| """Create (and memoize) the async engine. | ||
|
|
||
| Lazily initializing the engine keeps import-time side effects minimal and | ||
| allows test suites to override DB dependencies without requiring every | ||
| production DB driver to be installed. | ||
| """ | ||
|
|
||
| return create_async_engine(settings.async_database_uri, echo=False) |
There was a problem hiding this comment.
Using '@lru_cache' on 'get_engine' and 'get_sessionmaker' may cause issues in multi-threaded or multi-process environments. The SQLAlchemy async engine is not inherently thread-safe across process boundaries (e.g., when using multiprocessing or after fork). Consider using '@lru_cache(maxsize=1)' for clarity, or document that this service should not be used with multiprocessing/forking servers without proper engine disposal between forks.
| assert settings.frontend_oauth_redirect_url == "http://localhost:3000/auth/sso" | ||
|
|
||
|
|
||
| def test_settings_media_path_prefers_env_override(monkeypatch) -> None: |
There was a problem hiding this comment.
Missing type annotation for the 'monkeypatch' parameter. Should be 'monkeypatch: pytest.MonkeyPatch' for consistency with the type annotations used in test_db_utils.py.
| engine = create_async_engine(TEST_DATABASE_URL, future=True, connect_args={"uri": True}) | ||
|
|
||
| if TEST_MEDIA_ROOT.exists(): | ||
| shutil.rmtree(TEST_MEDIA_ROOT) | ||
| TEST_MEDIA_ROOT.mkdir(parents=True, exist_ok=True) | ||
| async def setup() -> AsyncClient: | ||
| if TEST_MEDIA_ROOT.exists(): | ||
| shutil.rmtree(TEST_MEDIA_ROOT) | ||
| TEST_MEDIA_ROOT.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| async with engine.begin() as conn: | ||
| await conn.run_sync(Base.metadata.create_all) | ||
| async with engine.begin() as conn: | ||
| await conn.run_sync(Base.metadata.create_all) | ||
|
|
||
| session_factory = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) | ||
| session_factory = async_sessionmaker(engine, expire_on_commit=False, class_=AsyncSession) | ||
|
|
||
| async def override_get_db() -> AsyncIterator[AsyncSession]: | ||
| async with session_factory() as session: | ||
| yield session | ||
| async def override_get_db() -> AsyncIterator[AsyncSession]: | ||
| async with session_factory() as session: | ||
| yield session | ||
|
|
||
| app.dependency_overrides[get_db] = override_get_db | ||
| app.dependency_overrides[get_db] = override_get_db | ||
|
|
||
| transport = ASGITransport(app=app) | ||
| async with AsyncClient(transport=transport, base_url="http://test") as client: | ||
| yield client | ||
| transport = ASGITransport(app=app) | ||
| return AsyncClient(transport=transport, base_url="http://test") | ||
|
|
||
| app.dependency_overrides.clear() | ||
| await engine.dispose() | ||
| shutil.rmtree(TEST_MEDIA_ROOT, ignore_errors=True) No newline at end of file | ||
| client = event_loop.run_until_complete(setup()) | ||
|
|
||
| try: | ||
| yield client | ||
| finally: | ||
| async def teardown() -> None: | ||
| await client.aclose() | ||
| app.dependency_overrides.clear() | ||
| await engine.dispose() | ||
| shutil.rmtree(TEST_MEDIA_ROOT, ignore_errors=True) | ||
|
|
||
| event_loop.run_until_complete(teardown()) |
There was a problem hiding this comment.
The 'engine' variable is created outside the 'setup' function but is accessed in the 'teardown' function. This could lead to issues if multiple tests run in parallel or if the fixture is called multiple times. Consider moving the engine creation inside the 'setup' function and returning both the client and engine as a tuple, or storing the engine in a way that ensures proper cleanup.
No description provided.