fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine
CI / lint (push) Successful in 1m46s
CI / benchmark-regression (push) Has been skipped
CI / push-validation (push) Successful in 45s
CI / helm (push) Successful in 52s
CI / build (push) Successful in 1m13s
CI / typecheck (push) Successful in 1m37s
CI / quality (push) Successful in 2m30s
CI / security (push) Successful in 2m39s
CI / e2e_tests (push) Successful in 5m31s
CI / integration_tests (push) Failing after 5m47s
CI / unit_tests (push) Failing after 5m56s
CI / coverage (push) Has been skipped
CI / docker (push) Has been skipped
CI / status-check (push) Failing after 9s
CI / benchmark-publish (push) Successful in 1h17m33s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 1m20s
CI / helm (pull_request) Successful in 43s
CI / unit_tests (pull_request) Failing after 8m41s
CI / push-validation (pull_request) Successful in 37s
CI / typecheck (pull_request) Successful in 1m33s
CI / e2e_tests (pull_request) Successful in 4m12s
CI / lint (pull_request) Successful in 1m19s
CI / security (pull_request) Successful in 1m1s
CI / build (pull_request) Successful in 57s
CI / integration_tests (pull_request) Failing after 6m39s
CI / quality (pull_request) Successful in 1m6s
CI / coverage (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
CI / lint (push) Successful in 1m46s
CI / benchmark-regression (push) Has been skipped
CI / push-validation (push) Successful in 45s
CI / helm (push) Successful in 52s
CI / build (push) Successful in 1m13s
CI / typecheck (push) Successful in 1m37s
CI / quality (push) Successful in 2m30s
CI / security (push) Successful in 2m39s
CI / e2e_tests (push) Successful in 5m31s
CI / integration_tests (push) Failing after 5m47s
CI / unit_tests (push) Failing after 5m56s
CI / coverage (push) Has been skipped
CI / docker (push) Has been skipped
CI / status-check (push) Failing after 9s
CI / benchmark-publish (push) Successful in 1h17m33s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 1m20s
CI / helm (pull_request) Successful in 43s
CI / unit_tests (pull_request) Failing after 8m41s
CI / push-validation (pull_request) Successful in 37s
CI / typecheck (pull_request) Successful in 1m33s
CI / e2e_tests (pull_request) Successful in 4m12s
CI / lint (pull_request) Successful in 1m19s
CI / security (pull_request) Successful in 1m1s
CI / build (pull_request) Successful in 57s
CI / integration_tests (pull_request) Failing after 6m39s
CI / quality (pull_request) Successful in 1m6s
CI / coverage (pull_request) Has been cancelled
CI / docker (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
- Fix extra blank line in step file causing lint failure (ruff E302) - Consolidate duplicate step_when_capture_engine_args and step_when_verify_thread_safe_args into a shared _run_get_current_revision_and_capture_kwargs() helper - Remove duplicate Scenario 3 (identical assertion to Scenario 1, different step impl) - Unify context attribute name to engine_creation_kwargs across all steps - Add explicit type annotations to all step function signatures
This commit was merged in pull request #10897.
This commit is contained in:
@@ -8,79 +8,15 @@ from unittest.mock import MagicMock, patch
|
||||
from behave import then, when
|
||||
|
||||
|
||||
def _run_get_current_revision_and_capture_kwargs(
|
||||
context: Any,
|
||||
) -> None:
|
||||
"""Shared helper: call get_current_revision() and capture create_engine kwargs.
|
||||
|
||||
@when("I request the current revision and capture the engine creation args")
|
||||
def step_when_capture_engine_args(context) -> None:
|
||||
"""Call get_current_revision and capture the create_engine call arguments."""
|
||||
fake_engine = MagicMock()
|
||||
fake_connection = MagicMock()
|
||||
fake_connection.__enter__ = MagicMock(return_value=fake_connection)
|
||||
fake_connection.__exit__ = MagicMock(return_value=False)
|
||||
fake_engine.connect.return_value = fake_connection
|
||||
|
||||
migration_ctx = MagicMock()
|
||||
migration_ctx.get_current_revision.return_value = None
|
||||
|
||||
captured_calls: list[tuple[Any, ...]] = []
|
||||
|
||||
def fake_create_engine(url: str, **kwargs: Any) -> MagicMock:
|
||||
captured_calls.append((url, kwargs))
|
||||
return fake_engine
|
||||
|
||||
with (
|
||||
patch(
|
||||
"cleveragents.infrastructure.database.migration_runner.create_engine",
|
||||
side_effect=fake_create_engine,
|
||||
),
|
||||
patch(
|
||||
"cleveragents.infrastructure.database.migration_runner.MigrationContext.configure",
|
||||
return_value=migration_ctx,
|
||||
),
|
||||
):
|
||||
context.revision_result = context.runner.get_current_revision()
|
||||
|
||||
context.engine_creation_calls = captured_calls
|
||||
|
||||
|
||||
@then("the SQLite engine should be created with check_same_thread set to False")
|
||||
def step_then_sqlite_engine_has_check_same_thread(context) -> None:
|
||||
"""Verify the SQLite engine was created with check_same_thread=False."""
|
||||
assert len(context.engine_creation_calls) == 1, (
|
||||
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_calls)}"
|
||||
)
|
||||
_url, kwargs = context.engine_creation_calls[0]
|
||||
assert "connect_args" in kwargs, (
|
||||
"Expected connect_args in create_engine kwargs for SQLite, "
|
||||
f"but got kwargs: {kwargs}"
|
||||
)
|
||||
assert kwargs["connect_args"].get("check_same_thread") is False, (
|
||||
"Expected check_same_thread=False in connect_args, "
|
||||
f"but got: {kwargs['connect_args']}"
|
||||
)
|
||||
|
||||
|
||||
@then("the non-SQLite engine should be created without check_same_thread")
|
||||
def step_then_non_sqlite_engine_no_check_same_thread(context) -> None:
|
||||
"""Verify non-SQLite engines are not given check_same_thread."""
|
||||
assert len(context.engine_creation_calls) == 1, (
|
||||
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_calls)}"
|
||||
)
|
||||
_url, kwargs = context.engine_creation_calls[0]
|
||||
connect_args = kwargs.get("connect_args", {})
|
||||
assert "check_same_thread" not in connect_args, (
|
||||
"Expected check_same_thread to be absent for non-SQLite engine, "
|
||||
f"but got connect_args: {connect_args}"
|
||||
)
|
||||
|
||||
|
||||
@when("I verify get_current_revision uses thread-safe engine args for SQLite")
|
||||
def step_when_verify_thread_safe_args(context) -> None:
|
||||
"""Verify get_current_revision passes check_same_thread=False for SQLite.
|
||||
|
||||
This test verifies the thread-safety fix by inspecting the engine
|
||||
creation arguments. The check_same_thread=False argument is what
|
||||
allows SQLite connections to be used across threads, so verifying
|
||||
it is present is equivalent to verifying thread-safety.
|
||||
Mocks ``create_engine`` and ``MigrationContext.configure`` so the call
|
||||
completes without a real database. The keyword arguments passed to
|
||||
``create_engine`` are stored on ``context.engine_creation_kwargs`` for
|
||||
subsequent assertion steps.
|
||||
"""
|
||||
fake_engine = MagicMock()
|
||||
fake_connection = MagicMock()
|
||||
@@ -107,25 +43,43 @@ def step_when_verify_thread_safe_args(context) -> None:
|
||||
return_value=migration_ctx,
|
||||
),
|
||||
):
|
||||
context.runner.get_current_revision()
|
||||
context.revision_result = context.runner.get_current_revision()
|
||||
|
||||
context.captured_engine_kwargs = captured_kwargs
|
||||
context.engine_creation_kwargs = captured_kwargs
|
||||
|
||||
|
||||
@then("the SQLite engine creation args should include check_same_thread False")
|
||||
def step_then_thread_safe_args_present(context) -> None:
|
||||
"""Assert that check_same_thread=False was passed to create_engine."""
|
||||
assert len(context.captured_engine_kwargs) == 1, (
|
||||
f"Expected exactly 1 create_engine call, "
|
||||
f"got {len(context.captured_engine_kwargs)}"
|
||||
@when("I request the current revision and capture the engine creation args")
|
||||
def step_when_capture_engine_args(context: Any) -> None:
|
||||
"""Call get_current_revision and capture the create_engine call arguments."""
|
||||
_run_get_current_revision_and_capture_kwargs(context)
|
||||
|
||||
|
||||
@then("the SQLite engine should be created with check_same_thread set to False")
|
||||
def step_then_sqlite_engine_has_check_same_thread(context: Any) -> None:
|
||||
"""Verify the SQLite engine was created with check_same_thread=False."""
|
||||
assert len(context.engine_creation_kwargs) == 1, (
|
||||
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_kwargs)}"
|
||||
)
|
||||
kwargs = context.captured_engine_kwargs[0]
|
||||
kwargs = context.engine_creation_kwargs[0]
|
||||
assert "connect_args" in kwargs, (
|
||||
"Expected connect_args in create_engine kwargs for SQLite, "
|
||||
f"but got kwargs: {kwargs}"
|
||||
)
|
||||
assert kwargs["connect_args"].get("check_same_thread") is False, (
|
||||
"Expected check_same_thread=False in connect_args — this is the "
|
||||
"fix for the SQLite threading bug (issue #10507). "
|
||||
f"Got: {kwargs['connect_args']}"
|
||||
"Expected check_same_thread=False in connect_args, "
|
||||
f"but got: {kwargs['connect_args']}"
|
||||
)
|
||||
|
||||
|
||||
@then("the non-SQLite engine should be created without check_same_thread")
|
||||
def step_then_non_sqlite_engine_no_check_same_thread(context: Any) -> None:
|
||||
"""Verify non-SQLite engines are not given check_same_thread."""
|
||||
assert len(context.engine_creation_kwargs) == 1, (
|
||||
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_kwargs)}"
|
||||
)
|
||||
kwargs = context.engine_creation_kwargs[0]
|
||||
connect_args = kwargs.get("connect_args", {})
|
||||
assert "check_same_thread" not in connect_args, (
|
||||
"Expected check_same_thread to be absent for non-SQLite engine, "
|
||||
f"but got connect_args: {connect_args}"
|
||||
)
|
||||
|
||||
@@ -29,8 +29,3 @@ Feature: TDD Issue #10507 — get_current_revision() must pass check_same_thread
|
||||
Given a migration runner configured for "postgresql://user:pass@localhost/testdb"
|
||||
When I request the current revision and capture the engine creation args
|
||||
Then the non-SQLite engine should be created without check_same_thread
|
||||
|
||||
Scenario: get_current_revision engine args are thread-safe for SQLite
|
||||
Given a migration runner configured for "sqlite:///:memory:"
|
||||
When I verify get_current_revision uses thread-safe engine args for SQLite
|
||||
Then the SQLite engine creation args should include check_same_thread False
|
||||
|
||||
Reference in New Issue
Block a user