refactor(tests): centralize integration test fixtures and eliminate duplication#1367
refactor(tests): centralize integration test fixtures and eliminate duplication#1367anik120 wants to merge 1 commit intolightspeed-core:mainfrom
Conversation
…uplication Refactors integration tests to eliminate code duplication by centralizing common fixtures, test constants, and mock builder utilities in `tests/integration/conftest.py`. Also adds comprehensive documentation for writing integration tests, so that all coding agents have a central source of truth of best practices. **Motivation** - **Code duplication**: Test files had duplicate fixture definitions (~143 lines across files) - **Manual mock creation**: Tests manually created mock objects with repetitive boilerplate - **Magic strings**: Hard-coded UUIDs and test values scattered across test files - **Missing documentation**: No central guide for writing integration tests **Benefits** - **D.R.Y principle**: Mock creation logic in one place - **Maintainability**: Changes to mock structure only need updates in conftest.py - **Consistency**: All tests use same constants and patterns - **(AI)Developer experience**: Clear documentation for writing new tests - **Less boilerplate**: New tests require less setup code Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
WalkthroughThis PR consolidates integration test infrastructure by centralizing shared fixtures, helper functions, and test constants into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/integration/conftest.py (1)
30-154: Move the reusable builders/constants out ofconftest.py.These
TEST_*values andcreate_mock_*helpers are being imported like normal test utilities, not used as fixtures. Keeping them here makes regular imports depend on pytest’s specialconftest.pyloading rules and grows the autoload surface of a fixture file. A smalltests/integration/helpers.py/constants.pymodule would keepconftest.pyfocused on fixtures.As per coding guidelines,
tests/**/conftest.py: "Useconftest.pyfor shared pytest fixtures".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 30 - 154, The TEST_* constants (TEST_USER_ID, TEST_USERNAME, TEST_CONVERSATION_ID, TEST_REQUEST_ID, TEST_OTHER_USER_ID, TEST_NON_EXISTENT_ID, TEST_MODEL, TEST_PROVIDER, TEST_MODEL_NAME) and the helper functions create_mock_llm_response, create_mock_vector_store_response, and create_mock_tool_call should be moved out of conftest.py into a regular test utility module (e.g., tests.integration.helpers or tests.integration.constants) and imported where needed; update imports so conftest.py only contains fixtures and tests import these symbols from the new module, leaving function/class names unchanged so callers continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/conftest.py`:
- Around line 359-360: Tests are replacing the session factory without
preserving autocommit/autoflush options, causing different flush behavior;
update the test patch to set app.database.session_local =
sessionmaker(autocommit=False, autoflush=False, bind=test_db_engine) after
assigning app.database.engine = test_db_engine so the session factory matches
initialize_database()'s settings (refer to initialize_database(), sessionmaker,
app.database.session_local, and app.database.engine).
- Around line 411-464: The test fixture currently patches
AsyncLlamaStackClientHolder only at app.endpoints.query, but app startup in
src/app/main.py still imports/initializes AsyncLlamaStackClientHolder; update
the mock_llama_stack_client fixture to also patch the symbol where main imports
it (AsyncLlamaStackClientHolder in app.main) or mark the fixture autouse=True so
it runs for all tests that create a TestClient (e.g., integration_http_client);
specifically ensure the fixture patches both
"app.endpoints.query.AsyncLlamaStackClientHolder" and
"app.main.AsyncLlamaStackClientHolder" (or make the fixture autouse) so that
AsyncLlamaStackClientHolder.get_client is replaced before src/app/main.py runs
during TestClient startup.
- Around line 336-362: The in-memory SQLite used by tests isn't shared across
threads because the test engine/session maker isn't configured with StaticPool;
update the test DB engine creation (the code that builds test_db_engine) to use
sqlalchemy.pool.StaticPool and set connect_args={"check_same_thread": False},
and ensure the fixture patch_db_session_fixture still sets
app.database.session_local = sessionmaker(bind=test_db_engine) so all threads
(including background tasks like quota_scheduler) share the single in-memory
connection; reference the test_db_engine creation and
patch_db_session_fixture/sessionmaker/app.database.session_local when making the
change.
In `@tests/integration/endpoints/test_query_integration.py`:
- Around line 1285-1290: The test currently uses create_mock_llm_response(...)
with content="" which still produces one assistant message (an empty-string), so
update the test to simulate a truly empty LLM response by ensuring the assistant
output list is empty: either call create_mock_llm_response with a parameter that
omits the assistant output (e.g., include_assistant=False or
assistant_content=None if supported) or, if that helper doesn't support it,
construct the mock_response manually as a response object whose
assistant/messages array is empty so the test exercises the “no output” branch.
---
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 30-154: The TEST_* constants (TEST_USER_ID, TEST_USERNAME,
TEST_CONVERSATION_ID, TEST_REQUEST_ID, TEST_OTHER_USER_ID, TEST_NON_EXISTENT_ID,
TEST_MODEL, TEST_PROVIDER, TEST_MODEL_NAME) and the helper functions
create_mock_llm_response, create_mock_vector_store_response, and
create_mock_tool_call should be moved out of conftest.py into a regular test
utility module (e.g., tests.integration.helpers or tests.integration.constants)
and imported where needed; update imports so conftest.py only contains fixtures
and tests import these symbols from the new module, leaving function/class names
unchanged so callers continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 75b4638f-c3cc-4827-970b-ad2ead3bf0c3
📒 Files selected for processing (5)
tests/integration/README.mdtests/integration/conftest.pytests/integration/endpoints/test_query_byok_integration.pytests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_streaming_query_byok_integration.py
💤 Files with no reviewable changes (2)
- tests/integration/endpoints/test_query_byok_integration.py
- tests/integration/endpoints/test_streaming_query_byok_integration.py
| @pytest.fixture(name="patch_db_session", autouse=True) | ||
| def patch_db_session_fixture( | ||
| test_db_session: Session, | ||
| test_db_engine: Engine, | ||
| ) -> Generator[Session, None, None]: | ||
| """Initialize database session for integration tests. | ||
|
|
||
| This sets up the global session_local in app.database to use the test database. | ||
| Uses an in-memory SQLite database, isolating tests from production data. | ||
| This fixture is autouse=True, so it automatically applies to all integration tests. | ||
|
|
||
| Args: | ||
| test_db_session: Test database session | ||
| test_db_engine: Test database engine | ||
|
|
||
| Yields: | ||
| The test database Session instance to be used by the test. | ||
| """ | ||
| # Store original values to restore later | ||
| original_engine = app.database.engine | ||
| original_session_local = app.database.session_local | ||
|
|
||
| # Set the test database engine and session maker globally | ||
| app.database.engine = test_db_engine | ||
| app.database.session_local = sessionmaker(bind=test_db_engine) | ||
|
|
||
| yield test_db_session |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "conftest.py" -path "*/integration/*" | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 107
🏁 Script executed:
cat -n tests/integration/conftest.py | head -100Repository: lightspeed-core/lightspeed-stack
Length of output: 4001
🏁 Script executed:
cat -n tests/integration/conftest.py | sed -n '1,400p'Repository: lightspeed-core/lightspeed-stack
Length of output: 15589
🌐 Web query:
SQLAlchemy in-memory SQLite StaticPool poolclass threading shared database
💡 Result:
To use an in-memory SQLite database with SQLAlchemy that is shared across multiple threads, configure the engine with StaticPool as the poolclass and set check_same_thread=False in connect_args. This maintains exactly one global connection shared by all threads. from sqlalchemy import create_engine from sqlalchemy.pool import StaticPool engine = create_engine( "sqlite://", connect_args={"check_same_thread": False}, poolclass=StaticPool, ) This approach is explicitly recommended in the official SQLAlchemy documentation for multi-threaded access to :memory: SQLite databases, as the database exists only within the scope of a single connection which must be shared across threads. StaticPool ensures a single connection is used globally. Note that for standard ":memory:" databases (not URI-based shared cache like "file::memory:?cache=shared"), StaticPool with check_same_thread=False is the standard solution for threading. The SQLite dialect defaults to SingletonThreadPool for single-thread reuse of the same :memory: DB per thread, but not for cross-thread sharing. Requires a recent SQLite version supporting multithreaded :memory: access. Use QueuePool or URI shared cache modes only if needing true concurrent connections to the same DB instance.
Citations:
- 1: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 2: https://sqlalchemy.org/docs/core/pooling.html
- 3: https://docs.sqlalchemy.org/13/dialects/sqlite.html
- 4: https://docs.sqlalchemy.org/14/core/pooling.html
🌐 Web query:
SQLAlchemy sqlite:///:memory: vs sqlite:// StaticPool connection pool
💡 Result:
For sqlite:///:memory: (in-memory SQLite), SQLAlchemy automatically uses SingletonThreadPool by default. This pool maintains one connection per thread, suitable for :memory: databases since each SQLite :memory: connection holds the entire distinct database in memory, and sqlite3 prohibits sharing connections across threads (check_same_thread=True). StaticPool is a different pool class that maintains exactly one single connection for all requests, regardless of threads. It can be explicitly specified via poolclass=StaticPool: from sqlalchemy import create_engine from sqlalchemy.pool import StaticPool engine = create_engine("sqlite:///:memory:", poolclass=StaticPool) Key differences: - SingletonThreadPool: One connection per thread (thread-local). Safe for multi-threaded access to :memory: DB within the same process, as each thread gets its own isolated connection/database. Recommended default for tests/single-process use. - StaticPool: Single shared connection for the entire engine/process. All threads share the same connection, which violates sqlite3's check_same_thread (defaults True for :memory:), likely causing OperationalError on cross-thread use. Only suitable for single-threaded applications or when explicitly setting check_same_thread=False (risky, may lead to corruption). For file-based sqlite:// (on-disk), recent SQLAlchemy (2.0+) defaults to QueuePool, but :memory: specifically uses SingletonThreadPool. Use SingletonThreadPool (default) for standard multi-threaded :memory: testing. Use StaticPool only if single-threaded and needing exactly one connection (rare for :memory:, as each checkout recreates the DB with NullPool). Official docs recommend SingletonThreadPool for :memory: compatibility with SQLite threading model.
Citations:
- 1: https://docs.sqlalchemy.org/14/core/pooling.html
- 2: https://sqlalchemy.org/docs/core/pooling.html
- 3: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 4:
NullPoolis used for in-memory SQLite database created using URI filenames whenSingletonThreadPoolshould have been used instead. sqlalchemy/sqlalchemy#6379 - 5: https://github.com/agronholm/sqlalchemy-1/blob/master/doc/build/core/pooling.rst
- 6: https://docs.sqlalchemy.org/en/14/core/pooling.html
🌐 Web query:
SQLAlchemy SQLite :memory: database shared across threads check_same_thread SingletonThreadPool StaticPool testing
💡 Result:
SQLAlchemy's handling of SQLite :memory: databases in multithreaded environments, particularly for testing, involves specific connection pooling strategies to address SQLite's threading limitations and the ephemeral nature of in-memory databases. Default Behavior for :memory: Databases: SQLAlchemy automatically uses SingletonThreadPool for sqlite:///:memory: databases. This pool maintains one connection per thread, ensuring each thread accesses its own isolated :memory: database. The check_same_thread flag defaults to True within each thread. This prevents thread-safety errors but means threads do not share the same database instance—each has a separate :memory: DB. This is suitable for single-threaded tests or when thread isolation is acceptable. For Shared :memory: Database Across Threads (e.g., true multithreaded testing): To share a single :memory: database across all threads, use StaticPool with check_same_thread=False. StaticPool maintains exactly one global connection shared by all threads. This requires a recent SQLite version supporting multithreaded :memory: access, and external synchronization (e.g., locks) may be needed for concurrent writes due to SQLite's limitations. Example configuration: from sqlalchemy.pool import StaticPool engine = create_engine( "sqlite://", connect_args={"check_same_thread": False}, poolclass=StaticPool ) SingletonThreadPool vs StaticPool: - SingletonThreadPool: One connection per thread (thread-local :memory: DBs). Used by default for :memory:. Not for production, mainly testing. May close excess connections non-deterministically. - StaticPool: Single global connection (shared :memory: DB). For multithreaded shared access. Testing Considerations: For pytest or unit tests, SingletonThreadPool often suffices per test thread/function. For tests simulating multithreading with shared state, StaticPool + check_same_thread=False is recommended. Use fixtures to create fresh engines per test for isolation. Note: High concurrency writes may still fail due to SQLite's single-writer limitation—consider queuing or file-based temp DBs for complex scenarios. Sources confirm this in official docs (SQLite dialect, pooling sections) across versions 1.4-2.x, with examples in FastAPI/SQLAlchemy testing discussions.
Citations:
- 1: http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html
- 2: https://docs.sqlalchemy.org/14/core/pooling.html
- 3: https://docs.sqlalchemy.org/en/20/dialects/sqlite.html
- 4: https://sqlalchemy.org/docs/core/pooling.html
- 5: https://docs.sqlalchemy.org/en/14/dialects/sqlite.html
- 6: https://stackoverflow.com/questions/27910829/sqlalchemy-and-sqlite-shared-cache
- 7: Documented testing approach does not work for SQLite in-memory instances fastapi/fastapi#8825
- 8: https://stackoverflow.com/questions/70028537/multithreading-with-sqlalchemy-orm-and-in-memory-sqlite
- 9: https://stackoverflow.com/questions/15681387/sqlite-works-with-file-dies-with-memory
- 10: https://woteq.com/how-to-test-sqlite-in-memory-databases-using-pytest/
🏁 Script executed:
# Check if there are any async fixtures or threading usage in the test files
cd tests/integration && grep -r "async\|thread\|Thread\|asyncio\|concurrent" --include="*.py" | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 2181
🏁 Script executed:
# Look for how test_db_session is used in actual tests - are multiple threads/connections accessing it?
rg -l "patch_db_session|test_db_session|test_db_engine" tests/integration --type py | head -5Repository: lightspeed-core/lightspeed-stack
Length of output: 159
🏁 Script executed:
# Check for actual threading/async issues in tests that might be affected
rg "TestClient|test_db_session|test_db_engine" tests/integration/endpoints -A 5 | head -60Repository: lightspeed-core/lightspeed-stack
Length of output: 2536
🏁 Script executed:
# Look at one of the actual integration tests to understand usage patterns
cat -n tests/integration/endpoints/test_query_integration.py | head -100Repository: lightspeed-core/lightspeed-stack
Length of output: 4199
🏁 Script executed:
# Check the exact URI behavior - what does "sqlite://" without :memory: do?
rg "sqlite://" tests/ -A 2 -B 2Repository: lightspeed-core/lightspeed-stack
Length of output: 2101
🏁 Script executed:
# Check if there's TestClient usage that might spawn threads
rg "TestClient|integration_http_client" tests/integration/endpoints -B 3 -A 3 | head -80Repository: lightspeed-core/lightspeed-stack
Length of output: 3258
🏁 Script executed:
# Look for any async context or thread spawning in test execution
rg "Thread|threading|concurrent|ProcessPool|QueuePool" tests/ --type pyRepository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Check if TestClient actually spawns threads or runs in same thread
rg "integration_http_client.post|integration_http_client.get" tests/integration/endpoints -B 5 -A 10 | head -100Repository: lightspeed-core/lightspeed-stack
Length of output: 2585
🏁 Script executed:
# Look at app.database to understand how it's used
find . -path "*/app/database.py" -o -path "*/app/database/*" | head -10Repository: lightspeed-core/lightspeed-stack
Length of output: 97
🏁 Script executed:
# Check what happens when patch_db_session is used - does app code run in different thread?
cat -n tests/integration/conftest.py | sed -n '304,334p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1383
🏁 Script executed:
# Check app/database.py to see how the engine is actually used
cat -n src/app/database.py | head -100Repository: lightspeed-core/lightspeed-stack
Length of output: 3819
🏁 Script executed:
# Search for any async request handling or threading in app code
rg "executor|thread|Thread|asyncio|create_task|gather" src/ --type py | grep -v ".pyc" | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 1613
🏁 Script executed:
# Check if FastAPI TestClient actually spawns threads
python3 << 'EOF'
import sys
sys.path.insert(0, '/tmp/search')
# TestClient doesn't spawn threads - it runs in same thread synchronously
# But let's verify the actual pooling behavior with :memory:
print("SQLite :memory: behavior with SingletonThreadPool (default):")
print("- Each thread gets ONE connection, held per-thread")
print("- Each connection to :memory: creates a separate isolated database")
print("- Without StaticPool, threads cannot share the same :memory: DB")
print("")
print("With StaticPool:")
print("- ONE connection shared globally across all threads")
print("- All threads access the same :memory: database")
print("")
print("The review comment is correct about the issue.")
EOFRepository: lightspeed-core/lightspeed-stack
Length of output: 484
🏁 Script executed:
# Final verification: check if tests ever run with multiple threads/workers
rg "pytest.*-n\|pytest.*workers\|xdist\|pytest-xdist" . --include="*.cfg" --include="*.ini" --include="*.toml" --include="*.yaml" --include="*.yml" --include="Makefile"Repository: lightspeed-core/lightspeed-stack
Length of output: 517
🏁 Script executed:
# Check for any pytest plugins that might affect threading
rg "pytest_plugins\|addopts" pyproject.toml pytest.ini setup.cfg 2>/dev/null || echo "No pytest config found"Repository: lightspeed-core/lightspeed-stack
Length of output: 98
🏁 Script executed:
# Verify the difference in SQLite URLs and pooling
cat > /tmp/test_sqlite.py << 'EOF'
from sqlalchemy import create_engine
from sqlalchemy.pool import StaticPool, SingletonThreadPool
# Current setup - uses SingletonThreadPool by default
print("Current: sqlite:///:memory: with check_same_thread=False")
print(" Pool type: SingletonThreadPool (default for :memory:)")
print(" Each thread: separate in-memory database")
print(" Problem: test code and app code in different threads see different DBs")
print()
# Suggested fix
print("Suggested: sqlite:// with StaticPool")
print(" Pool type: StaticPool")
print(" All threads: share single connection to in-memory DB")
print()
# Alternative - what "sqlite://" actually means
print("Note: 'sqlite://' (empty path) with SQLAlchemy typically means:")
print(" - Without :memory:, it defaults to on-disk database")
print(" - With StaticPool, it should still work but creates file-based DB")
print(" - This might not be suitable for test isolation")
EOF
python3 /tmp/test_sqlite.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 251
🏁 Script executed:
# Check if there are any request handlers or dependencies that might spawn threads
rg "Depends|get_db|get_session" tests/integration/endpoints -B 2 -A 2 | head -50Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Verify what "sqlite://" means in SQLAlchemy (without :memory:)
# and check if the suggested fix is using SQLite URI shared cache
rg "sqlite://" . --type py -B 2 -A 2 | grep -A 5 -B 5 "sqlite://" | head -40Repository: lightspeed-core/lightspeed-stack
Length of output: 2789
🏁 Script executed:
# Check if quota_scheduler or other threads access database during tests
rg "quota_scheduler\|get_session\|engine" tests/integration -B 3 -A 3 | head -60Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Look at actual test patterns - do they use integration_http_client with threading concerns?
cat -n tests/integration/endpoints/test_streaming_query_integration.py | head -50Repository: lightspeed-core/lightspeed-stack
Length of output: 2232
🏁 Script executed:
# Verify what happens with "sqlite://" - does it default to :memory: or file-based?
# Check SQLAlchemy documentation behavior
cat > /tmp/check_sqlite_uri.sh << 'EOF'
# According to SQLAlchemy docs, "sqlite://" resolves differently:
# - "sqlite:///:memory:" = in-memory database (three slashes)
# - "sqlite://" = empty path, may not work as intended without specifying a database
# The suggested diff shows just "sqlite://" which is ambiguous
# Let's check if there are any comments in the code about this
grep -r "sqlite://" /repo/tests/integration/conftest.py -B 5 -A 5
EOF
bash /tmp/check_sqlite_uri.shRepository: lightspeed-core/lightspeed-stack
Length of output: 144
🏁 Script executed:
# Final check: does the review's suggested diff actually work in practice?
# Verify the exact suggestion - is "sqlite://" with StaticPool tested anywhere?
find . -name "*.py" -path "*/tests/*" -exec grep -l "StaticPool\|sqlite://" {} \; 2>/dev/null | head -10Repository: lightspeed-core/lightspeed-stack
Length of output: 195
🏁 Script executed:
# Check if there's any documentation about this test setup
cat -n tests/integration/conftest.py | sed -n '218,243p'Repository: lightspeed-core/lightspeed-stack
Length of output: 1031
Add StaticPool to ensure truly shared in-memory test database across threads.
When multiple threads access the in-memory SQLite database, SQLAlchemy's default SingletonThreadPool gives each thread its own isolated :memory: database. This breaks test isolation when background threads (like quota_scheduler) or async tasks access the database—they see a different, empty database than the test thread.
Use StaticPool to maintain exactly one connection shared globally across all threads:
🔧 Suggested fix
from sqlalchemy import create_engine
+from sqlalchemy.pool import StaticPool
engine = create_engine(
- "sqlite:///:memory:",
+ "sqlite://",
echo=False, # Set to True to see SQL queries
connect_args={"check_same_thread": False}, # Allow multi-threaded access
+ poolclass=StaticPool,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/conftest.py` around lines 336 - 362, The in-memory SQLite
used by tests isn't shared across threads because the test engine/session maker
isn't configured with StaticPool; update the test DB engine creation (the code
that builds test_db_engine) to use sqlalchemy.pool.StaticPool and set
connect_args={"check_same_thread": False}, and ensure the fixture
patch_db_session_fixture still sets app.database.session_local =
sessionmaker(bind=test_db_engine) so all threads (including background tasks
like quota_scheduler) share the single in-memory connection; reference the
test_db_engine creation and
patch_db_session_fixture/sessionmaker/app.database.session_local when making the
change.
| app.database.engine = test_db_engine | ||
| app.database.session_local = sessionmaker(bind=test_db_engine) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the initialize_database function
rg -A 5 "def initialize_database" --type pyRepository: lightspeed-core/lightspeed-stack
Length of output: 377
🏁 Script executed:
# Read the conftest.py file around lines 359-360
cat -n tests/integration/conftest.py | head -370 | tail -20Repository: lightspeed-core/lightspeed-stack
Length of output: 870
🏁 Script executed:
# Check for any other references to session_local configuration
rg "session_local.*=" tests/ --type py -B 2 -A 2Repository: lightspeed-core/lightspeed-stack
Length of output: 4901
🏁 Script executed:
# Get the full initialize_database implementation
cat -n src/app/database.py | grep -A 30 "def initialize_database"Repository: lightspeed-core/lightspeed-stack
Length of output: 1521
🏁 Script executed:
# Get more of the initialize_database implementation
cat -n src/app/database.py | sed -n '160,220p'Repository: lightspeed-core/lightspeed-stack
Length of output: 2432
Preserve the autocommit=False, autoflush=False settings when patching the session factory.
initialize_database() creates the session factory with sessionmaker(autocommit=False, autoflush=False, bind=engine) at line 205 of src/app/database.py, but line 360 replaces it without these parameters. SQLAlchemy defaults both to True, changing the flush behavior during integration tests and causing divergence from production.
🔧 Suggested fix
- app.database.session_local = sessionmaker(bind=test_db_engine)
+ app.database.session_local = sessionmaker(
+ autocommit=False,
+ autoflush=False,
+ bind=test_db_engine,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/conftest.py` around lines 359 - 360, Tests are replacing
the session factory without preserving autocommit/autoflush options, causing
different flush behavior; update the test patch to set
app.database.session_local = sessionmaker(autocommit=False, autoflush=False,
bind=test_db_engine) after assigning app.database.engine = test_db_engine so the
session factory matches initialize_database()'s settings (refer to
initialize_database(), sessionmaker, app.database.session_local, and
app.database.engine).
| # Patch AsyncLlamaStackClientHolder where it's commonly used | ||
| mock_holder_class = mocker.patch("app.endpoints.query.AsyncLlamaStackClientHolder") | ||
|
|
||
| mock_client = mocker.AsyncMock() | ||
|
|
||
| # Mock responses.create with default assistant response | ||
| mock_response = mocker.MagicMock(spec=OpenAIResponseObject) | ||
| mock_response.id = "response-123" | ||
|
|
||
| mock_output_item = mocker.MagicMock() | ||
| mock_output_item.type = "message" | ||
| mock_output_item.role = "assistant" | ||
| mock_output_item.content = "This is a test response about Ansible." | ||
| mock_output_item.refusal = None | ||
|
|
||
| mock_response.output = [mock_output_item] | ||
| mock_response.stop_reason = "end_turn" | ||
| mock_response.tool_calls = [] | ||
|
|
||
| mock_usage = mocker.MagicMock() | ||
| mock_usage.input_tokens = 10 | ||
| mock_usage.output_tokens = 5 | ||
| mock_response.usage = mock_usage | ||
|
|
||
| mock_client.responses.create.return_value = mock_response | ||
|
|
||
| # Mock models.list | ||
| mock_model = mocker.MagicMock() | ||
| mock_model.id = "test-provider/test-model" | ||
| mock_model.custom_metadata = { | ||
| "provider_id": "test-provider", | ||
| "model_type": "llm", | ||
| } | ||
| mock_client.models.list.return_value = [mock_model] | ||
|
|
||
| # Mock shields.list (empty by default) | ||
| mock_client.shields.list.return_value = [] | ||
|
|
||
| # Mock vector_stores.list (empty by default) | ||
| mock_vector_stores_response = mocker.MagicMock() | ||
| mock_vector_stores_response.data = [] | ||
| mock_client.vector_stores.list.return_value = mock_vector_stores_response | ||
|
|
||
| # Mock conversations.create | ||
| mock_conversation = mocker.MagicMock() | ||
| mock_conversation.id = "conv_" + "a" * 48 # Proper conv_ format | ||
| mock_client.conversations.create = mocker.AsyncMock(return_value=mock_conversation) | ||
|
|
||
| # Mock version info | ||
| mock_client.inspect.version.return_value = VersionInfo(version="0.2.22") | ||
|
|
||
| # Create mock holder instance | ||
| mock_holder_instance = mock_holder_class.return_value | ||
| mock_holder_instance.get_client.return_value = mock_client |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Holder import/use sites in application and integration tests
rg -n "AsyncLlamaStackClientHolder" src/app tests/integration
# Request-driven integration tests that can execute app startup/lifespan
rg -n "integration_http_client|TestClient\(" tests/integrationRepository: lightspeed-core/lightspeed-stack
Length of output: 9008
🏁 Script executed:
grep -l "TestClient" tests/integration/test_*.py | while read f; do
echo "=== $f ==="
head -100 "$f" | grep -E "def (test_|.*fixture)|patch.*AsyncLlamaStackClientHolder|TestClient"
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 723
🏁 Script executed:
# Check if test_openapi_json.py, test_rh_identity_integration.py, test_middleware_integration.py patch AsyncLlamaStackClientHolder
for f in tests/integration/test_openapi_json.py tests/integration/test_rh_identity_integration.py tests/integration/test_middleware_integration.py; do
echo "=== $f ==="
rg -n "patch.*AsyncLlamaStackClientHolder" "$f" || echo "No AsyncLlamaStackClientHolder patches found"
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 372
🏁 Script executed:
# Check if the conftest fixture is used by any test that creates TestClient
rg -B 5 "TestClient\(" tests/integration | grep -E "def test_|@pytest.fixture|integration_http_client|conftest"Repository: lightspeed-core/lightspeed-stack
Length of output: 417
🏁 Script executed:
sed -n '304,350p' tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1759
🏁 Script executed:
# Check if integration_http_client fixture has a dependency on the mock fixture
rg -A 15 "def integration_http_client_fixture" tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 636
🏁 Script executed:
sed -n '400,470p' tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 2417
🏁 Script executed:
# Find the fixture name for the code block shown in the review
rg -B 5 "mock_holder_class = mocker.patch" tests/integration/conftest.py | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 398
🏁 Script executed:
sed -n '390,415p' tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 994
🏁 Script executed:
# Check if this fixture is autouse=True
rg -B 25 "mock_holder_class = mocker.patch.*query" tests/integration/conftest.py | grep -E "@pytest.fixture|autouse"Repository: lightspeed-core/lightspeed-stack
Length of output: 123
🏁 Script executed:
# Check if mock_llama_stack_client_fixture is autouse
rg -A 3 "@pytest.fixture.*mock_llama_stack_client" tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 220
🏁 Script executed:
# Check which tests use the mock_llama_stack_client fixture
rg "mock_llama_stack_client" tests/integration --type pyRepository: lightspeed-core/lightspeed-stack
Length of output: 13018
🏁 Script executed:
# Check if tests that use TestClient directly (without fixture) rely on mock_llama_stack_client
for f in tests/integration/test_openapi_json.py tests/integration/test_rh_identity_integration.py tests/integration/test_middleware_integration.py; do
echo "=== $f ==="
head -50 "$f" | grep -E "def (test_|.*fixture)|mock_llama_stack_client"
doneRepository: lightspeed-core/lightspeed-stack
Length of output: 317
🏁 Script executed:
# Check if any test files use integration_http_client (which is created from app import in conftest)
rg -l "integration_http_client" tests/integration --type py | head -10Repository: lightspeed-core/lightspeed-stack
Length of output: 227
🏁 Script executed:
# Check the full definitions of test_openapi_json.py and test_middleware_integration.py
head -80 tests/integration/test_openapi_json.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 2654
🏁 Script executed:
# Verify main.py lifespan handler creates AsyncLlamaStackClientHolder before returning
sed -n '50,70p' src/app/main.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 968
🏁 Script executed:
# Check if test_openapi_json.py or test_middleware_integration.py have any fixtures or setup that patches AsyncLlamaStackClientHolder
grep -n "patch\|fixture\|mocker\|mock" tests/integration/test_openapi_json.py | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 190
🏁 Script executed:
grep -n "patch\|fixture\|mocker\|mock" tests/integration/test_middleware_integration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Verify that integration_http_client fixture does NOT include mock_llama_stack_client as a dependency
rg -A 5 "def integration_http_client_fixture" tests/integration/conftest.py | grep -E "def |,"Repository: lightspeed-core/lightspeed-stack
Length of output: 177
🏁 Script executed:
# Check if patch_db_session is autouse=True (to understand how autouse fixtures work in this project)
rg -B 2 "patch_db_session" tests/integration/conftest.py | grep -E "@pytest.fixture|autouse"Repository: lightspeed-core/lightspeed-stack
Length of output: 130
🏁 Script executed:
# Check if tests that use integration_http_client expect mock_llama_stack_client to be available
rg -A 10 "def test_.*integration_http_client" tests/integration/endpoints/test_streaming_query_integration.py | head -30Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Verify test_streaming_query_integration.py - does it use mock_llama_stack_client or integration_http_client or both?
rg "mock_llama_stack_client|integration_http_client" tests/integration/endpoints/test_streaming_query_integration.py | head -10Repository: lightspeed-core/lightspeed-stack
Length of output: 161
🏁 Script executed:
# Check if patch_db_session is the only autouse fixture and if tests actually need mock_llama_stack_client
rg "@pytest.fixture.*autouse" tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 160
🏁 Script executed:
# Check for all autouse fixtures in conftest.py
rg -B 2 "autouse" tests/integration/conftest.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 770
🏁 Script executed:
# Verify: is there a separate autouse mock_llama_stack_client fixture?
rg -A 5 "@pytest.fixture" tests/integration/conftest.py | grep -B 5 "mock_llama_stack_client"Repository: lightspeed-core/lightspeed-stack
Length of output: 359
🏁 Script executed:
# Check the exact fixture definition including autouse parameter
sed -n '390,410p' tests/integration/conftest.py | cat -nRepository: lightspeed-core/lightspeed-stack
Length of output: 949
🏁 Script executed:
# Check if test_openapi_json.py function definitions - does it have mock_llama_stack_client as parameter?
sed -n '60,100p' tests/integration/test_openapi_json.py | grep -E "def test_"Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Full test function signatures in test_openapi_json.py
rg "def test_" tests/integration/test_openapi_json.py -A 1Repository: lightspeed-core/lightspeed-stack
Length of output: 940
🏁 Script executed:
# Check test_rh_identity_integration.py function signatures
head -60 tests/integration/test_rh_identity_integration.py | tail -20Repository: lightspeed-core/lightspeed-stack
Length of output: 674
🏁 Script executed:
# List all test functions in test_rh_identity_integration.py
rg "def test_" tests/integration/test_rh_identity_integration.py -A 1Repository: lightspeed-core/lightspeed-stack
Length of output: 268
🏁 Script executed:
# Check if test_streaming_query_integration.py patches AsyncLlamaStackClientHolder for the test that uses integration_http_client
rg -B 20 "def test_streaming_query_via_http\(" tests/integration/endpoints/test_streaming_query_integration.py | grep -E "def test_|patch|mocker"Repository: lightspeed-core/lightspeed-stack
Length of output: 58
🏁 Script executed:
# Full function definition for test_streaming_query_via_http
sed -n '243,280p' tests/integration/endpoints/test_streaming_query_integration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1403
🏁 Script executed:
# Check if there's a conftest.py at endpoints level that might have autouse fixtures
ls -la tests/integration/endpoints/conftest.py 2>/dev/null || echo "No conftest.py in endpoints"Repository: lightspeed-core/lightspeed-stack
Length of output: 103
🏁 Script executed:
# Verify main.py startup code - check if AsyncLlamaStackClientHolder in main.py can be bypassed
rg -B 5 -A 5 "await AsyncLlamaStackClientHolder" src/app/main.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 568
🏁 Script executed:
# Determine if test_streaming_query_via_http uses mock_llama_stack_client or only integration_http_client
sed -n '248,260p' tests/integration/endpoints/test_streaming_query_integration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 425
🏁 Script executed:
# Check what the test expects - if it mocks the holder or not
rg -A 30 "def test_streaming_query_v2_endpoint_empty_body_returns_422" tests/integration/endpoints/test_streaming_query_integration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1204
🏁 Script executed:
# Confirm: does this test need mocking or does integration_http_client already provide it?
# Look for patches in that test
sed -n '240,275p' tests/integration/endpoints/test_streaming_query_integration.pyRepository: lightspeed-core/lightspeed-stack
Length of output: 1393
Patch src/app/main.py in the shared fixture or make it autouse to prevent real Llama Stack initialization during TestClient startup.
The mock_llama_stack_client fixture currently patches only app.endpoints.query.AsyncLlamaStackClientHolder, but tests using integration_http_client (e.g., test_streaming_query_v2_endpoint_empty_body_returns_422) trigger app startup in src/app/main.py (lines 56-57) before any endpoint-level mocks apply. This allows the real AsyncLlamaStackClientHolder to initialize.
Add patches for all import sites, at minimum app.main.AsyncLlamaStackClientHolder, or mark the fixture autouse=True so it applies to all integration tests that create TestClient instances.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/conftest.py` around lines 411 - 464, The test fixture
currently patches AsyncLlamaStackClientHolder only at app.endpoints.query, but
app startup in src/app/main.py still imports/initializes
AsyncLlamaStackClientHolder; update the mock_llama_stack_client fixture to also
patch the symbol where main imports it (AsyncLlamaStackClientHolder in app.main)
or mark the fixture autouse=True so it runs for all tests that create a
TestClient (e.g., integration_http_client); specifically ensure the fixture
patches both "app.endpoints.query.AsyncLlamaStackClientHolder" and
"app.main.AsyncLlamaStackClientHolder" (or make the fixture autouse) so that
AsyncLlamaStackClientHolder.get_client is replaced before src/app/main.py runs
during TestClient startup.
| mock_response = create_mock_llm_response( | ||
| mocker, | ||
| content="", | ||
| input_tokens=10, | ||
| output_tokens=0, | ||
| ) |
There was a problem hiding this comment.
This no longer simulates a truly empty LLM response.
create_mock_llm_response() always adds one assistant output item. Passing content="" gives you an empty-string message, not a response with no output, so this can miss the branch the test name/docstring describe.
🔧 Suggested fix
mock_response = create_mock_llm_response(
mocker,
content="",
input_tokens=10,
output_tokens=0,
)
mock_response.id = "response-empty"
+ mock_response.output = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mock_response = create_mock_llm_response( | |
| mocker, | |
| content="", | |
| input_tokens=10, | |
| output_tokens=0, | |
| ) | |
| mock_response = create_mock_llm_response( | |
| mocker, | |
| content="", | |
| input_tokens=10, | |
| output_tokens=0, | |
| ) | |
| mock_response.id = "response-empty" | |
| mock_response.output = [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/endpoints/test_query_integration.py` around lines 1285 -
1290, The test currently uses create_mock_llm_response(...) with content=""
which still produces one assistant message (an empty-string), so update the test
to simulate a truly empty LLM response by ensuring the assistant output list is
empty: either call create_mock_llm_response with a parameter that omits the
assistant output (e.g., include_assistant=False or assistant_content=None if
supported) or, if that helper doesn't support it, construct the mock_response
manually as a response object whose assistant/messages array is empty so the
test exercises the “no output” branch.
1 participant
Description
Refactors integration tests to eliminate code duplication by centralizing common fixtures, test constants, and mock builder utilities in
tests/integration/conftest.py. Also adds comprehensive documentation for writing integration tests, so that all coding agents have a central source of truth of best practices.Motivation
Benefits
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Documentation
Tests