Conversation
…ess management - Convert _get_base_command method to BASE_PROC_START_COMMAND class attribute - Use unified command format without single quotes around PostgreSQL config values - Add _windows_terminate_process method for graceful Windows process termination - Update stop() method to use list args instead of shell=True for security - Add platform-specific termination logic with killpg fallback - Add comprehensive Windows compatibility test suite - Rename parameter in _windows_terminate_process method for clarity
…environment variable usage - Added detection of PostgreSQL path for Windows runners. - Set PostgreSQL path for Unix/macOS environments. - Updated test commands to utilize the dynamically set PostgreSQL executable path. - Introduced new Windows job configurations for PostgreSQL versions 16, 17, and 18 in tests.yml. - Removed unnecessary import in conftest.py as the plugin is registered via entry point.
…compatibility - Implement dynamic detection of pg_ctl path for Unix/macOS environments in the GitHub Actions workflow. - Add logging for exception handling during Windows process termination in PostgreSQLExecutor. - Update test mocks to reference the correct subprocess path for better compatibility. - Remove unnecessary import in conftest.py to streamline the codebase.
for more information, see https://pre-commit.ci
- Reintroduce logger initialization in executor.py for improved logging. - Remove unnecessary pass statement in exception handling within the stop method. - Update test assertions to ensure correct subprocess call format for Windows compatibility. - Temporarily modify os.killpg attribute in tests to validate fallback behavior on Windows termination.
… test assertions - Add timeout handling for process cleanup in the stop method to prevent zombie processes. - Update test to assert correct argument usage for mock_terminate in Windows compatibility tests.
- Added steps to install the package in editable mode before running tests in dockerised-postgres.yml, oldest-postgres.yml, and single-postgres.yml. - Streamlined the testing process by ensuring the package is available in the current environment for all workflows.
- Modified the package installation command to include the --no-deps flag, ensuring that only the editable package is installed without its dependencies before running tests.
- Added verification step to ensure pg_ctl is found in expected locations during the GitHub Actions workflow for single-postgres.yml. - Implemented error logging and exit strategy if PostgreSQL is not detected, improving robustness of the setup process.
- Introduced separate command templates for Unix and Windows to handle quoting and configuration differences. - Updated tests to validate the correct command template is used based on the operating system. - Enhanced test coverage for command formatting, ensuring proper handling of paths with spaces on Unix and omission of unnecessary parameters on Windows.
- Updated the pg_ctl path in single-postgres.yml to include the .exe extension for compatibility with Windows environments, ensuring proper execution of PostgreSQL commands.
- Changed the conftest file generation to use a more explicit pytest_plugins declaration for better clarity and compatibility with pytest's plugin system.
…for improved clarity - Changed the pytest_plugins declaration in the conftest file generation to specify the plugin path explicitly, enhancing compatibility with pytest's plugin system.
…tgreSQLExecutor - Added tests to verify correct command templates for Windows, Unix, and Darwin platforms, ensuring proper handling of `unix_socket_directories` and `log_destination`. - Implemented checks for locale settings on Darwin and preserved quoting for `postgres_options` across different platforms. - Improved test assertions for command generation with special characters and empty parameters, enhancing overall test robustness.
…andling - Introduced a pytest fixture to set Windows-compatible locale environment variables, ensuring compatibility with PostgreSQL's initdb on Windows. - Updated test cases for PostgreSQL start commands across Windows, Unix, and Darwin platforms to include a password parameter, enhancing test coverage and consistency.
… fixture from conftest.py - Modified locale settings in executor.py to ensure compatibility across Darwin and Windows platforms. - Removed the Windows locale setup fixture from conftest.py as the locale handling is now managed directly in executor.py, simplifying the test configuration.
…Executor - Updated the test case to create a dedicated socket directory for Unix systems, improving clarity and organization in the test setup. - Adjusted the path for the `unixsocketdir` parameter to use the newly created socket directory, ensuring proper configuration for PostgreSQLExecutor.
- Modified the upload path for pytest results to include both the temporary runner directory and a specific /tmp directory, ensuring comprehensive coverage of test artifacts during the workflow execution.
…etemp for improved temporary directory handling - Added the --basetemp option to pytest commands to specify a base temporary directory, enhancing the management of test artifacts during execution. - Simplified the upload path for pytest results to focus on the temporary runner directory, ensuring efficient artifact collection.
…t collection - Simplified the upload path for pytest results by removing the specific wildcard in the temporary directory, ensuring more efficient collection of test artifacts during the workflow execution.
Upgraded fizyk/actions-reuse pipenv-setup action from v4.2.1 to v4.4.0 which includes built-in support for editable package installation via the editable flag. This simplifies the workflows by removing the explicit pip install -e . steps. Changes: - Updated pipenv-setup to v4.4.0 in all workflow files - Added editable: true parameter to pipenv-setup steps - Removed separate "Install package in editable mode" steps from dockerised-postgres.yml, single-postgres.yml, and oldest-postgres.yml This aligns with the upstream pattern established in pytest-mongo and reduces workflow complexity while maintaining the same functionality. Co-authored-by: Cursor <cursoragent@cursor.com>
… checks - Upgraded the pipenv-run action to v4.2.1 in dockerised-postgres.yml, single-postgres.yml, and oldest-postgres.yml for consistency across workflows. - Refined the conditional check for installing libpq to use matrix.python-version instead of inputs.python-versions in oldest-postgres.yml and single-postgres.yml, improving clarity and accuracy. These changes enhance the maintainability and consistency of the workflow configurations.
…est_postgres_options_plugin.py - Updated the socket directory creation in test_executor.py to use basetemp for improved path management, addressing Unix domain socket length limitations. - Removed unnecessary conftest configuration in test_postgres_options_plugin.py to streamline the pytest setup. These changes enhance the clarity and efficiency of the test configurations.
… assertions - Removed unnecessary spaces in the command templates for both Unix and Windows to ensure proper execution of PostgreSQL commands. - Updated test assertions in test_executor.py and test_windows_compatibility.py to check for the correct formatting of the `unix_socket_directories` parameter, ensuring consistency across platforms. These changes enhance the reliability of command execution and improve test accuracy.
…utor.py - Adjusted assertions in test_actual_postgresql_start_unix and test_actual_postgresql_start_darwin to check for correct formatting of the `unix_socket_directories` parameter, ensuring consistency in command output. These changes improve the accuracy of the tests related to PostgreSQL command execution.
- Updated the platform_name parameter in test_executor.py to include "FreeBSD", expanding the test coverage for platform-specific functionality. This change enhances the robustness of the tests by ensuring compatibility with FreeBSD.
- Updated the --basetemp option in pytest commands to specify a consistent temporary directory for test artifacts, improving organization and clarity in the workflow. - Adjusted the artifact upload path to reflect the new temporary directory structure, ensuring efficient collection of test results. These changes enhance the maintainability and reliability of the testing workflow.
for more information, see https://pre-commit.ci
…tibility.py - Changed the docstring to use a raw string literal for better handling of backslashes in UNC path examples. - Clarified the description of UNC paths to ensure accurate representation of their format. These updates improve the clarity and correctness of the test documentation.
The pipenv-setup@v4.4.0 upgrade and editable: true flag additions have been extracted into a dedicated PR (dbfixtures#1294). Remove those changes from this branch to avoid duplication and conflicts once that PR is merged. Made-with: Cursor
- Updated the command templates in `PostgreSQLExecutor` to ensure the executable path is always double-quoted, improving compatibility with paths containing spaces. - Modified tests in `test_windows_compatibility.py` to reflect the changes in command formatting, ensuring that single-quoted PostgreSQL options are preserved correctly within double-quoted command strings. - Added new tests to verify that the `running()` method properly quotes the executable and datadir, enhancing robustness against shell parsing issues.
for more information, see https://pre-commit.ci
- Updated the command generation in `PostgreSQLExecutor` to escape single quotes in the `unixsocketdir` parameter, ensuring valid syntax for PostgreSQL GUC strings. - Refactored the `running()` method to use `subprocess.run` instead of `subprocess.getstatusoutput`, improving compatibility and security by avoiding shell parsing. - Added regression tests in `test_windows_compatibility.py` to verify correct escaping of apostrophes in `unixsocketdir` and ensure robust command generation.
for more information, see https://pre-commit.ci
- tests/test_windows_compatibility.py: mark the docstring with a backslash path example as a raw string so ruff D301 passes on pre-commit.ci. - .github/workflows/tests.yml: drop windows_postgres_18 and windows_postgres_16 jobs. ankane/setup-postgres@v1 only ships PostgreSQL 17 on the Windows runner, so those versions cannot install and the jobs are guaranteed to fail. Keep windows_postgres_17 and add the codecov_token secret so coverage uploads work. - .github/workflows/dockerised-postgres.yml: remove the stale editable: true flag. Per fizyk in dbfixtures#1294, the editable self-install is handled by the pytest-postgresql entry in the Pipfile, and the other workflows on main do not set the flag either. Made-with: Cursor
…ests - oldest-postgres.yml: drop redundant runner.os condition from install libpq step - Extract Windows CI into single-postgres-windows.yml for cleaner maintenance - single-postgres.yml now serves Linux/macOS only with no platform conditionals - tests.yml: wire windows_postgres_17 to single-postgres-windows.yml - test_windows_compatibility.py: add module-level pytestmark to skip on non-Windows Made-with: Cursor
- test_executor_init_bad_tmp_path: add parents=True to mkdir so nested path creation works on Windows (backslash in path literal is a separator, creating two levels that need parents=True) - executor.py stop(): set self._process = None after Windows terminate so mirakuru state is clean and stopped() context manager can call start() again without timing out - test_postgres_loader_in_ini: convert backslashes to forward slashes before writing Windows path into pytest.ini to prevent configparser from mangling the value as escape sequences Made-with: Cursor
- Override check_subprocess() in PostgreSQLExecutor to use pg_ctl status instead of checking whether the launcher subprocess (pg_ctl start -w) is still alive. pg_ctl start -w exits as soon as the server is ready, so mirakuru's default polling loop always timed out when stopped() tried to restart the server on Windows. Using pg_ctl status makes the start/stopped() cycle reliable on all platforms. - Wrap the psycopg.connect() call in assert_executor_start_stop() with retry() so that transient 'the database system is starting up' errors are tolerated. Under parallel xdist runs multiple PostgreSQL instances start simultaneously; the TCP port opens before recovery completes, causing an immediate connect to fail even though the server is running. Made-with: Cursor
- Updated assertions in `test_executor_with_special_chars_in_all_paths` to ensure that both `datadir` and `logfile_path` are properly quoted in the command string. This change enhances the reliability of the tests by explicitly checking for the presence of quoted paths, improving compatibility with special characters. Made-with: Cursor
- assert_executor_start_stop: capture conn returned by retry() and call conn.close() so the readiness backend session is released promptly instead of waiting for GC. - test_executor_with_special_chars_in_all_paths: strengthen the Unix unix_socket_directories assertion to include the actual socket_dir path value, so a missing or mis-quoted path fails the test. - PostgreSQLExecutor.start(): on Windows, run pg_ctl start -w synchronously via shell=True rather than delegating to mirakuru's subprocess polling. pg_ctl exits as soon as the server is ready, so mirakuru's check_subprocess loop always sees a dead launcher process and times out; bypassing it removes the 60-second timeout failure seen in test_noproc_cached_version and related Windows CI jobs. Made-with: Cursor
- start() Windows branch: replace shell=True/self.command with an argv list built from the same pieces used in WINDOWS_PROC_START_COMMAND (executable, datadir, port, logfile, startparams, postgres_options). Pass env=self.envvars so locale variables reach pg_ctl. Mirrors the list-form invocation already used in stop(). - check_subprocess(): only return self.running() on Windows (where start() is synchronous and mirakuru's loop is bypassed). On all other platforms delegate to super().check_subprocess() so TCPExecutor's port-reachability check and subprocess-alive check remain in effect, enabling fast ProcessFinishedWithError detection when pg_ctl fails to start. - Add missing shlex import. Made-with: Cursor
for more information, see https://pre-commit.ci
subprocess.run() on the Windows synchronous start path had no timeout, so a stuck pg_ctl could block indefinitely. Pass self._timeout (the configured mirakuru timeout in seconds) to subprocess.run(..., timeout=). Catch subprocess.TimeoutExpired — subprocess.run already kills the process before re-raising, so no extra cleanup is needed — and translate it to mirakuru.exceptions.TimeoutExpired so callers see the same exception type produced by the non-Windows path. Also imports TimeoutExpired from mirakuru.exceptions alongside the existing ProcessFinishedWithError import. Made-with: Cursor
📝 WalkthroughWalkthroughWindows support was added: executor got platform-specific startup/shutdown and command templating, CI workflows now detect/use pg_ctl on Windows and run Windows tests via a reusable workflow, and tests were expanded to cover Windows-specific behaviours and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Workflow
participant Runner as Actions Runner (windows-latest)
participant Installer as PostgreSQL installer / pg_ctl locator
participant Test as Pytest (xdist)
participant Codecov as Codecov upload
Workflow->>Runner: start windows_postgres job (matrix python versions)
Runner->>Installer: install PostgreSQL version & detect pg_ctl.exe
Installer-->>Runner: POSTGRESQL_EXEC path
Runner->>Test: run pytest (no:xdist) with POSTGRESQL_EXEC
Test-->>Runner: test results (basetemp, coverage.xml)
Runner->>Test: run pytest -n auto (xdist) with POSTGRESQL_EXEC
Test-->>Runner: xdist results, coverage-xdist.xml
alt tests fail
Runner->>Workflow: upload pytest basetemp artifact
end
Runner->>Codecov: upload coverage (unittests), continue-on-error
Codecov-->>Runner: upload status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cc7fe02. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
newsfragments/1182.feature.rst (1)
1-1: ⚡ Quick winMake the fragment more user-facing.
This reads more like an implementation note than release notes: naming
UNIX_PROC_START_COMMAND,WINDOWS_PROC_START_COMMAND, and_windows_terminate_processis probably too internal forCHANGES.rst. A shorter summary such as “Add Windows support forPostgreSQLExecutor, including platform-specific start/stop handling.” would fit better.As per coding guidelines, "Content should be suitable for inclusion in CHANGES.rst."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newsfragments/1182.feature.rst` at line 1, The fragment currently reads like an implementation note and exposes internal symbols (UNIX_PROC_START_COMMAND, WINDOWS_PROC_START_COMMAND, and _windows_terminate_process); change the single-line fragment to a user-facing summary such as: "Add Windows support for PostgreSQLExecutor, including platform-specific start/stop handling." Keep mention of PostgreSQLExecutor but remove the internal constant and private method names so the text is suitable for CHANGES.rst inclusion.
🤖 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/oldest-postgres.yml:
- Around line 55-56: The current step unconditionally runs "sudo apt install
libpq5" whenever the matrix entry contains PyPy (if: ${{
contains(matrix.python-version, 'pypy') }}), which will fail on non-Linux
runners; change the conditional to require a Linux runner as well (e.g. if: ${{
contains(matrix.python-version, 'pypy') && runner.os == 'Linux' }}) so the "run:
sudo apt install libpq5" only executes on Linux hosts.
In `@pytest_postgresql/executor.py`:
- Around line 202-203: The child process environment is currently replaced by
self.envvars when calling subprocess.run(args, ..., env=self.envvars), which
drops vital system variables on Windows; instead create a merged environment
from the parent (os.environ.copy()) and update it with self.envvars, then pass
that merged dict as env to subprocess.run (i.e., merged_env = os.environ.copy();
merged_env.update(self.envvars); subprocess.run(..., env=merged_env, ...)).
Ensure you import os if not already and apply this change where subprocess.run
is invoked in the executor code.
- Around line 189-200: The issue is shlex.split(self.startparams) using POSIX
mode that treats backslashes as escapes and breaks Windows paths; update the
call in executor.py where args is built (the args list including
self.executable, "start", "-D", self.datadir, "-o", pg_options, "-l",
self.logfile, *shlex.split(self.startparams)) to call
shlex.split(self.startparams, posix=False) so Windows backslashes are preserved
when tokenising startparams before passing to subprocess.
---
Nitpick comments:
In `@newsfragments/1182.feature.rst`:
- Line 1: The fragment currently reads like an implementation note and exposes
internal symbols (UNIX_PROC_START_COMMAND, WINDOWS_PROC_START_COMMAND, and
_windows_terminate_process); change the single-line fragment to a user-facing
summary such as: "Add Windows support for PostgreSQLExecutor, including
platform-specific start/stop handling." Keep mention of PostgreSQLExecutor but
remove the internal constant and private method names so the text is suitable
for CHANGES.rst inclusion.
🪄 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: 38ee55d4-f887-4bd8-9cca-9934eaffc275
📒 Files selected for processing (9)
.github/workflows/oldest-postgres.yml.github/workflows/single-postgres-windows.yml.github/workflows/single-postgres.yml.github/workflows/tests.ymlnewsfragments/1182.feature.rstpytest_postgresql/executor.pytests/test_executor.pytests/test_postgres_options_plugin.pytests/test_windows_compatibility.py
…nv/posix Bug fixes - executor.py running(): add capture_output=True to subprocess.run so pg_ctl status output no longer leaks to the terminal on every call; previously the change from getstatusoutput() dropped the implicit capture. - test_windows_compatibility.py: remove the module-level pytestmark that skipped every test on non-Windows. All tests in the file use patch() to mock platform.system() and subprocess calls, so none of them require a real Windows host; the blanket skip was silently hiding the entire template and process-management test suite from Linux/macOS CI runs. The unused sys import is removed along with the mark. Inline comment fixes - oldest-postgres.yml: narrow the 'install libpq' step condition to also require runner.os == 'Linux' so 'sudo apt install' is not attempted on non-Linux runners when the matrix entry contains PyPy. - executor.py Windows start() branch: build merged_env from os.environ.copy() updated with self.envvars so system-level PATH and other vital variables are inherited rather than replaced by the locale-only dict. - executor.py Windows start() branch: pass posix=False to shlex.split(self.startparams) so Windows backslashes in startparams are treated as literals rather than POSIX escape characters. Nitpick - newsfragments/1182.feature.rst: rewrite as a concise user-facing summary without internal symbol names. Made-with: Cursor
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pytest_postgresql/executor.py (1)
235-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMerge the parent environment in
init_directory()too.Line 246 and Line 251 still pass
env=self.envvarsdirectly tosubprocess.check_output(). That replaces the child environment instead of extending it, so the Windows path can still fail duringinitdbbeforestart()ever runs becausePATH,SystemRoot,TEMP, and friends are dropped. Reuse the merged-env approach you already added instart()and feed that into both branches.Proposed fix
def init_directory(self) -> None: """Initialize postgresql data directory. @@ init_directory = [self.executable, "initdb", "--pgdata", self.datadir] options = ["--username=%s" % self.user] + merged_env = os.environ.copy() + merged_env.update(self.envvars) if self.password: with tempfile.NamedTemporaryFile() as password_file: @@ init_directory += ["-o", " ".join(options)] # Passing envvars to command to avoid weird MacOs error. - subprocess.check_output(init_directory, env=self.envvars) + subprocess.check_output(init_directory, env=merged_env) else: options += ["--auth=trust"] init_directory += ["-o", " ".join(options)] # Passing envvars to command to avoid weird MacOs error. - subprocess.check_output(init_directory, env=self.envvars) + subprocess.check_output(init_directory, env=merged_env)#!/bin/bash # Demonstrate that subprocess env replaces, rather than merges, the parent environment. python - <<'PY' import subprocess import sys cmd = [ sys.executable, "-c", "import os; print('PATH' in os.environ, os.environ.get('LC_ALL'))", ] print(subprocess.check_output(cmd, env={'LC_ALL': 'C'}, text=True).strip()) PYExpected result:
False C. The same behaviour on Windows also strips variables such asSystemRootandTEMP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_postgresql/executor.py` around lines 235 - 251, The subprocess.check_output calls in the init_directory flow replace the child environment instead of extending it; modify the block around the two subprocess.check_output(init_directory, env=self.envvars) calls to build a merged environment (e.g. merged_env = os.environ.copy(); merged_env.update(self.envvars)) and pass merged_env to subprocess.check_output in both the password branch and the trust branch, keeping the existing password_file write/flush and the init_directory/options logic intact (refer to the init_directory variable and the subprocess.check_output calls to locate where to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pytest_postgresql/executor.py`:
- Around line 235-251: The subprocess.check_output calls in the init_directory
flow replace the child environment instead of extending it; modify the block
around the two subprocess.check_output(init_directory, env=self.envvars) calls
to build a merged environment (e.g. merged_env = os.environ.copy();
merged_env.update(self.envvars)) and pass merged_env to subprocess.check_output
in both the password branch and the trust branch, keeping the existing
password_file write/flush and the init_directory/options logic intact (refer to
the init_directory variable and the subprocess.check_output calls to locate
where to change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f76f5fb9-f38f-4560-a1bc-72d958f7f463
📒 Files selected for processing (4)
.github/workflows/oldest-postgres.ymlnewsfragments/1182.feature.rstpytest_postgresql/executor.pytests/test_windows_compatibility.py
✅ Files skipped from review due to trivial changes (1)
- newsfragments/1182.feature.rst

Chore that needs to be done:
pipenv run towncrier create [issue_number].[type].rstTypes are defined in the pyproject.toml, issue_number either from issue tracker or the Pull request number
Note
Medium Risk
Touches process lifecycle/quoting logic in
PostgreSQLExecutorand changes how PostgreSQL is started/stopped across platforms; regressions could break test startup or leave orphaned processes, especially on Windows.Overview
Adds Windows support for
PostgreSQLExecutorby introducing platform-specific pg_ctl start command templates, adjusting locale handling, runningpg_ctl startsynchronously on Windows, and adding Windows-specific termination/stop logic (plus safer non-shell subprocess calls for status/stop).Expands test coverage for quoting/paths and Windows behaviors (including new
test_windows_compatibility.py), updates existing tests to be more resilient under xdist, and tweaks INI path handling for Windows. CI is updated to dynamically locatepg_ctl, standardize--basetemp/artifact paths, fix the PyPy libpq condition, and add a dedicated Windows PostgreSQL workflow wired intotests.yml.Reviewed by Cursor Bugbot for commit cc7fe02. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation