feat: add async PostgreSQL fixture support#1295
Open
tboy1337 wants to merge 47 commits into
Open
Conversation
- Introduced AsyncDatabaseJanitor for managing database state asynchronously. - Added async loading capabilities with build_loader_async and sql_async functions. - Updated factories to include async versions of PostgreSQL fixtures. - Enhanced tests to cover async functionality for janitor and loader. - Updated pyproject.toml to include optional dependencies for async support.
- Added tests for custom database name handling in AsyncDatabaseJanitor. - Implemented tests for database initialization and dropping with various configurations. - Included tests for callable resolution in build_loader and build_loader_async with dot-separated paths. - Improved error handling tests for sql_async when aiofiles is not installed.
…c functions - Updated the loader and client to check for aiofiles and pytest_asyncio imports at runtime, raising ImportError with clear messages if they are not installed. - Refactored tests to mock the absence of these dependencies more effectively, ensuring proper error handling is validated.
… postgresql_async fixture Made-with: Cursor
… default postgresql_async fixture" This reverts commit 5c92310.
for more information, see https://pre-commit.ci
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml 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 async PostgreSQL fixture support, async loader and retry helpers, Windows asyncio plugin hooks, process fixture port-lock cleanup, and updated tests, docs, and dependency metadata. ChangesAsync PostgreSQL support
Sequence Diagram(s)sequenceDiagram
participant Test as pytest test
participant Factory as postgresql_async
participant Janitor as AsyncDatabaseJanitor
participant Loader as build_loader_async/sql_async
participant Retry as retry_async
participant Conn as psycopg.AsyncConnection
participant PG as PostgreSQL
Test->>Factory: request async fixture
Factory->>Janitor: create and initialise
Janitor->>Retry: connect with retry_async
Retry->>Conn: AsyncConnection.connect()
Conn->>PG: open session
Janitor->>PG: CREATE DATABASE
Test->>Conn: await cursor.execute(...)
Janitor->>Loader: load SQL or callable
Loader->>Conn: execute SQL / commit
Test->>Janitor: fixture teardown
Janitor->>PG: terminate backends and DROP DATABASE
Conn->>PG: close()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Contributor
Author
|
pre-commit broke coderabbit @fizyk |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pytest_postgresql/factories/process.py (1)
85-110:⚠️ Potential issue | 🟠 MajorConfigurable process scopes currently leak the port lock file.
Line 110 now allows repeated setup/teardown cycles, but the
postgresql-<port>.portsentinel created on Line 140 is never removed if the fixture later tears down or fails. Withscope="function"and a fixed port or narrow range, the second invocation can fail withPortForExceptioneven though the previous server has already stopped.Possible fix
- postgresql_executor = PostgreSQLExecutor( - executable=postgresql_ctl, - host=host or config.host, - port=pg_port, - user=user or config.user, - password=password or config.password, - dbname=pg_dbname, - options=options or config.options, - datadir=str(datadir), - unixsocketdir=unixsocketdir or config.unixsocketdir, - logfile=str(logfile_path), - startparams=startparams or config.startparams, - postgres_options=postgres_options or config.postgres_options, - ) - # start server - with postgresql_executor: - postgresql_executor.wait_for_postgres() - janitor = DatabaseJanitor( - user=postgresql_executor.user, - host=postgresql_executor.host, - port=postgresql_executor.port, - dbname=postgresql_executor.template_dbname, - as_template=True, - version=postgresql_executor.version, - password=postgresql_executor.password, - ) - if config.drop_test_database: - janitor.drop() - with janitor: - for load_element in pg_load: - janitor.load(load_element) - yield postgresql_executor + try: + postgresql_executor = PostgreSQLExecutor( + executable=postgresql_ctl, + host=host or config.host, + port=pg_port, + user=user or config.user, + password=password or config.password, + dbname=pg_dbname, + options=options or config.options, + datadir=str(datadir), + unixsocketdir=unixsocketdir or config.unixsocketdir, + logfile=str(logfile_path), + startparams=startparams or config.startparams, + postgres_options=postgres_options or config.postgres_options, + ) + # start server + with postgresql_executor: + postgresql_executor.wait_for_postgres() + janitor = DatabaseJanitor( + user=postgresql_executor.user, + host=postgresql_executor.host, + port=postgresql_executor.port, + dbname=postgresql_executor.template_dbname, + as_template=True, + version=postgresql_executor.version, + password=postgresql_executor.password, + ) + if config.drop_test_database: + janitor.drop() + with janitor: + for load_element in pg_load: + janitor.load(load_element) + yield postgresql_executor + finally: + port_filename_path.unlink(missing_ok=True)Also applies to: 133-141, 155-186
🧹 Nitpick comments (1)
tests/test_retry.py (1)
18-32: Avoid the real backoff in the failure-path tests.These two tests add roughly three seconds of wall-clock delay and can become timing-sensitive on busy runners. Patching
pytest_postgresql.retry.asyncio.sleep(and the clock helper for the timeout case) would keep the async retry coverage fast and deterministic.Also applies to: 35-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_retry.py` around lines 18 - 32, The tests test_retry_async_succeeds_after_failures (and the related timeout test) are incurring real backoff delays; patch the sleep and clock used by retry_async to make them deterministic and fast: in the tests monkeypatch pytest_postgresql.retry.asyncio.sleep to an async no-op (or a function that records calls) and also patch the retry module's clock/timer helper used for timeout handling so the timeout case can advance without waiting; update the tests to use these monkeypatched replacements while calling retry_async to eliminate wall-clock backoff and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/factories/client.py`:
- Around line 102-119: The async fixture decorator call for
postgresql_async_factory must set loop_scope to match the fixture cache scope to
avoid event-loop lifetime misconfiguration; update the `@pytest_asyncio.fixture`
invocation (the decorator on postgresql_async_factory) to pass loop_scope=scope
so the event loop scope is at least as broad as the fixture scope (keeping the
existing scope=scope argument).
In `@tests/test_postgresql.py`:
- Around line 109-127: The async test test_postgres_terminate_connection_async
is missing the pytest-xdist isolation marker; add the same
`@pytest.mark.xdist_group`(name="terminate_connection") decorator to the async
test (alongside the existing `@pytest.mark.asyncio` and `@pytest.mark.parametrize`)
so it gets run in the same xdist group as the synchronous counterpart and avoids
cross-test interference.
---
Nitpick comments:
In `@tests/test_retry.py`:
- Around line 18-32: The tests test_retry_async_succeeds_after_failures (and the
related timeout test) are incurring real backoff delays; patch the sleep and
clock used by retry_async to make them deterministic and fast: in the tests
monkeypatch pytest_postgresql.retry.asyncio.sleep to an async no-op (or a
function that records calls) and also patch the retry module's clock/timer
helper used for timeout handling so the timeout case can advance without
waiting; update the tests to use these monkeypatched replacements while calling
retry_async to eliminate wall-clock backoff and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 821e8573-8a73-4dae-a6fc-dde60894e840
📒 Files selected for processing (18)
newsfragments/1235.feature.rstpyproject.tomlpytest_postgresql/factories/__init__.pypytest_postgresql/factories/client.pypytest_postgresql/factories/noprocess.pypytest_postgresql/factories/process.pypytest_postgresql/janitor.pypytest_postgresql/loader.pypytest_postgresql/retry.pypytest_postgresql/types.pytests/conftest.pytests/docker/test_noproc_docker.pytests/test_factory_errors.pytests/test_janitor.pytests/test_loader.pytests/test_postgresql.pytests/test_retry.pytests/test_template_database.py
- Updated the postgresql_async fixture to include loop_scope for better async handling. - Added xdist_group marker to the test_postgres_terminate_connection_async for improved test organization. - Refactored retry tests to mock sleep and current time, ensuring accurate timeout handling and retry logic validation.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_retry.py (1)
49-64: Optionally harden timeout assertions.You may want to assert no sleep occurred and only one failing call happened in this scenario, to lock in current timeout behaviour more explicitly.
Optional tightening
- async def always_fail() -> None: - raise ValueError("boom") + always_fail = AsyncMock(side_effect=ValueError("boom")) @@ with pytest.raises(TimeoutError, match="Failed after"): await retry_async(always_fail, timeout=1, possible_exception=ValueError) + sleep_mock.assert_not_awaited() + assert always_fail.await_count == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_retry.py` around lines 49 - 64, Add explicit assertions that no sleep was awaited and that only the expected number of time-checks/calls happened: after the with-block raising TimeoutError, call sleep_mock.assert_not_awaited() to ensure no backoff was used and assert call_count == 2 (verifying get_current_datetime was called exactly once to capture the start and once to detect the timeout) to lock the current timeout behavior for retry_async when always_fail raises immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_retry.py`:
- Around line 49-64: Add explicit assertions that no sleep was awaited and that
only the expected number of time-checks/calls happened: after the with-block
raising TimeoutError, call sleep_mock.assert_not_awaited() to ensure no backoff
was used and assert call_count == 2 (verifying get_current_datetime was called
exactly once to capture the start and once to detect the timeout) to lock the
current timeout behavior for retry_async when always_fail raises immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bf34b69-b624-4e9d-8cb3-1fdaeffb1f94
📒 Files selected for processing (3)
pytest_postgresql/factories/client.pytests/test_postgresql.pytests/test_retry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_postgresql.py
- Changed isolation level and autocommit settings to direct attribute assignment instead of using methods. - Removed unnecessary mock methods in test for improved clarity and performance.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_janitor.py (1)
124-143: Consider adding test coverage for async loaders.This test validates that
AsyncDatabaseJanitor.load()works with synchronous loaders via theinspect.isawaitablecheck. Consider adding a companion test that exercises the async loader path (e.g., using aPathargument to triggersql_async), ensuring theawait resultbranch is also covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_janitor.py` around lines 124 - 143, Add a companion async test that exercises the AsyncDatabaseJanitor.load() path where the loader is asynchronous (so the inspect.isawaitable branch and the await result path are executed): create an async loader (or point to an async loader via a Path to trigger sql_async), patch tests.loader.psycopg.connect like in test_janitor_populate_async, instantiate AsyncDatabaseJanitor(version=10, **call_kwargs), await janitor.load(async_loader) and assert connect_mock.called and connect_mock.call_args.kwargs == call_kwargs; ensure the test uses `@pytest.mark.asyncio` and param or fixture to provide the async loader so sql_async/inspect.isawaitable logic in AsyncDatabaseJanitor.load is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/janitor.py`:
- Around line 252-259: The SQL string concatenation in the async helper
_terminate_connection (async def _terminate_connection(cur: AsyncCursor, dbname:
str)) currently joins two literals without a space, producing "...pid)FROM..."
and causing a syntax error; fix it by ensuring there's whitespace between ")”
and "FROM" (e.g., include a trailing space on the first literal or a leading
space on the second) so the executed query becomes "...pid) FROM
pg_stat_activity ...", and apply the same whitespace fix to the synchronous
DatabaseJanitor._terminate_connection query to avoid the identical runtime
error.
---
Nitpick comments:
In `@tests/test_janitor.py`:
- Around line 124-143: Add a companion async test that exercises the
AsyncDatabaseJanitor.load() path where the loader is asynchronous (so the
inspect.isawaitable branch and the await result path are executed): create an
async loader (or point to an async loader via a Path to trigger sql_async),
patch tests.loader.psycopg.connect like in test_janitor_populate_async,
instantiate AsyncDatabaseJanitor(version=10, **call_kwargs), await
janitor.load(async_loader) and assert connect_mock.called and
connect_mock.call_args.kwargs == call_kwargs; ensure the test uses
`@pytest.mark.asyncio` and param or fixture to provide the async loader so
sql_async/inspect.isawaitable logic in AsyncDatabaseJanitor.load is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 951653a7-eb09-47eb-a5cc-adc4bbea59c1
📒 Files selected for processing (2)
pytest_postgresql/janitor.pytests/test_janitor.py
- Added a space at the end of the SQL query string in both DatabaseJanitor and AsyncDatabaseJanitor to ensure proper formatting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/janitor.py`:
- Around line 224-231: The CREATE DATABASE DDL currently builds SQL with
f-strings using self.dbname and self.template_dbname (in the method that calls
cur.execute with query), which can break on embedded quotes; replace this string
interpolation with psycopg.sql.Identifier composition: import psycopg.sql as
sql, build the base sql.SQL("CREATE DATABASE
{}").format(sql.Identifier(self.dbname)), conditionally append sql.SQL("
TEMPLATE {}").format(sql.Identifier(self.template_dbname)) when present and
sql.SQL(" IS_TEMPLATE = true") when self.as_template is true, then pass the
composed sql.SQL object to await cur.execute(...) instead of the f-string.
Ensure all occurrences that construct database identifiers (the blocks using
self.dbname, self.template_dbname, and as_template) use sql.Identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dceab04-c112-4e1c-8a96-71c78d839bfd
📒 Files selected for processing (1)
pytest_postgresql/janitor.py
- Updated PostgreSQLExecutor to conditionally omit Unix socket parameters on Windows. - Enhanced error handling in PostgreSQLExecutor for Windows compatibility. - Refactored AsyncDatabaseJanitor to use async methods for setting isolation level and autocommit. - Updated tests to mock async connection methods for better compatibility.
for more information, see https://pre-commit.ci
Remove Windows executor compatibility changes (log_destination quoting, unix_socket_directories platform guard, os.killpg AttributeError catch) and the Windows-only event_loop_policy fixture from conftest.py. These belong in the separate Windows compatibility PR dbfixtures#1182. Made-with: Cursor
… for PR-1182) Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pytest_postgresql/janitor.py (1)
223-250:⚠️ Potential issue | 🟠 MajorCompose database identifiers with
psycopg.sql.Identifier.
self.dbname,self.template_dbname, anddbnameare still interpolated straight into DDL here. Any identifier containing"will generate invalid SQL, so the new async path inherits the same quoting bug as the sync janitor. Please build these statements withsql.Identifier(...)instead of f-strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_postgresql/janitor.py` around lines 223 - 250, Replace all f-string interpolation of database identifiers with psycopg's composable SQL identifiers: build queries using psycopg.sql.SQL(...) and psycopg.sql.Identifier(...) instead of f-strings for the code paths that use self.dbname and self.template_dbname (the block that constructs query and calls await cur.execute(f"{query};")), the drop method (ALTER DATABASE ... with is_template false; DROP DATABASE IF EXISTS ...), and the helper _dont_datallowconn (which runs ALTER DATABASE ... with allow_connections false;). Use sql.Identifier(dbname) / sql.Identifier(self.template_dbname) to compose the identifier parts and sql.SQL(...) to join fragments so the executed statements are safe even when identifiers contain quotes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/plugin.py`:
- Line 138: The module-level call to
factories.postgresql_async("postgresql_proc") (assigned to postgresql_async)
triggers ImportError when pytest_asyncio is not installed; change this to defer
creation so the ImportError only occurs if the async fixture is actually
requested: replace the module-level invocation of factories.postgresql_async
with a lazy registration pattern — either wrap the call in a try/except
ImportError and register a stub fixture named postgresql_async that raises a
clear ImportError when used, or implement a small wrapper function/fixture that
calls factories.postgresql_async("postgresql_proc") on first access; update
references to the postgresql_async symbol accordingly so the optional [async]
extra remains optional.
---
Duplicate comments:
In `@pytest_postgresql/janitor.py`:
- Around line 223-250: Replace all f-string interpolation of database
identifiers with psycopg's composable SQL identifiers: build queries using
psycopg.sql.SQL(...) and psycopg.sql.Identifier(...) instead of f-strings for
the code paths that use self.dbname and self.template_dbname (the block that
constructs query and calls await cur.execute(f"{query};")), the drop method
(ALTER DATABASE ... with is_template false; DROP DATABASE IF EXISTS ...), and
the helper _dont_datallowconn (which runs ALTER DATABASE ... with
allow_connections false;). Use sql.Identifier(dbname) /
sql.Identifier(self.template_dbname) to compose the identifier parts and
sql.SQL(...) to join fragments so the executed statements are safe even when
identifiers contain quotes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9ce58b1-1939-4f2d-abc2-0e5fabc5dc59
📒 Files selected for processing (5)
pytest_postgresql/executor.pypytest_postgresql/janitor.pypytest_postgresql/plugin.pytests/conftest.pytests/test_janitor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
…baseJanitor - Updated SQL query construction to use psycopg.sql for better safety and readability. - Refactored database creation, alteration, and dropping methods to utilize composable SQL objects. - Improved test assertions by rendering SQL commands for clarity in test outputs.
for more information, see https://pre-commit.ci
- Updated the postgresql_async fixture to conditionally use pytest_asyncio if available, enhancing compatibility with synchronous fixtures. - Added tests to ensure that the fixture does not raise errors at creation time when pytest_asyncio is absent, while still raising an ImportError during usage without pytest_asyncio.
for more information, see https://pre-commit.ci
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_janitor.py (1)
173-175: Make the patched cursor helper signature explicit.When patched onto
AsyncDatabaseJanitor.cursor, the first bound argument is the instance. Declaringselfexplicitly in_ctxavoids brittle behaviour if future tests passdbname.♻️ Suggested tweak
- async def _ctx(dbname: str = "postgres") -> AsyncIterator[AsyncMock]: + async def _ctx(self: AsyncDatabaseJanitor, dbname: str = "postgres") -> AsyncIterator[AsyncMock]: yield cur🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_janitor.py` around lines 173 - 175, The helper asynccontextmanager _ctx used to patch AsyncDatabaseJanitor.cursor must declare the bound instance explicitly; change the signature to include self as the first parameter (e.g. async def _ctx(self, dbname: str = "postgres") -> AsyncIterator[AsyncMock]) so the patched method receives the instance correctly when called on AsyncDatabaseJanitor, then yield cur as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pytest_postgresql/factories/client.py`:
- Around line 116-127: The async fixture postgresql_async_factory currently gets
decorated even when pytest_asyncio is None, causing an async def to be passed to
pytest and leading to "coroutine was never awaited" errors; change the
implementation so that when pytest_asyncio is None you register a synchronous
fixture function (not async) that immediately raises ImportError with the same
message, and only define the async def postgresql_async_factory when
pytest_asyncio is present; update usage of fixture_decorator/pytest.fixture to
wrap the sync stub in that branch so the guard executes at fixture call time and
surfaces the clear ImportError instead of an unawaited coroutine.
---
Nitpick comments:
In `@tests/test_janitor.py`:
- Around line 173-175: The helper asynccontextmanager _ctx used to patch
AsyncDatabaseJanitor.cursor must declare the bound instance explicitly; change
the signature to include self as the first parameter (e.g. async def _ctx(self,
dbname: str = "postgres") -> AsyncIterator[AsyncMock]) so the patched method
receives the instance correctly when called on AsyncDatabaseJanitor, then yield
cur as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6aae3db-164c-4f28-99f3-850e516c8115
📒 Files selected for processing (5)
pytest_postgresql/factories/client.pypytest_postgresql/janitor.pypytest_postgresql/plugin.pytests/test_factory_errors.pytests/test_janitor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_factory_errors.py
…n pytest-asyncio is absent - Updated the postgresql_async fixture to return a synchronous stub that raises ImportError if pytest-asyncio is not available, preventing coroutine-related warnings. - Modified tests to verify that the synchronous stub is correctly registered and raises the appropriate error when used without pytest-asyncio.
Inline AsyncConnection.connect kwargs instead of dict unpacking so strict mypy accepts the connection parameters. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
…ject.toml; enhance documentation for async extra to reflect the new version requirement.
…ying minimum pytest-asyncio version 0.24. Update error messages to reflect new version requirement and clarify async fixture behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_factory_errors.py`:
- Line 32: In the pytest.raises call with the match parameter on line 32, escape
the dot metacharacter in the version string pattern. The current pattern uses a
literal dot in "pytest-asyncio >= 0.24" which in regex will match any character.
Replace the unescaped dot before the version numbers with a backslash-escaped
dot to ensure the assertion matches only the intended literal dot character and
follows regex best practices.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87a86d75-749f-4ec6-9089-51833cef50ac
📒 Files selected for processing (3)
README.rstpytest_postgresql/factories/client.pytests/test_factory_errors.py
✅ Files skipped from review due to trivial changes (1)
- README.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- pytest_postgresql/factories/client.py
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
…o use escaped regex pattern.
…itionally based on asyncio plugin presence; refactor password handling in janitor tests for consistency.
fizyk
reviewed
Jun 21, 2026
…he scope parameter and setting defaults directly in the fixture decorators. Update README to clarify async fixture behavior and dependencies. Remove unused types module.
…s and modify test cases to handle port initialization correctly by passing None instead of -1. This change ensures better compatibility with various configurations.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
…files and documentation. Enhance README to reflect changes in async fixture requirements and clarify Windows event loop handling for compatibility with psycopg async.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.rst (1)
54-56: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueClarify Windows/Python 3.14+ event loop handling.
The statement "On Python 3.14+, this uses pytest-asyncio's loop-factory hook instead of the deprecated asyncio policy API" could be clearer. Based on the plugin implementation, the loop-factory hook is invoked whenever
pytest-asyncio >= 1.4is installed (viapytest_asyncio_loop_factories), regardless of Python version. On Python 3.14+, the policy API fallback is skipped because Python's default event loop is sufficient. Consider rewording to clarify the distinction.Suggested revision
- On Windows, the plugin configures a ``SelectorEventLoop`` automatically (required - by ``psycopg`` async). On Python 3.14+, this uses pytest-asyncio's loop-factory - hook instead of the deprecated asyncio policy API. + On Windows, the plugin configures a ``SelectorEventLoop`` automatically (required + by ``psycopg`` async) via pytest-asyncio's loop-factory hook when available + (pytest-asyncio >= 1.4). On Python 3.14+, the deprecated asyncio policy API is + avoided in favour of Python's default event loop behaviour.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.rst` around lines 54 - 56, The documentation in README.rst lines 54-56 currently conflates two separate concerns about event loop handling. Clarify that the loop-factory hook is used whenever pytest-asyncio >= 1.4 is installed (regardless of Python version), and separately explain that on Python 3.14+, the deprecated asyncio policy API fallback is skipped because Python's default event loop is sufficient. Reword the statement to make this distinction explicit rather than implying the hook is only used on Python 3.14+.tests/test_plugin_asyncio.py (1)
16-25: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExercise the Windows path explicitly in this 3.14 test.
This currently passes on non-Windows via the early return at platform check, so it does not strictly validate the intended Windows+3.14 behaviour.
🧪 Proposed test hardening
`@pytest.mark.skipif`(sys.version_info < (3, 14), reason="Deprecation applies from Python 3.14") def test_pytest_configure_skips_deprecated_policy_on_python_314() -> None: """pytest_configure must not call deprecated asyncio policy APIs on Python 3.14+.""" config = MagicMock() config.pluginmanager.has_plugin.return_value = True - with patch.object(asyncio, "set_event_loop_policy") as set_policy: + with patch.object(sys, "platform", "win32"), patch.object(asyncio, "set_event_loop_policy") as set_policy: pytest_configure(config) set_policy.assert_not_called()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_plugin_asyncio.py` around lines 16 - 25, The test_pytest_configure_skips_deprecated_policy_on_python_314 function currently passes on non-Windows systems due to an early platform check return in the pytest_configure function, which means it doesn't actually validate the Windows-specific behavior on Python 3.14+. Modify the test to explicitly mock the platform check (such as sys.platform or the equivalent platform detection method used in pytest_configure) to force the Windows code path to execute, ensuring that the test properly validates the intended Windows+Python 3.14 behavior of not calling deprecated asyncio policy APIs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/plugin.py`:
- Around line 73-80: The pytest_asyncio_loop_factories hook function is
registered unconditionally, which causes pytest plugin validation to fail if
pytest-asyncio is unavailable or outdated. Mark this hook as optional by adding
the `@pytest.hookimpl`(optionalhook=True) decorator above the function definition
to allow pytest to gracefully skip this hook when the pytest-asyncio hookspec is
not available.
---
Nitpick comments:
In `@README.rst`:
- Around line 54-56: The documentation in README.rst lines 54-56 currently
conflates two separate concerns about event loop handling. Clarify that the
loop-factory hook is used whenever pytest-asyncio >= 1.4 is installed
(regardless of Python version), and separately explain that on Python 3.14+, the
deprecated asyncio policy API fallback is skipped because Python's default event
loop is sufficient. Reword the statement to make this distinction explicit
rather than implying the hook is only used on Python 3.14+.
In `@tests/test_plugin_asyncio.py`:
- Around line 16-25: The
test_pytest_configure_skips_deprecated_policy_on_python_314 function currently
passes on non-Windows systems due to an early platform check return in the
pytest_configure function, which means it doesn't actually validate the
Windows-specific behavior on Python 3.14+. Modify the test to explicitly mock
the platform check (such as sys.platform or the equivalent platform detection
method used in pytest_configure) to force the Windows code path to execute,
ensuring that the test properly validates the intended Windows+Python 3.14
behavior of not calling deprecated asyncio policy APIs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b094f99-5570-4b6e-af65-1612033117c0
📒 Files selected for processing (9)
PipfileREADME.rstnewsfragments/1235.feature.rstnewsfragments/1295.bugfix.rstpyproject.tomlpytest_postgresql/factories/client.pypytest_postgresql/plugin.pytests/test_factory_errors.pytests/test_plugin_asyncio.py
✅ Files skipped from review due to trivial changes (2)
- newsfragments/1295.bugfix.rst
- newsfragments/1235.feature.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_factory_errors.py
- pytest_postgresql/factories/client.py
…ith pytest-asyncio and update test cases to ensure compatibility with the new loop-factory hook. Add optional hook implementation for pytest-asyncio loop factories.
…mprove compatibility with asyncio. Introduce helper functions to check platform and Python version, and update tests to reflect these changes.
…ntext to ensure it is only registered when on a Windows platform. Update tests to verify the correct behavior of the loop factory registration based on the operating system.
This update introduces a conditional import for pytest-asyncio, allowing the plugin to function without it. The version check for pytest-asyncio has been refactored to improve clarity and maintain compatibility with versions below 1.4. This change enhances the plugin's flexibility in various environments.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
The postgresql_oldest / postgres (3.10) job failed due to a hosted runner losing communication after 46m; all other matrix jobs passed. Co-authored-by: Cursor <cursoragent@cursor.com>
… parsing This change simplifies the regex used in `build_loader` and `build_loader_async` functions, allowing for deeper nested import paths without limiting the split. Additionally, new tests have been added to verify the correct behavior of these functions with deeply nested import paths.
tboy1337
changed the title
fizyk
reviewed
Jun 27, 2026
…or psycopg async Updated the README to explain the necessity of using a SelectorEventLoop on Windows due to incompatibility with the ProactorEventLoop. Added details on pytest-asyncio's loop-factory hook and its role in configuring the event loop. In plugin.py, improved comments and documentation regarding the registration of the SelectorEventLoop factory, ensuring clarity on its purpose and usage in async tests on Windows.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
postgresql_asyncfixture factory andAsyncDatabaseJanitorclass for testing async code against PostgreSQL usingpsycopg.AsyncConnection.asyncextra (pip install pytest-postgresql[async]) that pulls inpytest-asyncio(>= 1.4) andaiofilesas dependencies.DatabaseJanitorDDL to usepsycopg.sqlcomposable identifiers (same approach used by the async janitor).postgresql_proceven when setup fails.Changes
pytest_postgresql/factories/client.py: newpostgresql_asyncfactory with a synchronous stub that raisesImportErrorwhen pytest-asyncio is absentpytest_postgresql/factories/process.py: try/finally port-lock cleanup on teardownpytest_postgresql/janitor.py: newAsyncDatabaseJanitorwithinit,drop,load,cursor, and async context manager support; sync janitor refactored topsycopg.sqlidentifierspytest_postgresql/loader.py: newbuild_loader_asyncandsql_asyncfor async SQL file loading viaaiofiles; switch toimportlib.import_moduleand allow deeply nested import pathspytest_postgresql/retry.py: newretry_asynccoroutine mirroring the existingretryhelperpytest_postgresql/plugin.py: WindowsSelectorEventLoopsupport via optional pytest-asyncio loop-factory hookpyproject.toml: new[async]optional dependency groupREADME.rst: async installation, usage, and Windows event-loop notestests/: unit and integration tests covering async paths, plugin behavior, and process fixture cleanupSummary by CodeRabbit
New Features
asyncinstall extra for async test dependencies.Bug Fixes
Documentation