Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182
Refactor PostgreSQLExecutor to support Windows compatibility for process management#1182tboy1337 wants to merge 29 commits intodbfixtures:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 POSTGRESQL_EXEC; wildcard test import removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as PostgreSQLExecutor
participant OS as OperatingSystem
participant Proc as PostgreSQLProcess
Client->>Executor: stop()
Executor->>Executor: determine platform & build stop args
Executor->>OS: spawn subprocess (pg_ctl stop) with list 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->>Executor: call super().stop()
alt super().stop() raises (e.g. os.killpg unavailable)
Executor->>Executor: fallback -> _windows_terminate_process()
end
end
Executor-->>Client: stop complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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)
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}
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
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
tboy1337
force-pushed
the
edit
branch
2 times, most recently
from
059e4bc to
54140ac
Compare
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
…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
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.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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.
… 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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pytest_postgresql/executor.py (2)
281-300: Theexcept AttributeErrorfallback is fragile in a subtle edge case.If we're on the Windows branch (line 288) and
_windows_terminate_processsomehow allows anAttributeErrorto escape (despite its internal catch), the fallback at line 296-297 calls it again — risking the same failure and an unhandled exception. In practice, the internalexcept (OSError, AttributeError)in_windows_terminate_processshould prevent this, but the structure is still a bit brittle.Consider narrowing the scope by only applying this
except AttributeErrorto the non-Windows (super().stop()) branch:Suggested restructure
try: if platform.system() == "Windows": self._windows_terminate_process(sig) else: - super().stop(sig, exp_sig) + try: + super().stop(sig, exp_sig) + except AttributeError: + if not hasattr(os, "killpg"): + self._windows_terminate_process(sig) + else: + raise except ProcessFinishedWithError: # Finished, leftovers ought to be cleaned afterwards anyway pass - except AttributeError: - # Fallback for edge cases where os.killpg doesn't exist (e.g., Windows) - if not hasattr(os, "killpg"): - self._windows_terminate_process(sig) - else: - raiseThis way, the
AttributeErrorhandler only wrapssuper().stop()— the call that can genuinely fail from a missingos.killpg— and doesn't inadvertently swallow or double-invoke on the Windows path.
283-285: Ruff S603 is a false positive here — list-stylesubprocesscall with no shell expansion.The
S603hint flags "check for execution of untrusted input", but withshell=False(the default) and list arguments, there is no shell-injection vector. The executable path originates from fixture configuration (pg_ctl). This is safe to suppress with an inline# noqa: S603if it's noisy in CI.
…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.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
- 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 112-114: The artifact paths currently list
"/tmp/pytest-of-runner/**" and "${{ runner.temp }}/pytest-of-runner/**" which
miss Windows pytest temp locations; update the workflow to either (a) add a
Windows-style glob like
"C:/Users/runner*/AppData/Local/Temp/pytest-of-runner/**" (or a more generic
"%TEMP%/pytest-of-runner/**" equivalent) to the upload-artifact paths, or (b)
set an env var in the job (e.g. PYTEST_DEBUG_TEMPROOT or
tmp_path_retention_policy) to point pytest to the runner.temp directory so the
existing "${{ runner.temp }}/pytest-of-runner/**" will always capture
artifacts—modify the upload-artifact path list and/or the job env block
accordingly so Windows runners produce matching artifact paths.
🧹 Nitpick comments (1)
.github/workflows/single-postgres.yml (1)
97-99:containscheck matches all matrix entries, not just the PyPy job.
contains(inputs.python-versions, 'pypy')tests whether the entire JSON string contains the substring'pypy', so thesudo apt install libpq5step runs for every Python version in the matrix whenever pypy is listed — not only for the pypy matrix entry. This is a pre-existing pattern, but now would be a good time to tighten it:- if: ${{ contains(inputs.python-versions, 'pypy') && runner.os == 'Linux' }} + if: ${{ startsWith(matrix.python-version, 'pypy') && runner.os == 'Linux' }}This limits the extra
apt installto only the job that actually needs it.
…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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/single-postgres.yml:
- Around line 103-112: The artifact upload glob won't match when pytest is run
with --basetemp, so update the upload step that references the pytest temp dir
(the actions/upload-artifact@v6 step's path) to use a glob rooted at the runner
temp directory rather than "pytest-of-*/**"; for example replace the current
path "${{ runner.temp }}/pytest-of-*/**" with a recursive pattern like "${{
runner.temp }}/**" (or a more specific recursive pattern if you only want
certain files) so artifacts created under the explicit --basetemp location are
actually collected on failure.
|
@tboy1337 there's a conflic against dockerised-postrgresql.yml workflow. |
…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>
Should be fixed now. |
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>
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.
|
@coderabbitai resume |
✅ 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.
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.
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.
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.
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.
There was a problem hiding this comment.
Those tests should only fire for windows, right? Use https://docs.pytest.org/en/stable/how-to/skipping.html#id1 to skip... maybe whole module if tests are not being run on windows
There was a problem hiding this comment.
These tests are intentionally cross-platform. Every test in the module mocks platform.system() (via patch("pytest_postgresql.executor.platform.system", return_value="Windows")) and uses MagicMock for any subprocess/process objects, so there is no real dependency on the host OS.
The file name means "tests that verify Windows compatibility code paths" — not "tests that should only run on Windows." Running them on Linux CI is the whole point: it confirms that the Windows-specific branches (WINDOWS_PROC_START_COMMAND, _windows_terminate_process, the stop() override) produce correct behaviour without needing a real Windows runner.
Adding a module-level skipif(platform.system() != "Windows") would silently drop all of these tests from the Linux CI matrix and eliminate coverage of those code paths entirely, which would be counterproductive.
2 participants
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