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.
for more information, see https://pre-commit.ci
…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.
- 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.
…via entry point, streamlining the codebase.
- 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.
Resolved conflict in .github/workflows/dockerised-postgres.yml by keeping the three-step workflow approach from edit branch and updating pipenv-setup action version to v4.2.1 to match main branch updates. The resolution maintains consistency with other workflow files (oldest-postgres.yml and single-postgres.yml) which use the same pattern: - pipenv-setup@v4.2.1 for environment setup - pipenv-run@v4.1.1 for package installation and test execution Co-authored-by: Cursor <cursoragent@cursor.com>
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.
for more information, see https://pre-commit.ci
- 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.
…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
|
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)
📝 WalkthroughWalkthroughAdded Windows compatibility to pytest-postgresql: platform-specific start command templates, a Windows process-termination helper, OS-aware locale handling, CI workflow updates for per-OS Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f5cc98c. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 63-80: The start command templates UNIX_PROC_START_COMMAND and
WINDOWS_PROC_START_COMMAND currently interpolate {executable} unquoted which
breaks on Windows when the pg_ctl path contains spaces; update both templates to
wrap the {executable} placeholder in double quotes and ensure any existing
surrounding quotes remain correct, and in the running() method replace the
unquoted subprocess.getstatusoutput(self.executable + ...) usage with a command
string that quotes self.executable (e.g. prefix the command with
"\"{self.executable}\" ...") so the shell sees the executable as a single token;
also adjust the command-formatting expectations in test_windows_compatibility.py
to expect quoted executable paths.
In `@tests/test_windows_compatibility.py`:
- Around line 212-233: The test test_postgres_options_with_double_quotes is
invalid because PostgreSQLExecutor.command cannot accept unescaped internal
double quotes; update the test to either remove it or fix the input and
assertions: call PostgreSQLExecutor with a safe postgres_options value (e.g. use
escaped quotes or an equivalent without inner double quotes) so the produced
executor.command reflects a syntactically valid shell argument, and assert the
fully quoted/escaped -o argument is present on executor.command (reference
PostgreSQLExecutor and its command property, and the test function name
test_postgres_options_with_double_quotes to locate the case).
🪄 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: 5b092fd1-0603-48c5-9061-2dc1c7f8ef24
📒 Files selected for processing (9)
.github/workflows/oldest-postgres.yml.github/workflows/single-postgres.yml.github/workflows/tests.ymlnewsfragments/1182.feature.rstpytest_postgresql/executor.pytests/conftest.pytests/test_executor.pytests/test_postgres_options_plugin.pytests/test_windows_compatibility.py
💤 Files with no reviewable changes (2)
- tests/conftest.py
- tests/test_postgres_options_plugin.py
- 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
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.
testing
Note
Medium Risk
Touches PostgreSQL process start/stop command construction and termination behavior across platforms; mistakes could cause test DB startup/shutdown failures, especially on Windows. CI matrix expansion mitigates this but increases workflow complexity.
Overview
Adds Windows compatibility to
PostgreSQLExecutorby introducing platform-specificpg_ctl startcommand templates (Unix vs Windows), adjusting locale selection (Con Windows), and improving quoting for executables/paths.Updates shutdown handling to avoid Unix-only process-group signaling on Windows by adding
_windows_terminate_process()and switchingstop()to callpg_ctl stopvia argument list plus a Windows termination fallback.Expands CI to run PostgreSQL 16–18 on Windows, dynamically discovers
pg_ctlpaths per OS, and standardizes pytest temp paths/artifact uploads via--basetemp. Adds extensive tests covering template selection, quoting edge cases, and Windows-specific stop/termination behavior.Reviewed by Cursor Bugbot for commit e8ed655. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores