Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Open
tboy1337 wants to merge 36 commits intodbfixtures:mainfrom
Open
Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182tboy1337 wants to merge 36 commits intodbfixtures:mainfrom
tboy1337 wants to merge 36 commits intodbfixtures:mainfrom
Conversation
Open
fizyk
reviewed
Sep 4, 2025
Contributor
📝 WalkthroughWalkthroughPlatform-aware start command templates and locale handling added; module logger and Windows-specific termination helper implemented; stop() updated to use list args with Windows fallback and force‑kill; extensive Windows and cross‑platform tests plus CI changes for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as PostgreSQLExecutor
participant OS as OperatingSystem
participant Proc as PostgreSQLProcess
Client->>Executor: stop()
Executor->>Executor: detect platform & prepare stop args
alt Platform == "Windows"
Executor->>Executor: call _windows_terminate_process()
Executor->>Proc: proc.terminate()
Proc-->>Executor: may stop
alt still running after timeout
Executor->>Proc: proc.kill()
Proc-->>Executor: killed
end
else Non-Windows
Executor->>OS: call super().stop() (uses os.killpg)
alt super().stop() raises AttributeError / unsupported
Executor->>Executor: fallback -> _windows_terminate_process()
Executor->>Proc: proc.terminate()/kill()
end
end
Executor-->>Client: stop complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 263-283: In stop(self, sig: Optional[int] = None, exp_sig:
Optional[int] = None) replace the subprocess.check_output call that builds a
shell string with a list-style invocation (e.g., [self.executable, "stop", "-D",
self.datadir, "-m", "f"]) and remove shell=True so subprocess runs the
executable directly; this avoids breaking paths with spaces and removes the
shell injection risk while keeping the same arguments and behavior before the
platform-specific termination logic (_windows_terminate_process / super().stop).
- Around line 53-66: The _get_base_command constructs the -o string with
unix_socket_directories unquoted which breaks for paths with spaces; update
_get_base_command to detect whitespace in the unixsocketdir variable and wrap
that value in double quotes (or escape quotes) when building the -o option so
the produced fragment becomes -c unix_socket_directories="/path/with space"
(referencing the unix_socket_directories token and BASE_PROC_START_COMMAND
assignment), and add a unit test that passes a datadir/unixsocketdir containing
a space to ensure the command line includes the quoted socket directory (or
alternatively, add explicit validation/documentation and raise a clear error
when whitespace is present).
🧹 Nitpick comments (1)
pytest_postgresql/executor.py (1)
231-262: Tidy up the termination helper API surface.
sigis unused in_windows_terminate_process, and_unix_terminate_processis currently unused. Either wire_unix_terminate_processintostop()for symmetry and usesig, or drop/rename the parameter to avoid dead code and lint noise.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
5d4fb91 to
efa4633
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 35-39: The test assertions in tests/test_windows_compatibility.py
don't match the actual string produced by _get_base_command(): update the
assertions to expect the escaped-quote form for unix_socket_directories (i.e.,
include the backslash-escaped double quotes around {unixsocketdir}) and assert
that the non-quoted form is not present; specifically change the positive
assertion referencing unix_socket_directories to look for
unix_socket_directories=\"{unixsocketdir}\" and keep the negative assertion for
unix_socket_directories='{unixsocketdir}' to ensure consistency with
_get_base_command().
- Around line 189-204: The test's expected_parts entry for
unix_socket_directories doesn't match the actual command string (which includes
escaped quotes), so update the expectation in the test to match the
implementation: replace the "-c unix_socket_directories=C:/temp/socket" entry
with the escaped-quote form that appears in executor.command (e.g., -c
unix_socket_directories=\"C:/temp/socket\"), keeping the rest of the
expected_parts and the loop over executor.command unchanged; reference the test
variable executor.command and the expected_parts list when making the change.
🧹 Nitpick comments (2)
pytest_postgresql/executor.py (2)
25-25: Unused importsignal.The
signalmodule is imported but not used anywhere in the file. This appears to be a leftover from development.🧹 Suggested fix
-import signal
250-269: Good refactor to list-based subprocess call.The change from shell string to list format (
[self.executable, "stop", "-D", self.datadir, "-m", "f"]) properly addresses the previous security concern and handles paths with spaces correctly.One minor observation: the
AttributeErrorfallback (lines 263-268) uses string matching"killpg" in str(e)which is somewhat fragile. Consider whether this fallback is still needed given the explicitplatform.system()check now handles Windows upfront.
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
b02c61b to
4a0b22e
Compare
fizyk
reviewed
Jan 23, 2026
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
059e4bc to
54140ac
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pytest_postgresql/executor.py`:
- Around line 226-246: The parameter sig in _windows_terminate_process is
intentionally unused and triggers Ruff ARG002; either rename it to _sig to
indicate it’s intentionally unused or keep sig and add a local lint suppression
(e.g., append a noqa comment for ARG002) where the function is defined to
silence the warning; update the function signature (or add the noqa) in the
_windows_terminate_process definition and run linters to confirm the ARG002
warning is resolved.
…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
fizyk
approved these changes
Jan 29, 2026
…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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/single-postgres.yml (1)
82-86:⚠️ Potential issue | 🟡 MinorArtifact upload path is Unix-only.
/tmp/pytest-of-runner/**on line 86 won't match anything on Windows. This is gated behindif: failure()so it won't fail the build, but Windows test failure artefacts will be silently lost. Consider adding a platform-conditional path or using${{ runner.temp }}.
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 59-62: The workflow hardcodes POSTGRESQL_EXEC to
/usr/lib/postgresql/... which breaks macOS runners; change the step that sets
POSTGRESQL_EXEC (the block under the if: runner.os != 'Windows') to detect
pg_ctl on Unix-like systems instead of a Debian path — use command -v pg_ctl and
if that fails try macOS Homebrew locations (e.g.,
/opt/homebrew/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl and
/usr/local/opt/postgresql@${{ inputs.postgresql }}/bin/pg_ctl) or set a fallback
to the current Debian path; ensure the final value exported as POSTGRESQL_EXEC
is the first existing pg_ctl found so CI on macos_postgres_* with
ankane/setup-postgres works correctly.
🧹 Nitpick comments (4)
pytest_postgresql/executor.py (2)
226-246: Broadly swallowing exceptions in cleanup is acceptable but consider logging.The
except (OSError, AttributeError): passon lines 244–246 silently discards allOSErrorvariants (including permission errors) andAttributeError. For a teardown helper this is pragmatic, but a debug-level log would aid troubleshooting when processes fail to terminate.
261-266: String-basedAttributeErrormatching is fragile.Checking
"killpg" in str(e)couples the fallback to the exact wording of CPython's error message, which could change across versions or implementations. Consider catching the missing attribute directly.Proposed fix
except AttributeError as e: - # Fallback for edge cases where os.killpg doesn't exist - if "killpg" in str(e): + # Fallback for edge cases where os.killpg doesn't exist (e.g. Windows) + if not hasattr(os, "killpg"): self._windows_terminate_process(sig) else: raisetests/conftest.py (1)
7-9: Remove the commented-out import rather than leaving dead code.The commented-out line still triggers a Ruff warning (RUF100) for the stale
noqadirective. Since the comment on line 8 already explains the rationale, the old import line adds no value.Proposed fix
# Plugin is registered via entry point in pyproject.toml - no need to import here -# from pytest_postgresql.plugin import * # noqa: F403,F401tests/test_windows_compatibility.py (1)
74-98: Patch target forsubprocess.check_outputshould be where it's looked up.The patch
patch("subprocess.check_output")works becauseexecutor.pyusesimport subprocessand thensubprocess.check_output(...), so patching the module attribute is fine. However, best practice is to patch where the name is used:pytest_postgresql.executor.subprocess.check_output. This avoids accidentally affecting other code if tests are ever run in parallel or the import style changes.This applies to all test methods in this class that patch
subprocess.check_output.
…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.
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_windows_compatibility.py`:
- Around line 127-155: The test fails on Unix because os.killpg exists so the
stop() except branch re-raises; to exercise the fallback, remove killpg from the
executor module's os before calling stop: inside the test's with-block, import
sys and call delattr(sys.modules['pytest_postgresql.executor'].os, "killpg") (or
use monkeypatch.delattr on that module's os) so that hasattr(os, "killpg") is
False when PostgreSQLExecutor.stop() runs; keep references to
PostgreSQLExecutor.stop, os.killpg, and _windows_terminate_process to locate the
code to change.
🧹 Nitpick comments (3)
pytest_postgresql/executor.py (2)
32-34: Logger initialisation breaks import grouping.The
logger = logging.getLogger(__name__)statement is sandwiched between twomirakuruimports. Move it after all top-level imports to keep the import block contiguous and PEP 8–compliant.Suggested reordering
from mirakuru import TCPExecutor - -logger = logging.getLogger(__name__) from mirakuru.exceptions import ProcessFinishedWithError from packaging.version import parse from pytest_postgresql.exceptions import ExecutableMissingException, PostgreSQLUnsupported + +logger = logging.getLogger(__name__)
229-254:_windows_terminate_processlooks good overall; minor nit on redundantpass.The graceful-then-force-kill approach is sound. However, the
passon line 254 is redundant — thelogger.debug(...)call already satisfies the except block body.Suggested fix
logger.debug( "Exception during Windows process termination: %s: %s", type(e).__name__, e, ) - passtests/test_windows_compatibility.py (1)
74-98: Windows stop test: consider asserting the exact arguments passed tocheck_output.The test asserts
mock_subprocess.assert_called_once()but doesn't verify the arguments. Sincestop()now uses list-form args ([self.executable, "stop", "-D", self.datadir, "-m", "f"]), asserting the call args would catch regressions in the command construction.Optional enhancement
# Should call pg_ctl stop and Windows terminate - mock_subprocess.assert_called_once() + mock_subprocess.assert_called_once_with( + ["/path/to/pg_ctl", "stop", "-D", "/tmp/data", "-m", "f"], + ) mock_terminate.assert_called_once()
- 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.
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/dockerised-postgres.yml:
- Around line 52-61: The workflow uses
fizyk/actions-reuse/.github/actions/pipenv-run@v4.1.1 while another workflow
uses pipenv-run@v4.2.1 causing inconsistent behavior; update the pipenv-run
invocation in this workflow (the step named "Run test noproc fixture on docker"
that references fizyk/actions-reuse/.github/actions/pipenv-run) to the same
version (e.g., `@v4.2.1` or the chosen canonical/latest tag) so all workflows use
a single pipenv-run version, and verify other workflows reference the same tag.
In @.github/workflows/single-postgres.yml:
- Around line 97-104: The workflows use the same action
fizyk/actions-reuse/.github/actions/pipenv-run at different tags (v4.2.1 vs
v4.1.1); pick a single pinned tag and make both workflow steps use that same tag
to avoid drift (update the uses: value in the Run test and Run xdist test steps
to match the version used in the other CI workflow and run the CI to verify no
behavioral changes).
- Around line 94-96: The conditional for the "install libpq" step uses
contains(inputs.python-versions, 'pypy') which inspects the entire input string
and causes the step to run for every matrix job whenever any entry contains
"pypy"; change the if to check the matrix entry instead (e.g., use
startsWith(matrix.python, 'pypy') or contains(matrix.python, 'pypy')) and keep
the runner.os == 'Linux' part so the step only runs on Linux PyPy jobs; update
the step identified as "install libpq" to use that matrix-based condition.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dockerised-postgres.yml: - Around line 52-61: The workflow uses fizyk/actions-reuse/.github/actions/pipenv-run@v4.1.1 while another workflow uses pipenv-run@v4.2.1 causing inconsistent behavior; update the pipenv-run invocation in this workflow (the step named "Run test noproc fixture on docker" that references fizyk/actions-reuse/.github/actions/pipenv-run) to the same version (e.g., `@v4.2.1` or the chosen canonical/latest tag) so all workflows use a single pipenv-run version, and verify other workflows reference the same tag. In @.github/workflows/single-postgres.yml: - Around line 97-104: The workflows use the same action fizyk/actions-reuse/.github/actions/pipenv-run at different tags (v4.2.1 vs v4.1.1); pick a single pinned tag and make both workflow steps use that same tag to avoid drift (update the uses: value in the Run test and Run xdist test steps to match the version used in the other CI workflow and run the CI to verify no behavioral changes). - Around line 94-96: The conditional for the "install libpq" step uses contains(inputs.python-versions, 'pypy') which inspects the entire input string and causes the step to run for every matrix job whenever any entry contains "pypy"; change the if to check the matrix entry instead (e.g., use startsWith(matrix.python, 'pypy') or contains(matrix.python, 'pypy')) and keep the runner.os == 'Linux' part so the step only runs on Linux PyPy jobs; update the step identified as "install libpq" to use that matrix-based condition..github/workflows/dockerised-postgres.yml (1)
52-61: Inconsistentpipenv-runaction version across workflows.This file uses
fizyk/actions-reuse/.github/actions/pipenv-run@v4.1.1(line 59), whilesingle-postgres.ymlusespipenv-run@v4.2.1(line 98). Consider aligning both to the same (latest) version to avoid subtle behavioural differences between CI workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dockerised-postgres.yml around lines 52 - 61, The workflow uses fizyk/actions-reuse/.github/actions/pipenv-run@v4.1.1 while another workflow uses pipenv-run@v4.2.1 causing inconsistent behavior; update the pipenv-run invocation in this workflow (the step named "Run test noproc fixture on docker" that references fizyk/actions-reuse/.github/actions/pipenv-run) to the same version (e.g., `@v4.2.1` or the chosen canonical/latest tag) so all workflows use a single pipenv-run version, and verify other workflows reference the same tag..github/workflows/single-postgres.yml (2)
97-104:pipenv-runaction pinned tov4.2.1here butv4.1.1indockerised-postgres.yml.Both workflows use
fizyk/actions-reuse/.github/actions/pipenv-runbut at different versions. Consider aligning them to avoid subtle behavioural drift between the two CI paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/single-postgres.yml around lines 97 - 104, The workflows use the same action fizyk/actions-reuse/.github/actions/pipenv-run at different tags (v4.2.1 vs v4.1.1); pick a single pinned tag and make both workflow steps use that same tag to avoid drift (update the uses: value in the Run test and Run xdist test steps to match the version used in the other CI workflow and run the CI to verify no behavioral changes).
94-96:libpqinstall condition triggers for all matrix entries, not just PyPy.
contains(inputs.python-versions, 'pypy')checks the entire JSON array string, so when any entry matches (e.g."pypy-3.11"), the step runs for every matrix job — including CPython ones. Harmless but wasteful. If you want it scoped to PyPy runners only:Proposed fix
- if: ${{ contains(inputs.python-versions, 'pypy') && runner.os == 'Linux' }} + if: ${{ startsWith(matrix.python-version, 'pypy') && runner.os == 'Linux' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/single-postgres.yml around lines 94 - 96, The conditional for the "install libpq" step uses contains(inputs.python-versions, 'pypy') which inspects the entire input string and causes the step to run for every matrix job whenever any entry contains "pypy"; change the if to check the matrix entry instead (e.g., use startsWith(matrix.python, 'pypy') or contains(matrix.python, 'pypy')) and keep the runner.os == 'Linux' part so the step only runs on Linux PyPy jobs; update the step identified as "install libpq" to use that matrix-based condition.
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
… 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_windows_compatibility.py (1)
527-530: Inconsistentplatform.systempatch target withtest_executor.py.
test_executor.pyusespatch("pytest_postgresql.executor.platform.system", ...)(module-qualified), while this file usespatch("platform.system", ...)(global). Both are functionally equivalent because Python modules are singletons, but the inconsistency is confusing. Aligning to the module-qualified form is the conventional recommendation when patching a name used inside a specific module.♻️ Suggested change (applies to both `test_stop_method_windows` and `test_stop_method_unix`)
- patch("platform.system", return_value="Windows"), + patch("pytest_postgresql.executor.platform.system", return_value="Windows"),- patch("platform.system", return_value="Linux"), + patch("pytest_postgresql.executor.platform.system", return_value="Linux"),Also applies to: 557-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_windows_compatibility.py` around lines 527 - 530, The test patches use patch("platform.system", ...) which is inconsistent with test_executor.py; change the patch target to the module-qualified name where the code under test references it (patch("pytest_postgresql.executor.platform.system", return_value="Windows") and similarly for the Unix case) so the patch applies to the symbol used in the executor module; update both tests that patch platform.system (references: test_stop_method_windows and test_stop_method_unix) to use the module-qualified target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_executor.py`:
- Line 214: The tests pass hardcoded string literals to password= (e.g.,
password="test pass") which triggers Ruff S106; fix by appending an inline noqa
comment to each occurrence — add `# noqa: S106` to the password= lines in
tests/test_executor.py (the occurrences that set password="test pass" and the
other two literal passwords) so Ruff ignores S106 in these test-only
credentials, or alternatively add a per-file ignore for S106 under
[tool.ruff.lint.per-file-ignores] in pyproject.toml for tests/test_executor.py
if you prefer a file-level suppression.
- Around line 125-136: The test currently only checks for
"unix_socket_directories=" in executor.command but not that the socket path is
wrapped in single quotes; update the assertion(s) that reference
executor.command (the ones near the current block and the repeated occurrences
around the other noted locations) to assert the stricter form that includes
single quotes around the value (e.g., check for the substring
"unix_socket_directories='"). Keep the Windows assertions unchanged and ensure
these tightened assertions are present alongside the existing
assert_executor_start_stop usage to validate the template selection on Unix-like
platforms.
In `@tests/test_windows_compatibility.py`:
- Around line 282-287: The comment contradicts the assertion: either remove/fix
the comment or stop expecting a trailing space before the closing quote. Update
the test by changing expected_opts to not include the trailing space (e.g. end
with \'/tmp/socket\'" instead of \'/tmp/socket\' ") and/or modify the code that
builds the -o parameter (the template that uses postgres_options) to trim any
extra space when postgres_options is empty so that the produced command (checked
via expected_opts and command) contains no space before the closing quote.
---
Nitpick comments:
In `@tests/test_windows_compatibility.py`:
- Around line 527-530: The test patches use patch("platform.system", ...) which
is inconsistent with test_executor.py; change the patch target to the
module-qualified name where the code under test references it
(patch("pytest_postgresql.executor.platform.system", return_value="Windows") and
similarly for the Unix case) so the patch applies to the symbol used in the
executor module; update both tests that patch platform.system (references:
test_stop_method_windows and test_stop_method_unix) to use the module-qualified
target.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
newsfragments/1182.feature.rsttests/test_executor.pytests/test_postgres_options_plugin.pytests/test_windows_compatibility.py
💤 Files with no reviewable changes (1)
- tests/test_postgres_options_plugin.py
… 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.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_executor.py (1)
354-357:⚠️ Potential issue | 🟡 MinorWeak assertion — missing single-quote verification for
unix_socket_directories.Line 356 checks
"unix_socket_directories=" in executor.commandwithout verifying the surrounding single quotes. This would pass even if the template accidentally dropped the quotes. The stricter form used at lines 133, 178, and 229 should be applied here for consistency.The same issue applies to line 392.
Suggested fix
# Verify Unix template is used - assert "unix_socket_directories=" in executor.command + assert "unix_socket_directories='" in executor.command assert "log_destination='stderr'" in executor.commandAlso at line 392:
# Verify Unix template is used - assert "unix_socket_directories=" in executor.command + assert "unix_socket_directories='" in executor.command assert "log_destination='stderr'" in executor.command🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` around lines 354 - 357, The assertion is too loose: update the test that checks executor.command to assert the unix_socket_directories entry includes the surrounding single quotes (i.e., match "unix_socket_directories='...'" rather than just "unix_socket_directories=") so it mirrors the stricter checks used elsewhere; change the assertion that references executor.command and apply the same stricter assertion to the other occurrence noted (the second executor.command check) so both verify the single-quote-delimited value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_executor.py`:
- Around line 354-357: The assertion is too loose: update the test that checks
executor.command to assert the unix_socket_directories entry includes the
surrounding single quotes (i.e., match "unix_socket_directories='...'" rather
than just "unix_socket_directories=") so it mirrors the stricter checks used
elsewhere; change the assertion that references executor.command and apply the
same stricter assertion to the other occurrence noted (the second
executor.command check) so both verify the single-quote-delimited value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pytest_postgresql/executor.pytests/test_executor.pytests/test_windows_compatibility.py
…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.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_executor.py (1)
214-214: Ruff S106 hardcoded password warnings remain unaddressed.Lines 214, 312, 351, and 387 still trigger Ruff S106. A previous review already flagged this — consider adding
# noqa: S106to these lines or configuring a per-file ignore inpyproject.tomlfor the test suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` at line 214, Replace the hardcoded password occurrences in the tests by marking them as intentional for the test suite: add a trailing inline noqa to each literal password assignment (e.g., append "# noqa: S106" to the password="test pass" arguments in the failing tests) or alternatively add a per-file ignore for S106 in pyproject.toml for tests/test_executor.py; locate these occurrences by searching for the password="..." arguments used in the test helper calls within tests/test_executor.py (the same literal appears at the four reported sites) and apply one consistent suppression approach across all four instances.
🧹 Nitpick comments (1)
tests/test_executor.py (1)
139-179: Clean unit test for template selection — consider adding FreeBSD for completeness.The mock approach is sound: patching
pytest_postgresql.executor.platform.systembefore__init__correctly influences template selection without requiring a real PostgreSQL start. The assertions align with the Unix and Windows template definitions.Minor nit:
"FreeBSD"is recognised as a valid Unix-like platform intest_actual_postgresql_start_unix(line 325) but is not included in this parametrised set. Since it should select the same Unix template as Linux, adding it would improve coverage symmetry at negligible cost.Optional: add FreeBSD to the parametrised set
`@pytest.mark.parametrize`( "platform_name", - ["Windows", "Linux", "Darwin"], + ["Windows", "Linux", "Darwin", "FreeBSD"], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` around lines 139 - 179, Add "FreeBSD" to the parametrized platform list in test_executor_platform_template_selection so the test covers the same Unix-like path as test_actual_postgresql_start_unix; update the pytest.mark.parametrize ("platform_name", ["Windows", "Linux", "Darwin"]) to include "FreeBSD" so the mocked platform.system used when instantiating PostgreSQLExecutor exercises the Unix/Darwin branch and the existing assertions against executor.command remain valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_executor.py`:
- Line 214: Replace the hardcoded password occurrences in the tests by marking
them as intentional for the test suite: add a trailing inline noqa to each
literal password assignment (e.g., append "# noqa: S106" to the password="test
pass" arguments in the failing tests) or alternatively add a per-file ignore for
S106 in pyproject.toml for tests/test_executor.py; locate these occurrences by
searching for the password="..." arguments used in the test helper calls within
tests/test_executor.py (the same literal appears at the four reported sites) and
apply one consistent suppression approach across all four instances.
---
Nitpick comments:
In `@tests/test_executor.py`:
- Around line 139-179: Add "FreeBSD" to the parametrized platform list in
test_executor_platform_template_selection so the test covers the same Unix-like
path as test_actual_postgresql_start_unix; update the pytest.mark.parametrize
("platform_name", ["Windows", "Linux", "Darwin"]) to include "FreeBSD" so the
mocked platform.system used when instantiating PostgreSQLExecutor exercises the
Unix/Darwin branch and the existing assertions against executor.command remain
valid.
- 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.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_executor.py (1)
214-214: Hardcodedpassword=strings still trigger Ruff S106 on lines 214, 312, 351, and 387.This was raised previously and is still unresolved. Either suppress inline or via
pyproject.toml.🔧 Proposed inline suppression
- password="test pass", + password="test pass", # noqa: S106Apply the same
# noqa: S106comment to lines 312, 351, and 387.Alternatively, add a file-level ignore to
pyproject.toml:[tool.ruff.lint.per-file-ignores] "tests/**" = ["S106"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` at line 214, Hardcoded password literals like password="test pass" in the test file are triggering Ruff S106; either add an inline suppression (# noqa: S106) to each occurrence of the password="test pass" literal in the tests, or add a per-file ignore in pyproject.toml under [tool.ruff.lint.per-file-ignores] (e.g. "tests/**" = ["S106"]) to silence S106 for test files; update whichever code locations contain the password="test pass" literal accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_executor.py`:
- Line 214: Hardcoded password literals like password="test pass" in the test
file are triggering Ruff S106; either add an inline suppression (# noqa: S106)
to each occurrence of the password="test pass" literal in the tests, or add a
per-file ignore in pyproject.toml under [tool.ruff.lint.per-file-ignores] (e.g.
"tests/**" = ["S106"]) to silence S106 for test files; update whichever code
locations contain the password="test pass" literal accordingly.
fizyk
reviewed
Feb 23, 2026
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/single-postgres.yml (1)
100-109:⚠️ Potential issue | 🟡 MinorAlign
--basetempwith the artefact glob to avoid missing failure artefacts.Line [100] and Line [104] set
--basetemp="${{ runner.temp }}", but Line [109] uploads only${{ runner.temp }}/pytest-*/**. With explicit--basetemp, pytest temp dirs are typically created directly under the provided base temp path, so this glob can miss relevant files.Proposed fix
- command: pytest -svv -p no:xdist --postgresql-exec="${{ env.POSTGRESQL_EXEC }}" -k "not docker" --cov-report=xml --basetemp="${{ runner.temp }}" + command: pytest -svv -p no:xdist --postgresql-exec="${{ env.POSTGRESQL_EXEC }}" -k "not docker" --cov-report=xml --basetemp="${{ runner.temp }}/pytest-tmp" @@ - command: pytest -n auto --dist loadgroup --max-worker-restart 0 --postgresql-exec="${{ env.POSTGRESQL_EXEC }}" -k "not docker" --cov-report=xml:coverage-xdist.xml --basetemp="${{ runner.temp }}" + command: pytest -n auto --dist loadgroup --max-worker-restart 0 --postgresql-exec="${{ env.POSTGRESQL_EXEC }}" -k "not docker" --cov-report=xml:coverage-xdist.xml --basetemp="${{ runner.temp }}/pytest-tmp" @@ - path: ${{ runner.temp }}/pytest-*/** + path: ${{ runner.temp }}/pytest-tmp/**#!/bin/bash set -euo pipefail file=".github/workflows/single-postgres.yml" echo "Checking pytest basetemp usage:" rg -n --fixed-strings '--basetemp="${{ runner.temp }}"' "$file" echo echo "Checking artifact path glob:" rg -n --fixed-strings 'path: ${{ runner.temp }}/pytest-*/**' "$file" echo echo "If both matches are present, basetemp/artifact path alignment should be rechecked."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/single-postgres.yml around lines 100 - 109, The artifact upload can miss pytest temp files because pytest is given --basetemp="${{ runner.temp }}" while the upload step uses path: ${{ runner.temp }}/pytest-*/**, so adjust one side to match: either set --basetemp to a pytest-specific subdir (e.g. --basetemp="${{ runner.temp }}/pytest-<something>") or broaden the upload-artifact glob to capture all files under the base temp (e.g. path: ${{ runner.temp }}/**). Update the step that runs pytest (keep --basetemp="${{ runner.temp }}" if you prefer) and/or update the actions/upload-artifact with the new glob so that artifacts produced under --basetemp="${{ runner.temp }}" are reliably uploaded.
🧹 Nitpick comments (2)
tests/test_windows_compatibility.py (1)
63-74: Consider a small executor-builder helper to reduce repeated setup.A local helper/fixture for
PostgreSQLExecutor(...)construction would remove a lot of repetition and make intent-focused assertions easier to scan.Also applies to: 81-92, 103-113, 126-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_windows_compatibility.py` around lines 63 - 74, Tests repeat constructing PostgreSQLExecutor with similar args; add a small helper (e.g., build_executor or executor_factory) or a pytest fixture that encapsulates the common setup (including the patch of pytest_postgresql.executor.platform.system) and returns a ready PostgreSQLExecutor instance; update the test cases that currently call PostgreSQLExecutor directly (the instances created inside the patched context) to use this helper/fixture and pass only differing parameters like executable, datadir or startparams so the test bodies become shorter and intent-focused.tests/test_executor.py (1)
288-401: Optional tidy-up: consolidate OS-specific start tests with parametrisation.
test_actual_postgresql_start_windows,test_actual_postgresql_start_unix, andtest_actual_postgresql_start_darwinshare almost identical setup; combining with parametrised expectations would reduce duplication while keeping intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_executor.py` around lines 288 - 401, Consolidate the three near-identical tests (test_actual_postgresql_start_windows, test_actual_postgresql_start_unix, test_actual_postgresql_start_darwin) into a single parametrized test (e.g., test_actual_postgresql_start_by_os) that yields a tuple per OS with expected assertions (socket substring, log substring, optional locale dict) and use pytest.param(..., marks=pytest.mark.skipif(platform.system()!= "<OS>", reason=...)) to skip non-matching platforms; inside the test reuse the same setup (get_config, process._pg_exe, process._pg_port, process._prepare_dir, PostgreSQLExecutor, assert_executor_start_stop) and assert on executor.command and executor.envvars according to the parametrized expectations so all three behaviors are tested without duplicated code, referencing symbols PostgreSQLExecutor, get_config, process._pg_exe, process._pg_port, process._prepare_dir, and assert_executor_start_stop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/single-postgres.yml:
- Around line 100-109: The artifact upload can miss pytest temp files because
pytest is given --basetemp="${{ runner.temp }}" while the upload step uses path:
${{ runner.temp }}/pytest-*/**, so adjust one side to match: either set
--basetemp to a pytest-specific subdir (e.g. --basetemp="${{ runner.temp
}}/pytest-<something>") or broaden the upload-artifact glob to capture all files
under the base temp (e.g. path: ${{ runner.temp }}/**). Update the step that
runs pytest (keep --basetemp="${{ runner.temp }}" if you prefer) and/or update
the actions/upload-artifact with the new glob so that artifacts produced under
--basetemp="${{ runner.temp }}" are reliably uploaded.
---
Nitpick comments:
In `@tests/test_executor.py`:
- Around line 288-401: Consolidate the three near-identical tests
(test_actual_postgresql_start_windows, test_actual_postgresql_start_unix,
test_actual_postgresql_start_darwin) into a single parametrized test (e.g.,
test_actual_postgresql_start_by_os) that yields a tuple per OS with expected
assertions (socket substring, log substring, optional locale dict) and use
pytest.param(..., marks=pytest.mark.skipif(platform.system()!= "<OS>",
reason=...)) to skip non-matching platforms; inside the test reuse the same
setup (get_config, process._pg_exe, process._pg_port, process._prepare_dir,
PostgreSQLExecutor, assert_executor_start_stop) and assert on executor.command
and executor.envvars according to the parametrized expectations so all three
behaviors are tested without duplicated code, referencing symbols
PostgreSQLExecutor, get_config, process._pg_exe, process._pg_port,
process._prepare_dir, and assert_executor_start_stop.
In `@tests/test_windows_compatibility.py`:
- Around line 63-74: Tests repeat constructing PostgreSQLExecutor with similar
args; add a small helper (e.g., build_executor or executor_factory) or a pytest
fixture that encapsulates the common setup (including the patch of
pytest_postgresql.executor.platform.system) and returns a ready
PostgreSQLExecutor instance; update the test cases that currently call
PostgreSQLExecutor directly (the instances created inside the patched context)
to use this helper/fixture and pass only differing parameters like executable,
datadir or startparams so the test bodies become shorter and intent-focused.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/single-postgres.ymltests/test_executor.pytests/test_windows_compatibility.py
- 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.
Member
|
@tboy1337 I've merged main into your branch to have the tests run |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
80-100: Consider decoupling Windows jobs from Linux prerequisites to keep Windows signal available.Right now, a Linux failure can skip all Windows jobs, which may hide Windows-specific regressions. Keeping only Windows-internal ordering still preserves staged execution.
♻️ Suggested workflow adjustment
windows_postgres_18: - needs: [postgresql_18] uses: ./.github/workflows/single-postgres.yml with: postgresql: 18 os: windows-latest python-versions: '["3.12", "3.13", "3.14"]' secrets: codecov_token: ${{ secrets.CODECOV_TOKEN }} windows_postgres_17: - needs: [postgresql_17, windows_postgres_18] + needs: [windows_postgres_18] uses: ./.github/workflows/single-postgres.yml with: postgresql: 17 os: windows-latest python-versions: '["3.12", "3.13", "3.14"]' secrets: codecov_token: ${{ secrets.CODECOV_TOKEN }} windows_postgres_16: - needs: [postgresql_16, windows_postgres_17] + needs: [windows_postgres_17] uses: ./.github/workflows/single-postgres.yml with: postgresql: 16 os: windows-latest python-versions: '["3.13", "3.14"]'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 80 - 100, The Windows matrix jobs are currently gated on Linux "postgresql_*" jobs via the needs arrays (e.g., windows_postgres_18 needs: [postgresql_18]), which causes Linux failures to block Windows runs; change the needs for these jobs so they only depend on other Windows jobs to preserve staged ordering but keep Windows signal available: remove references to postgresql_18/17/16 from the needs list for windows_postgres_18/windows_postgres_17/windows_postgres_16 and instead chain the Windows jobs (e.g., windows_postgres_18 has no Linux needs, windows_postgres_17 needs: [windows_postgres_18], windows_postgres_16 needs: [windows_postgres_17]) while leaving each job's uses, with, and secrets blocks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 80-100: The Windows matrix jobs are currently gated on Linux
"postgresql_*" jobs via the needs arrays (e.g., windows_postgres_18 needs:
[postgresql_18]), which causes Linux failures to block Windows runs; change the
needs for these jobs so they only depend on other Windows jobs to preserve
staged ordering but keep Windows signal available: remove references to
postgresql_18/17/16 from the needs list for
windows_postgres_18/windows_postgres_17/windows_postgres_16 and instead chain
the Windows jobs (e.g., windows_postgres_18 has no Linux needs,
windows_postgres_17 needs: [windows_postgres_18], windows_postgres_16 needs:
[windows_postgres_17]) while leaving each job's uses, with, and secrets blocks
intact.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/oldest-postgres.yml.github/workflows/single-postgres.yml.github/workflows/tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/oldest-postgres.yml
…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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_windows_compatibility.py (2)
13-25: Template quote assertions are partially duplicated across two tests.
test_unix_command_template_has_single_quotesandtest_unix_command_template_includes_unix_socket_directoriesoverlap on the sameunix_socket_directoriesexpectation. Consider merging or narrowing one test to reduce redundancy and keep failure signals sharper.Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_windows_compatibility.py` around lines 13 - 25, Two tests currently duplicate the same assertion about unix_socket_directories; merge or narrow them to remove redundancy: update test_unix_command_template_has_single_quotes and test_unix_command_template_includes_unix_socket_directories so only one asserts "unix_socket_directories='{unixsocketdir}'" against PostgreSQLExecutor.UNIX_PROC_START_COMMAND (keep the other test focused on a distinct expectation such as only asserting "log_destination='stderr'" or other quote behavior), or combine both expectations into a single test that validates both the single-quote usage and the unix_socket_directories value to avoid duplicate failure signals.
568-600: Usemonkeypatch.delattrinstead of manual attribute mutation and restoration.This test currently mutates the global
os.killpgdirectly with manual try/finally cleanup. Using pytest'smonkeypatch.delattrensures automatic cleanup and eliminates the risk of state leakage if assertions fail mid-test. Sinceos.killpgis conditionally present, useraising=Falseto gracefully handle environments where it doesn't exist.Suggested refactor
- def test_stop_method_fallback_on_killpg_error(self) -> None: + def test_stop_method_fallback_on_killpg_error(self, monkeypatch) -> None: @@ - # Temporarily remove os.killpg so hasattr(os, "killpg") returns False - real_killpg = getattr(os, "killpg", None) - try: - if real_killpg is not None: - delattr(os, "killpg") - result = executor.stop() - finally: - if real_killpg is not None: - os.killpg = real_killpg + # Temporarily remove killpg in a test-safe way + monkeypatch.delattr("pytest_postgresql.executor.os.killpg", raising=False) + result = executor.stop()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_windows_compatibility.py` around lines 568 - 600, Replace the manual deletion/restoration of os.killpg in test_stop_method_fallback_on_killpg_error with pytest's monkeypatch.delattr to avoid global state leakage: add the monkeypatch fixture to the test signature, remove the try/finally block that manipulates os.killpg, and call monkeypatch.delattr(os, "killpg", raising=False) before invoking executor.stop(); keep the existing patches (patching subprocess.check_output, platform.system, TCPExecutor.stop side_effect, and patch.object(executor, "_windows_terminate_process")) so the test behavior is unchanged while cleanup is automatic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_windows_compatibility.py`:
- Around line 13-25: Two tests currently duplicate the same assertion about
unix_socket_directories; merge or narrow them to remove redundancy: update
test_unix_command_template_has_single_quotes and
test_unix_command_template_includes_unix_socket_directories so only one asserts
"unix_socket_directories='{unixsocketdir}'" against
PostgreSQLExecutor.UNIX_PROC_START_COMMAND (keep the other test focused on a
distinct expectation such as only asserting "log_destination='stderr'" or other
quote behavior), or combine both expectations into a single test that validates
both the single-quote usage and the unix_socket_directories value to avoid
duplicate failure signals.
- Around line 568-600: Replace the manual deletion/restoration of os.killpg in
test_stop_method_fallback_on_killpg_error with pytest's monkeypatch.delattr to
avoid global state leakage: add the monkeypatch fixture to the test signature,
remove the try/finally block that manipulates os.killpg, and call
monkeypatch.delattr(os, "killpg", raising=False) before invoking
executor.stop(); keep the existing patches (patching subprocess.check_output,
platform.system, TCPExecutor.stop side_effect, and patch.object(executor,
"_windows_terminate_process")) so the test behavior is unchanged while cleanup
is automatic.
Member
|
@tboy1337 would you be able to push the editable package in separate PR? Or would you mind If I did it? |
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.
Refactors
PostgreSQLExecutorto improve Windows compatibility and security for PostgreSQL process management. This change addresses platform-specific issues with process termination and command formatting that could cause failures on Windows systems.Changes
_get_base_commandmethod toBASE_PROC_START_COMMANDclass attribute with cross-platform compatible syntax (removed single quotes around PostgreSQL config values that caused issues on Windows)_windows_terminate_processmethod that attempts graceful shutdown before force-killing stuck processesstop()method to use list arguments instead ofshell=Trueto prevent shell injection vulnerabilitiesos.killpgAttributeError on WindowsTest plan
tests/test_windows_compatibility.pywith 7 test caseskillpgAttributeErrorSummary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
Refactor