test: move real-DB store tests to tests/integration/#43
test: move real-DB store tests to tests/integration/#43aivong-openhands wants to merge 5 commits intomainfrom
Conversation
test_org_store.py and test_api_key_store.py exercise real SQLite
database behavior (queries, constraints, transactions) via
async_session_maker — that is integration-level testing, not unit
testing. Moving them out of tests/unit/ makes the boundary explicit.
Changes:
- Add tests/integration/ with __init__.py and conftest.py (DB fixtures)
- git mv tests/unit/test_org_store.py → tests/integration/
- git mv tests/unit/test_api_key_store.py → tests/integration/
- git mv tests/unit/storage/test_api_key_store.py
→ tests/integration/test_api_key_store_system_keys.py
- Register 'integration' pytest marker in pyproject.toml
Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an enterprise integration test workflow and supporting pytest configuration and fixtures: a new GitHub Actions workflow to run integration tests, an Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Ubuntu Runner
participant Py as Python/Poetry
participant Pytest as pytest
participant Art as Artifact Storage
GH->>Runner: start workflow (push/PR)
Runner->>Py: setup Python 3.12, install Poetry
Runner->>Py: poetry install --with dev,test (./enterprise)
Runner->>Pytest: run pytest ./enterprise/tests/integration --forked -n auto (COVERAGE_FILE set)
Pytest->>Runner: generate coverage file .coverage.enterprise.integration.<pyver>
Runner->>Art: upload artifact coverage-enterprise-integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Mirrors the enterprise unit test job in py-tests.yml but points at ./enterprise/tests/integration instead of ./enterprise/tests/unit. Co-Authored-By: Claude <noreply@anthropic.com>
Add pytest_collection_modifyitems hook to integration conftest so every test collected from that directory is automatically tagged @pytest.mark.integration. Enables targeted runs via `pytest -m integration` without modifying individual test files. Co-Authored-By: Claude <noreply@anthropic.com>
Coverage report
This PR does not seem to contain any modification to coverable code. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
enterprise/tests/integration/conftest.py (2)
35-37:pytest_collection_modifyitemssignature is a bit narrow.The standard hook signature is
pytest_collection_modifyitems(session, config, items). Pytest's hook dispatcher tolerates fewer args, but declaring onlyitemsskips a deprecation-prone pattern and prevents future breakage if pytest ever tightens enforcement. Purely a robustness nit.♻️ Suggested signature
-def pytest_collection_modifyitems(items): +def pytest_collection_modifyitems(config, items): for item in items: item.add_marker(pytest.mark.integration)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@enterprise/tests/integration/conftest.py` around lines 35 - 37, The hook function pytest_collection_modifyitems currently only accepts items; change its signature to the standard pytest_collection_modifyitems(session, config, items) to match Pytest's expected hook and avoid future deprecation issues, keeping the body (for item in items: item.add_marker(pytest.mark.integration)) the same; if linters complain about unused session/config, name them _session and _config or prefix with underscores.
69-71: Engine/session resources aren't disposed between tests.
engineandasync_engineare function-scoped but neverdispose()d, so each test leaks a SQLite connection pool bound to the now-deletedtmp_pathDB. Underpytest-xdist --forked -n autothis is harmless (each test gets a new process), but when running without--forkedyou'll accumulate file handles and, on Windows, potentially fail to clean uptmp_path. Consider converting toyieldfixtures that call.dispose()in teardown.♻️ Proposed fix
`@pytest.fixture` def engine(db_path): """Create a sync engine with tables using file-based DB.""" engine = create_engine( f'sqlite:///{db_path}', connect_args={'check_same_thread': False} ) Base.metadata.create_all(engine) - return engine + try: + yield engine + finally: + engine.dispose()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@enterprise/tests/integration/conftest.py` around lines 69 - 71, The engine and async_engine fixtures currently create engines but never dispose them, leaking SQLite connection pools; change those fixtures to yield-style fixtures that yield the engine(s) to tests and then call .dispose() in the teardown (for async_engine, await async_engine.dispose() if it is async) to release file handles; ensure session_maker can continue returning sessionmaker(bind=engine) but relies on the yielded engine so disposal happens after tests complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/enterprise-integration-tests.yml:
- Around line 36-41: The CI job uses the wrong matrix key name: replace every
occurrence of matrix.python_version with matrix.python-version (e.g., update
COVERAGE_FILE env and the actions/upload-artifact "name" and "path" inputs) so
the coverage filename and uploaded artifact include the actual Python version;
specifically edit the lines that set COVERAGE_FILE and the upload-artifact
inputs to use ${ { matrix.python-version } } instead of ${ {
matrix.python_version } }.
In `@enterprise/tests/integration/conftest.py`:
- Around line 74-87: The async_engine fixture currently calls
asyncio.run(create_tables()) which binds the engine to a throwaway event loop;
instead make async_engine depend on the existing sync engine fixture (engine)
and remove the asyncio.run/create_tables step so tables are created by engine's
setup, or convert async_engine into an async pytest fixture that performs
create_tables() with await on the test's event loop; locate the async_engine
function and modify it to either accept engine as a parameter and drop the
table-creation coroutine, or change its signature to async def async_engine(...)
and await create_tables() (keeping create_async_engine(...) usage intact) so the
async primitives are bound to the test event loop.
---
Nitpick comments:
In `@enterprise/tests/integration/conftest.py`:
- Around line 35-37: The hook function pytest_collection_modifyitems currently
only accepts items; change its signature to the standard
pytest_collection_modifyitems(session, config, items) to match Pytest's expected
hook and avoid future deprecation issues, keeping the body (for item in items:
item.add_marker(pytest.mark.integration)) the same; if linters complain about
unused session/config, name them _session and _config or prefix with
underscores.
- Around line 69-71: The engine and async_engine fixtures currently create
engines but never dispose them, leaking SQLite connection pools; change those
fixtures to yield-style fixtures that yield the engine(s) to tests and then call
.dispose() in the teardown (for async_engine, await async_engine.dispose() if it
is async) to release file handles; ensure session_maker can continue returning
sessionmaker(bind=engine) but relies on the yielded engine so disposal happens
after tests complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2607a880-7f3a-407c-b825-353333cf16de
📒 Files selected for processing (7)
.github/workflows/enterprise-integration-tests.ymlenterprise/pyproject.tomlenterprise/tests/integration/__init__.pyenterprise/tests/integration/conftest.pyenterprise/tests/integration/test_api_key_store.pyenterprise/tests/integration/test_api_key_store_system_keys.pyenterprise/tests/integration/test_org_store.py
… and workflow - Fix matrix.python_version → matrix.python-version in enterprise-integration-tests.yml - Use standard pytest_collection_modifyitems(config, items) hook signature - Convert engine fixture to yield + dispose() to release SQLite connections - Remove asyncio.run(create_tables()) from async_engine; depend on engine fixture so tables already exist, avoiding a throwaway event loop binding Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/review |
1 participant
Why
The store tests in
tests/unit/spun up a real SQLite database on every run, making them integration tests by nature. Running them alongside true unit tests inflates CI time and blurs the unit/integration boundary. Moving them totests/integration/makes the distinction explicit and allows them to be skipped or run separately.Summary
test_org_store.pyandtest_api_key_store.pyfromtests/unit/totests/integration/tests/unit/storage/test_api_key_store.py(system key tests) totests/integration/test_api_key_store_system_keys.pytests/integration/conftest.pywith shared DB fixtures (db_path,engine,session_maker,async_engine,async_session_maker) and apytest_collection_modifyitemshook that auto-applies@pytest.mark.integrationto all collected testsintegrationmarker inenterprise/pyproject.toml.github/workflows/enterprise-integration-tests.yml— a dedicated CI job that runstests/integration/so the moved tests remain in CI coverageIssue Number
N/A
How to Test
Integration tests can be excluded from a unit-only run with
-m "not integration".Type
Notes
engineandasync_enginefixtures are yield-based and call.dispose()in teardown to release SQLite connectionsasync_enginedepends on the syncenginefixture so tables are created once before async tests run (avoidsasyncio.run()binding to a throwaway event loop)Summary by CodeRabbit