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.
- 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
…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
for more information, see https://pre-commit.ci
- Introduced a new static method _windows_pg_options to centralize the construction of the -o argument for the Windows pg_ctl start invocation, ensuring consistent command arguments across initialization and start methods. - Updated the command template in the Windows start branch to utilize the new helper method, improving clarity and maintainability. - Enhanced tests to verify that the command template and generated options do not include single quotes, aligning with Windows command requirements.
for more information, see https://pre-commit.ci
…e in PostgreSQLExecutor - Changed shlex.split(self.startparams) to use posix=True for proper handling of backslashes in start parameters on Windows. - Set self._process to None after terminating the process in the Windows branch to ensure clean state management.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Windows support for PostgreSQL test execution: executor gains platform-specific pg_ctl command templates and lifecycle handling; GitHub Actions workflows discover and export pg_ctl on runners and add a Windows reusable workflow; extensive tests validate command generation, quoting, and process lifecycle across platforms. ChangesPostgreSQL Windows Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as PostgreSQLExecutor
participant PGCtl as pg_ctl
participant Subprocess as Subprocess
participant TCP as TCPExecutor
rect rgba(100, 150, 200, 0.5)
note over Executor: Unix/Linux Flow
Client->>Executor: start()
Executor->>Executor: Build shell command (Unix template)
Executor->>PGCtl: shell string with single-quoted GUCs
PGCtl->>Subprocess: execute via shell
Subprocess->>Subprocess: Start PostgreSQL
Executor->>Executor: check_subprocess() -> delegate to TCPExecutor
TCP->>TCP: TCP socket check
TCP-->>Executor: readiness confirmed
Executor-->>Client: startup complete
end
rect rgba(200, 150, 100, 0.5)
note over Executor: Windows Flow
Client->>Executor: start()
Executor->>Executor: Build argv list (Windows template)
Executor->>Executor: _windows_pg_options() escapes quotes
Executor->>Subprocess: argv list, no shell
Subprocess->>Subprocess: Start pg_ctl.exe
Executor->>PGCtl: pg_ctl status (argv)
PGCtl-->>Executor: return code check
Executor->>Executor: check_subprocess() uses running()
Executor-->>Client: startup complete
end
rect rgba(150, 200, 100, 0.5)
note over Executor: Stop (platform aware)
Client->>Executor: stop()
alt Windows
Executor->>PGCtl: pg_ctl stop (argv)
Executor->>Executor: _windows_terminate_process()
Executor->>Subprocess: terminate then wait
alt timeout
Executor->>Subprocess: kill and wait
end
else Unix
Executor->>TCP: TCPExecutor.stop()
TCP->>TCP: os.killpg
end
Executor-->>Client: shutdown complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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 45 minutes and 13 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pytest_postgresql/executor.py (1)
356-372: 💤 Low valueReliance on mirakuru internal attribute
_process.Setting
self._process = Noneafter stopping on Windows is necessary for proper lifecycle management (allowingstart()to be called again), but it accesses mirakuru's internal attribute. This works with mirakuru 2.6.0 but could break if the library's internals change.Consider adding a brief comment noting this dependency on mirakuru internals, or document the mirakuru version constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pytest_postgresql/executor.py` around lines 356 - 372, The code relies on mirakuru's internal attribute self._process (used in stop and cleared so start() can be called again) which is fragile; add a short inline comment next to the self._process = None assignments (both inside the Windows branch after _windows_terminate_process(sig) and in the AttributeError fallback) stating that this touches mirakuru internals and that the behavior is tied to mirakuru >=2.6.0 (or document the required mirakuru version in module docs), so future maintainers know this dependency and can update if mirakuru internals change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pytest_postgresql/executor.py`:
- Around line 356-372: The code relies on mirakuru's internal attribute
self._process (used in stop and cleared so start() can be called again) which is
fragile; add a short inline comment next to the self._process = None assignments
(both inside the Windows branch after _windows_terminate_process(sig) and in the
AttributeError fallback) stating that this touches mirakuru internals and that
the behavior is tied to mirakuru >=2.6.0 (or document the required mirakuru
version in module docs), so future maintainers know this dependency and can
update if mirakuru internals change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88f7c902-813f-4723-b503-04d9d1d7f84f
📒 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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 49419dc. Configure here.
…indows compatibility - Changed the artifact name in the GitHub Actions workflow for PostgreSQL on Windows to include 'windows' for clarity. - Updated shlex.split in PostgreSQLExecutor to use posix=False, ensuring proper handling of backslashes in start parameters on Windows.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/executor.py`:
- Around line 352-354: The pg_ctl stop invocation using
subprocess.check_output([self.executable, "stop", "-D", self.datadir, "-m",
"f"]) can hang indefinitely; update the stop() path in
pytest_postgresql/executor.py to call subprocess.check_output with a timeout
(same timeout value used/normalized in start()), and catch
subprocess.TimeoutExpired to normalize/handle launcher hangs the same way
start() does (re-raise or convert to the same error path/logging). Ensure you
reference the existing start() behavior and use the same timeout semantics and
exception handling so a wedged PostgreSQL instance won't stall teardown.
- Around line 190-221: The Windows start() branch can return before Postgres is
accepting connections if self.startparams lacks -w/--wait; modify the Windows
path in start() (the code that builds args using self.executable,
_windows_pg_options and self.startparams) to either ensure a wait flag is
present (inject "-w" or "--wait" into args when neither -w nor --wait appears in
self.startparams) or, after subprocess.run returns successfully, call the
existing wait_for_postgres() (or perform the same TCP readiness poll that
TCPExecutor.start() uses) before returning so readiness semantics match the Unix
path.
🪄 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: 6ba72afd-2413-4855-9f98-8d62f30c88e5
📒 Files selected for processing (2)
.github/workflows/single-postgres-windows.ymlpytest_postgresql/executor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/single-postgres-windows.yml
- Added wait_for_postgres() call after pg_ctl start to ensure the server is ready for connections. - Implemented timeout handling in the stop() method to raise TimeoutExpired if the stop command exceeds the configured timeout.
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
1 participant
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.
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
Changes PostgreSQL process startup/shutdown logic and command construction (including platform-specific paths/quoting), which can affect test stability across OSes. CI workflow updates add coverage but may expose platform-specific edge cases in
pg_ctldiscovery and process cleanup.Overview
Adds Windows support to
PostgreSQLExecutorvia platform-specificpg_ctlstart command generation, locale selection, readiness checks, and Windows-safe stop/termination handling (avoidingshell=Trueand relying on argv-basedsubprocesscalls).Updates tests to cover new quoting/escaping behavior (including special-character paths), adds a dedicated Windows compatibility test suite, and makes executor start/stop assertions more robust by retrying initial connections.
Enhances GitHub Actions by adding a Windows PostgreSQL job, making
pg_ctlpath detection cross-platform in the single-postgres workflow, and adjusting temp paths/artifact collection for better Windows/macOS support.Reviewed by Cursor Bugbot for commit 49419dc. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests