Skip to content

Edit#2

Closed
tboy1337 wants to merge 46 commits into
mainfrom
edit
Closed

Edit#2
tboy1337 wants to merge 46 commits into
mainfrom
edit

Conversation

@tboy1337

@tboy1337 tboy1337 commented May 1, 2026

Copy link
Copy Markdown
Owner

Chore that needs to be done:

  • Add newsfragment pipenv run towncrier create [issue_number].[type].rst

Types 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 PostgreSQLExecutor and 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 PostgreSQLExecutor by introducing platform-specific pg_ctl start command templates, adjusting locale handling, running pg_ctl start synchronously 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 locate pg_ctl, standardize --basetemp/artifact paths, fix the PyPy libpq condition, and add a dedicated Windows PostgreSQL workflow wired into tests.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

    • Added Windows support for the PostgreSQL executor with platform-aware start/stop handling and command templating.
  • Bug Fixes

    • More reliable startup/shutdown on Windows, including improved process termination and readiness checks.
    • Better handling of complex paths, quoting and platform-specific options.
  • Tests

    • Expanded cross‑platform test coverage with extensive Windows compatibility and real-start integration tests.
  • Documentation

    • Added release note documenting Windows support.

tboy1337 and others added 30 commits April 15, 2026 09:55
…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.
- 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.
…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.
tboy1337 and others added 14 commits April 15, 2026 10:02
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.
- 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.
- 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
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
@coderabbitai

coderabbitai Bot commented May 1, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Windows 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

Cohort / File(s) Summary
GitHub Actions Workflows
.github/workflows/oldest-postgres.yml, .github/workflows/single-postgres.yml, .github/workflows/single-postgres-windows.yml, .github/workflows/tests.yml
Constrain libpq install to matrix Linux+PyPy; add a reusable Windows workflow (single-postgres-windows.yml) that finds pg_ctl.exe, runs pytest (including xdist) on windows-latest, uploads artifacts/coverage; wire a new Windows job (windows_postgres_17) into tests.yml.
Core Executor Logic
pytest_postgresql/executor.py
Introduce OS-specific start command templates (Unix vs Windows), alter quoting/option concatenation, implement synchronous Windows start via pg_ctl start -w using subprocess.run, platform-specific running() / check_subprocess() behaviour, and Windows termination that calls terminate/wait then kill with mirakuru state adjustments and AttributeError fallback.
Tests — executor & compatibility
tests/test_executor.py, tests/test_windows_compatibility.py
Expand tests: retry connect on transient startup, parent-dir creation for tmpdir, add parametrised template-selection unit tests, extensive Windows compatibility tests for command generation, quoting, UNC/space/Unicode edge cases, and process lifecycle; add real-start integration tests per platform (conditionally skipped).
Plugin Configuration Tests
tests/test_postgres_options_plugin.py
Normalise test.sql path for pytest.ini by replacing Windows backslashes with forward slashes.
Release Notes
newsfragments/1182.feature.rst
Add fragment documenting Windows support for PostgreSQLExecutor.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through scripts and subtle quirks,
Found pg_ctl.exe in Windows works,
Quoted paths and sockets neat,
Tests now dance on every seat—
A rabbit cheers for cross‑OS perks!

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The pull request title 'Edit' is vague and generic, providing no meaningful information about the substantial changes across workflows, executor platform support, and test coverage. Replace with a descriptive title that captures the main change, such as 'Add Windows support for PostgreSQLExecutor and CI integration' or 'Add Windows PostgreSQL executor support and CI workflows'.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Newsfragment Check ✅ Passed Newsfragment file 1182.feature.rst exists with valid filename format matching the pattern [number].[type].rst

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch edit

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment thread pytest_postgresql/executor.py
Comment thread tests/test_windows_compatibility.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
newsfragments/1182.feature.rst (1)

1-1: ⚡ Quick win

Make 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_process is probably too internal for CHANGES.rst. A shorter summary such as “Add Windows support for PostgreSQLExecutor, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1e9f1f6 and cc7fe02.

📒 Files selected for processing (9)
  • .github/workflows/oldest-postgres.yml
  • .github/workflows/single-postgres-windows.yml
  • .github/workflows/single-postgres.yml
  • .github/workflows/tests.yml
  • newsfragments/1182.feature.rst
  • pytest_postgresql/executor.py
  • tests/test_executor.py
  • tests/test_postgres_options_plugin.py
  • tests/test_windows_compatibility.py

Comment thread .github/workflows/oldest-postgres.yml Outdated
Comment thread pytest_postgresql/executor.py Outdated
Comment thread pytest_postgresql/executor.py Outdated
…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
@cursor

cursor Bot commented May 1, 2026

Copy link
Copy Markdown

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
pytest_postgresql/executor.py (1)

235-251: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Merge the parent environment in init_directory() too.

Line 246 and Line 251 still pass env=self.envvars directly to subprocess.check_output(). That replaces the child environment instead of extending it, so the Windows path can still fail during initdb before start() ever runs because PATH, SystemRoot, TEMP, and friends are dropped. Reuse the merged-env approach you already added in start() 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())
PY

Expected result: False C. The same behaviour on Windows also strips variables such as SystemRoot and TEMP.

🤖 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

📥 Commits

Reviewing files that changed from the base of the PR and between cc7fe02 and c96148a.

📒 Files selected for processing (4)
  • .github/workflows/oldest-postgres.yml
  • newsfragments/1182.feature.rst
  • pytest_postgresql/executor.py
  • tests/test_windows_compatibility.py
✅ Files skipped from review due to trivial changes (1)
  • newsfragments/1182.feature.rst

@tboy1337 tboy1337 closed this May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant