From 8f2b25f0e07e9b44c575e83bb752e99e2018782e Mon Sep 17 00:00:00 2001 From: phernandez Date: Mon, 13 Apr 2026 14:23:36 -0500 Subject: [PATCH] test(core): stabilize postgres fixtures Signed-off-by: phernandez --- AGENTS.md | 12 ++-- justfile | 6 +- pyproject.toml | 6 ++ src/basic_memory/repository/repository.py | 6 +- test-int/conftest.py | 81 +++++++++++++++++------ test-int/semantic/conftest.py | 71 ++++++++++++++------ tests/api/v2/conftest.py | 12 +++- tests/cli/conftest.py | 14 +++- tests/conftest.py | 72 +++++++++++++++----- tests/mcp/conftest.py | 17 ++++- 10 files changed, 221 insertions(+), 76 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 239bcc12..2fea45b2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,16 +22,16 @@ See the [README.md](README.md) file for a project overview. - Run unit tests (Postgres): `just test-unit-postgres` - Run integration tests (SQLite): `just test-int-sqlite` - Run integration tests (Postgres): `just test-int-postgres` -- Run impacted tests: `just testmon` (pytest-testmon) +- Run impacted tests: `just testmon` (pytest-testmon; only tests affected by changed code) - Run MCP smoke test: `just test-smoke` -- Fast local loop: `just fast-check` +- Fast local loop: `just fast-check` (default iteration flow) - Local consistency check: `just doctor` - Generate HTML coverage: `just coverage` - Single test: `pytest tests/path/to/test_file.py::test_function_name` - Run benchmarks: `pytest test-int/test_sync_performance_benchmark.py -v -m "benchmark and not slow"` - Lint: `just lint` or `ruff check . --fix` -- Type check: `just typecheck` or `uv run pyright` -- Type check (supplemental): `just typecheck-ty` or `uv run ty check src/` +- Type check: `just typecheck` or `uv run ty check src tests test-int` +- Type check (pyright): `just typecheck-pyright` or `uv run pyright` - Format: `just format` or `uv run ruff format .` - Run all code checks: `just check` (runs lint, format, typecheck, test) - Create db migration: `just migration "Your migration message"` @@ -48,10 +48,12 @@ See the [README.md](README.md) file for a project overview. ### Code/Test/Verify Loop (fast path) 1) **Code:** make changes. -2) **Test:** `just fast-check` (lint/format/typecheck + impacted tests + MCP smoke). +2) **Test:** `just fast-check` (lint/format/typecheck + pytest-testmon impacted tests for changed code). 3) **Verify:** `just doctor` (end-to-end file ↔ DB loop in a temp project). 4) **Full gate (when needed):** `just test` or `just check` for SQLite + Postgres. +Run `just test-smoke` when you specifically need the MCP smoke flow. + If testmon is “cold,” the first run may be long. Subsequent runs get much faster. ### Test Structure diff --git a/justfile b/justfile index 04c5b25a..c029d677 100644 --- a/justfile +++ b/justfile @@ -62,20 +62,20 @@ test-int-postgres: fi # Run tests impacted by recent changes (requires pytest-testmon) +# Pass paths or node ids after `just testmon` to limit the candidate set further. testmon *args: - BASIC_MEMORY_ENV=test uv run pytest -p pytest_mock -v --no-cov --testmon --testmon-forceselect {{args}} + BASIC_MEMORY_ENV=test uv run pytest -p pytest_mock -v --no-cov --testmon {{args}} # Run MCP smoke test (fast end-to-end loop) test-smoke: BASIC_MEMORY_ENV=test uv run pytest -p pytest_mock -v --no-cov -m smoke test-int/mcp/test_smoke_integration.py -# Fast local loop: lint, format, typecheck, impacted tests +# Fast local loop: lint, format, typecheck, impacted tests via pytest-testmon fast-check: just fix just format just typecheck just testmon - just test-smoke # Reset Postgres test database (drops and recreates schema) # Useful when Alembic migration state gets out of sync during development diff --git a/pyproject.toml b/pyproject.toml index 207b0365..674d8310 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -71,6 +71,12 @@ addopts = "--cov=basic_memory --cov-report term-missing" testpaths = ["tests", "test-int"] asyncio_mode = "strict" asyncio_default_fixture_loop_scope = "function" +filterwarnings = [ + "ignore:The @wait_container_is_ready decorator is deprecated.*:DeprecationWarning:testcontainers\\.core\\.waiting_utils", + "ignore:The default datetime adapter is deprecated as of Python 3\\.12.*:DeprecationWarning:aiosqlite\\.core", + "ignore:codecs\\.open\\(\\) is deprecated\\. Use open\\(\\) instead\\.:DeprecationWarning:frontmatter", + "ignore:Parsing dates involving a day of month without a year specified is ambiguous.*:DeprecationWarning:dateparser\\.utils\\.strptime", +] markers = [ "benchmark: Performance benchmark tests (deselect with '-m \"not benchmark\"')", "slow: Slow-running tests (deselect with '-m \"not slow\"')", diff --git a/src/basic_memory/repository/repository.py b/src/basic_memory/repository/repository.py index 558321d8..88684119 100644 --- a/src/basic_memory/repository/repository.py +++ b/src/basic_memory/repository/repository.py @@ -280,9 +280,9 @@ class Repository[T: Base]: if isinstance(entity_data, dict): update_data = cast(dict[str, Any], entity_data) - for key, value in update_data.items(): - if key in self.valid_columns: - setattr(entity, key, value) + for key in self.valid_columns: + if key in update_data: + setattr(entity, key, update_data[key]) elif isinstance(entity_data, self.Model): for column in self.valid_columns: diff --git a/test-int/conftest.py b/test-int/conftest.py index a49763d3..4a961063 100644 --- a/test-int/conftest.py +++ b/test-int/conftest.py @@ -57,7 +57,12 @@ import pytest import pytest_asyncio from pathlib import Path from sqlalchemy import text -from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import ( + AsyncEngine, + AsyncSession, + async_sessionmaker, + create_async_engine, +) from sqlalchemy.pool import NullPool from testcontainers.postgres import PostgresContainer @@ -118,6 +123,21 @@ def postgres_container(db_backend): yield postgres +@pytest_asyncio.fixture(autouse=True) +async def cleanup_global_db_after_test() -> AsyncGenerator[None, None]: + """Close any module-level DB engine created outside fixture ownership.""" + yield + + # Trigger: integration tests invoke CLI/MCP routes through the production + # client fallback, bypassing this file's engine_factory fixture. + # Why: those fallback engines live in basic_memory.db module state and can + # otherwise leave a non-daemon aiosqlite worker alive after pytest finishes. + # Outcome: every test boundary becomes a cleanup point for fallback engines. + from basic_memory import db + + await db.shutdown_db() + + POSTGRES_EPHEMERAL_TABLES = [ "search_vector_embeddings", "search_vector_chunks", @@ -182,12 +202,36 @@ async def _reset_postgres_integration_schema(engine) -> None: ) +@pytest_asyncio.fixture(scope="session", loop_scope="session") +async def postgres_engine( + db_backend: Literal["sqlite", "postgres"], postgres_container +) -> AsyncGenerator[AsyncEngine | None, None]: + """Create the shared Postgres engine once per integration test session.""" + if db_backend != "postgres": + yield None + return + + sync_url = _resolve_postgres_sync_url(postgres_container) + async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") + engine = create_async_engine( + async_url, + echo=False, + poolclass=NullPool, + ) + + try: + yield engine + finally: + await engine.dispose() + + @pytest_asyncio.fixture async def engine_factory( app_config, config_manager, db_backend: Literal["sqlite", "postgres"], postgres_container, + postgres_engine, tmp_path, ) -> AsyncGenerator[tuple, None]: """Create engine and session factory for the configured database backend.""" @@ -195,18 +239,17 @@ async def engine_factory( from basic_memory import db if db_backend == "postgres": - # Postgres mode using testcontainers - sync_url = _resolve_postgres_sync_url(postgres_container) - async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") + assert postgres_engine is not None - engine = create_async_engine( - async_url, - echo=False, - poolclass=NullPool, - ) + # Trigger: full-stack MCP/CLI tests exercise sync/indexing code that can + # recover from DB errors by rolling back and opening later scoped sessions. + # Why: one savepoint-backed connection is too brittle for that flow. + # Outcome: reuse the engine, but reset rows/schema before each test and + # let app code use normal transaction boundaries. + await _reset_postgres_integration_schema(postgres_engine) session_maker = async_sessionmaker( - bind=engine, + bind=postgres_engine, class_=AsyncSession, expire_on_commit=False, autoflush=False, @@ -214,17 +257,17 @@ async def engine_factory( # Set module-level state to prevent MCP lifespan from re-initializing # This ensures get_or_create_db() sees an existing engine and skips initialization - db._engine = engine + db._engine = postgres_engine db._session_maker = session_maker - await _reset_postgres_integration_schema(engine) - - yield engine, session_maker - - # Clean up module-level state - await engine.dispose() - db._engine = None - db._session_maker = None + try: + yield postgres_engine, session_maker + finally: + # Clean up module-level state + if db._engine is postgres_engine: + db._engine = None + if db._session_maker is session_maker: + db._session_maker = None else: # SQLite: Create fresh database (fast with tmp files) diff --git a/test-int/semantic/conftest.py b/test-int/semantic/conftest.py index 9d29a136..bca78ffb 100644 --- a/test-int/semantic/conftest.py +++ b/test-int/semantic/conftest.py @@ -8,6 +8,7 @@ SearchService for each (backend, provider) combination. from __future__ import annotations import os +from collections.abc import AsyncGenerator from dataclasses import dataclass from pathlib import Path @@ -15,7 +16,12 @@ import pytest import pytest_asyncio from dotenv import load_dotenv from sqlalchemy import text -from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import ( + AsyncEngine, + AsyncSession, + async_sessionmaker, + create_async_engine, +) from sqlalchemy.pool import NullPool from testcontainers.postgres import PostgresContainer @@ -152,25 +158,8 @@ async def sqlite_engine_factory(tmp_path): yield engine, session_maker -@pytest_asyncio.fixture -async def postgres_engine_factory(pgvector_container): - """Create a Postgres engine + session factory with pgvector extension.""" - if pgvector_container is None: - yield None - return - - sync_url = pgvector_container.get_connection_url() - async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") - - engine = create_async_engine(async_url, echo=False, poolclass=NullPool) - session_maker = async_sessionmaker( - bind=engine, - class_=AsyncSession, - expire_on_commit=False, - autoflush=False, - ) - - # Create schema from scratch for each test +async def _reset_postgres_semantic_schema(engine: AsyncEngine) -> None: + """Reset the semantic Postgres schema to a clean baseline.""" async with engine.begin() as conn: await conn.execute(text("DROP TABLE IF EXISTS search_vector_embeddings CASCADE")) await conn.execute(text("DROP TABLE IF EXISTS search_vector_chunks CASCADE")) @@ -182,9 +171,47 @@ async def postgres_engine_factory(pgvector_container): await conn.execute(CREATE_POSTGRES_SEARCH_INDEX_METADATA) await conn.execute(CREATE_POSTGRES_SEARCH_INDEX_PERMALINK) - yield engine, session_maker - await engine.dispose() +@pytest_asyncio.fixture(scope="session", loop_scope="session") +async def postgres_engine(pgvector_container) -> AsyncGenerator[AsyncEngine | None, None]: + """Create the shared semantic Postgres engine once per test session.""" + if pgvector_container is None: + yield None + return + + sync_url = pgvector_container.get_connection_url() + async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") + + engine = create_async_engine(async_url, echo=False, poolclass=NullPool) + + try: + yield engine + finally: + await engine.dispose() + + +@pytest_asyncio.fixture +async def postgres_engine_factory(postgres_engine): + """Create a Postgres session factory isolated by schema reset.""" + if postgres_engine is None: + yield None + return + + # Trigger: semantic provider combos create and rebuild vector tables. + # Why: the main suite showed savepoint-bound shared connections can get + # poisoned by app-level rollback/recovery paths. + # Outcome: keep the pgvector engine warm, but reset schema per test and let + # repository code use normal Postgres transactions. + await _reset_postgres_semantic_schema(postgres_engine) + + session_maker = async_sessionmaker( + bind=postgres_engine, + class_=AsyncSession, + expire_on_commit=False, + autoflush=False, + ) + + yield postgres_engine, session_maker # --- Embedding provider factories --- diff --git a/tests/api/v2/conftest.py b/tests/api/v2/conftest.py index 633415d6..02725969 100644 --- a/tests/api/v2/conftest.py +++ b/tests/api/v2/conftest.py @@ -14,13 +14,21 @@ from basic_memory.models import Project @pytest_asyncio.fixture -async def app(test_config, engine_factory, app_config) -> FastAPI: +async def app(test_config, engine_factory, app_config) -> AsyncGenerator[FastAPI, None]: """Create FastAPI test application.""" from basic_memory.api.app import app + previous_overrides = dict(app.dependency_overrides) app.dependency_overrides[get_app_config] = lambda: app_config app.dependency_overrides[get_engine_factory] = lambda: engine_factory - return app + try: + yield app + finally: + # Trigger: the FastAPI app is a module-level singleton shared across tests. + # Why: dependency overrides that capture a per-test engine can leak into + # later CLI/MCP tests and create connections outside fixture ownership. + # Outcome: each API test leaves the shared app exactly as it found it. + app.dependency_overrides = previous_overrides @pytest_asyncio.fixture diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py index 6d1c635c..5f982ec8 100644 --- a/tests/cli/conftest.py +++ b/tests/cli/conftest.py @@ -38,13 +38,23 @@ def isolated_home(tmp_path, monkeypatch) -> Path: @pytest_asyncio.fixture -async def app(app_config, project_config, engine_factory, test_config, aiolib) -> FastAPI: +async def app( + app_config, project_config, engine_factory, test_config, aiolib +) -> AsyncGenerator[FastAPI, None]: """Create test FastAPI application.""" app = fastapi_app + previous_overrides = dict(app.dependency_overrides) app.dependency_overrides[get_app_config] = lambda: app_config app.dependency_overrides[get_project_config] = lambda: project_config app.dependency_overrides[get_engine_factory] = lambda: engine_factory - return app + try: + yield app + finally: + # Trigger: CLI tests share the module-level FastAPI app with API/MCP tests. + # Why: leaving per-test dependency overrides installed lets later commands + # talk to stale engines that no cleanup fixture owns. + # Outcome: keep CLI app wiring isolated to the requesting test. + app.dependency_overrides = previous_overrides @pytest_asyncio.fixture diff --git a/tests/conftest.py b/tests/conftest.py index d97cb1ad..23d82137 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -146,7 +146,7 @@ def _resolve_postgres_sync_url(postgres_container) -> str: async def _reset_postgres_test_schema(engine: AsyncEngine, async_url: str) -> None: - """Restore the shared Postgres schema to a clean baseline before each test.""" + """Restore the shared Postgres schema to a clean baseline.""" from basic_memory.models.search import ( CREATE_POSTGRES_SEARCH_INDEX_FTS, CREATE_POSTGRES_SEARCH_INDEX_METADATA, @@ -183,6 +183,29 @@ async def _reset_postgres_test_schema(engine: AsyncEngine, async_url: str) -> No command.stamp(_postgres_alembic_config(async_url), "head") +@pytest_asyncio.fixture(scope="session", loop_scope="session") +async def postgres_engine( + db_backend, postgres_container +) -> AsyncGenerator[AsyncEngine | None, None]: + """Create the shared Postgres engine once per test session.""" + if db_backend != "postgres": + yield None + return + + sync_url = _resolve_postgres_sync_url(postgres_container) + async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") + engine = create_async_engine( + async_url, + echo=False, + poolclass=NullPool, + ) + + try: + yield engine + finally: + await engine.dispose() + + @pytest.fixture def anyio_backend(): return "asyncio" @@ -194,6 +217,19 @@ def suppress_logfire_no_config_warning(monkeypatch) -> None: monkeypatch.setenv("LOGFIRE_IGNORE_NO_CONFIG", "1") +@pytest_asyncio.fixture(autouse=True) +async def cleanup_global_db_after_test() -> AsyncGenerator[None, None]: + """Close any module-level DB engine created outside fixture ownership.""" + yield + + # Trigger: a test exercises production fallback routing instead of the + # per-test engine fixture. + # Why: that path stores an engine in basic_memory.db module state, and + # a later fixture can overwrite the reference before it is disposed. + # Outcome: close straggler aiosqlite worker threads before the loop closes. + await db.shutdown_db() + + @pytest.fixture def project_root() -> Path: return Path(__file__).parent.parent @@ -293,6 +329,7 @@ async def engine_factory( config_manager, db_backend, postgres_container, + postgres_engine, ) -> AsyncGenerator[tuple[AsyncEngine, async_sessionmaker[AsyncSession]], None]: """Engine factory for SQLite or Postgres tests. @@ -306,19 +343,20 @@ async def engine_factory( ) if db_backend == "postgres": - # Postgres mode using testcontainers - # Get async connection URL (asyncpg driver - same as production) + assert postgres_engine is not None sync_url = _resolve_postgres_sync_url(postgres_container) async_url = sync_url.replace("postgresql+psycopg2", "postgresql+asyncpg") - engine = create_async_engine( - async_url, - echo=False, - poolclass=NullPool, # NullPool for better test isolation - ) + # Trigger: Basic Memory sync/indexing paths can catch database errors, + # roll back, and continue through new scoped sessions. + # Why: binding every scoped session to one savepoint-backed connection can + # leave that connection in PendingRollbackError state for later sessions. + # Outcome: keep the session-scoped engine, but use normal Postgres + # transaction semantics and reset rows/schema between tests. + await _reset_postgres_test_schema(postgres_engine, async_url) session_maker = async_sessionmaker( - bind=engine, + bind=postgres_engine, class_=AsyncSession, expire_on_commit=False, autoflush=False, @@ -328,16 +366,16 @@ async def engine_factory( # Some codepaths (e.g. app initialization / MCP lifespan) call db.get_or_create_db(), # which would otherwise create a separate engine and run migrations, conflicting with # our test-created schema (and causing DuplicateTableError). - db._engine = engine + db._engine = postgres_engine db._session_maker = session_maker - await _reset_postgres_test_schema(engine, async_url) - - yield engine, session_maker - - await engine.dispose() - db._engine = None - db._session_maker = None + try: + yield postgres_engine, session_maker + finally: + if db._engine is postgres_engine: + db._engine = None + if db._session_maker is session_maker: + db._session_maker = None else: # SQLite mode db_type = DatabaseType.MEMORY diff --git a/tests/mcp/conftest.py b/tests/mcp/conftest.py index db77585f..7e3fe704 100644 --- a/tests/mcp/conftest.py +++ b/tests/mcp/conftest.py @@ -1,6 +1,7 @@ """Tests for the MCP server implementation using FastAPI TestClient.""" -from typing import Any, AsyncGenerator, cast +from collections.abc import AsyncGenerator, Generator +from typing import Any, cast import pytest import pytest_asyncio @@ -20,13 +21,23 @@ def mcp() -> FastMCP: @pytest.fixture(scope="function") -def app(app_config, project_config, engine_factory, config_manager) -> FastAPI: +def app( + app_config, project_config, engine_factory, config_manager +) -> Generator[FastAPI, None, None]: """Create test FastAPI application.""" app = fastapi_app + previous_overrides = dict(app.dependency_overrides) app.dependency_overrides[get_app_config] = lambda: app_config app.dependency_overrides[get_project_config] = lambda: project_config app.dependency_overrides[get_engine_factory] = lambda: engine_factory - return app + try: + yield app + finally: + # Trigger: the FastAPI app is a module-level singleton shared across tests. + # Why: stale dependency overrides can hold onto a disposed per-test engine + # and reopen SQLite connections during later unrelated tests. + # Outcome: restore the shared app's override table after each MCP test. + app.dependency_overrides = previous_overrides @pytest_asyncio.fixture(scope="function")