Cleaner error message when the server is not installed - closes #1031 - #1396
Cleaner error message when the server is not installed - closes #1031#1396fizyk wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe change centralises PostgreSQL executable discovery. It validates configured and discovered paths, reports ChangesPostgreSQL executable discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant _pg_exe
participant _pg_bindir
participant pg_config
participant FileSystem
participant ExecutableMissingException
_pg_exe->>_pg_bindir: Query pg_config --bindir
_pg_bindir->>pg_config: Run pg_config --bindir
pg_config-->>_pg_bindir: Return trimmed binary directory
_pg_exe->>FileSystem: Check platform-specific pg_ctl names
FileSystem-->>_pg_exe: Return executable or missing result
_pg_exe->>ExecutableMissingException: Report checked locations on failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/process.py`:
- Around line 64-86: Update _pg_ctl_missing_message so the bindir=None branch
only states that pg_config could not be used to locate PostgreSQL binaries,
without claiming that no PostgreSQL installation exists. Preserve the existing
message for a successfully reported bindir and the remainder of the remediation
guidance.
In `@tests/test_pg_exe.py`:
- Around line 113-120: Update the error parameter values in the
pytest.mark.parametrize declaration to use a list instead of a tuple, preserving
the existing FileNotFoundError, PermissionError, and CalledProcessError cases so
the module satisfies Ruff PT007.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc493594-a1e4-4b48-803b-fde21c02ade6
📒 Files selected for processing (7)
README.rstnewsfragments/1031.bugfix.rstnewsfragments/1031.docs.rstpytest_postgresql/executor.pypytest_postgresql/factories/process.pytests/test_executor.pytests/test_pg_exe.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/process.py`:
- Around line 96-107: Update the pg_ctl path checks in the surrounding
executable-discovery function to use os.path.isfile() instead of
os.path.exists() for both the configured postgresql_ctl value and each
discovered candidate, ensuring directories are rejected and invalid paths reach
ExecutableMissingException.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b7b1fe1-d3a7-4899-9e2e-99622e9149e9
📒 Files selected for processing (1)
pytest_postgresql/factories/process.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/process.py`:
- Line 56: Update _pg_bindir to pass a finite timeout to subprocess.check_output
when invoking pg_config, preserving the existing SubprocessError handling so
TimeoutExpired is handled consistently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e239636-9fd3-475b-8c72-4b15a5367ad0
📒 Files selected for processing (1)
pytest_postgresql/factories/process.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/process.py`:
- Around line 108-119: Update _pg_exe to require execute permission when
accepting configured and _pg_bindir-discovered pg_ctl candidates, using an
appropriate executable-file check; leave explicit factory arguments unvalidated.
Preserve the existing candidate search and ExecutableMissingException behavior,
and add regression coverage for non-executable configured and discovered
candidates.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76003069-c14a-47c6-adfd-302d4658d6cc
📒 Files selected for processing (2)
pytest_postgresql/factories/process.pytests/test_pg_exe.py
46aa9ce to
33523ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytest_postgresql/factories/_pg.py`:
- Line 7: Update the import used by the annotations in the module to import
Iterable from collections.abc instead of typing, while preserving the existing
annotation contract and usage.
In `@tests/test_executor.py`:
- Line 520: Update all six patch decorators in the affected tests to target the
local binding used by postgresql_proc, replacing
pytest_postgresql.factories._pg._pg_exe with
pytest_postgresql.factories.process._pg_exe so raw_func invokes the mocked
executor.
In `@tests/test_pg_exe.py`:
- Around line 96-97: Combine the nested patch and pytest.raises context managers
in each affected test into a single with statement. Update the tests around the
visible subprocess.check_output patches and ExecutableMissingException
assertions, including all four reported locations, while preserving the existing
patch arguments, exception matching, and test behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8645f445-b50b-4ea6-8fee-14d3767f2b65
📒 Files selected for processing (4)
pytest_postgresql/factories/_pg.pypytest_postgresql/factories/process.pytests/test_executor.pytests/test_pg_exe.py
|
|
||
| with ( | ||
| patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"), | ||
| patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Patch the binding used by postgresql_proc.
pytest_postgresql.factories.process imports _pg_exe into its own module namespace, and postgresql_proc calls that local binding. (raw.githubusercontent.com)
Patching pytest_postgresql.factories._pg._pg_exe at Line [520], Line [557], Line [607], Line [660], Line [698], and Line [744] does not replace the function that raw_func(...) invokes. These tests can run real discovery with config_mock and fail before the mocked executor is reached.
Change each patch target to pytest_postgresql.factories.process._pg_exe.
Proposed fix
- patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"),
+ patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"),Also applies to: 557-557, 607-607, 660-660, 698-698, 744-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_executor.py` at line 520, Update all six patch decorators in the
affected tests to target the local binding used by postgresql_proc, replacing
pytest_postgresql.factories._pg._pg_exe with
pytest_postgresql.factories.process._pg_exe so raw_func invokes the mocked
executor.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_executor.py`:
- Line 557: Update the patch targets at the five executor test sites surrounding
the relevant mocked process cases to patch the binding used by postgresql_proc:
replace pytest_postgresql.factories._pg._pg_exe with
pytest_postgresql.factories.process._pg_exe, preserving the existing return
value and test behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fbb3ba2b-953b-42c3-8a4c-9c6f475ec8f3
📒 Files selected for processing (1)
tests/test_executor.py
|
|
||
| with ( | ||
| patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"), | ||
| patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
Patch the binding used by postgresql_proc.
pytest_postgresql.factories.process calls its imported local _pg_exe binding. Patching pytest_postgresql.factories._pg._pg_exe does not replace that binding. These tests can execute real executable discovery before the mocked process behaviour is reached.
Change the patch target at Line 557, Line 607, Line 660, Line 698, and Line 744 to pytest_postgresql.factories.process._pg_exe.
Proposed fix
- patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"),
+ patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"),Also applies to: 607-607, 660-660, 698-698, 744-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_executor.py` at line 557, Update the patch targets at the five
executor test sites surrounding the relevant mocked process cases to patch the
binding used by postgresql_proc: replace pytest_postgresql.factories._pg._pg_exe
with pytest_postgresql.factories.process._pg_exe, preserving the existing return
value and test behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_executor.py`:
- Line 607: Update the patch target in the tests using postgresql_proc so they
patch the local binding consumed by pytest_postgresql.factories.process: change
each pytest_postgresql.factories._pg._pg_exe target to
pytest_postgresql.factories.process._pg_exe at all four occurrences. Preserve
the existing mocked return value and test behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ad4bac4d-f98d-47ca-a36c-462fef018953
📒 Files selected for processing (1)
tests/test_executor.py
|
|
||
| with ( | ||
| patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"), | ||
| patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"), |
There was a problem hiding this comment.
Patch the binding used by postgresql_proc.
pytest_postgresql.factories.process imports _pg_exe into its own module namespace, and postgresql_proc calls that local binding. Patching pytest_postgresql.factories._pg._pg_exe at Lines [607], [660], [698], and [744] does not replace the function used by raw_func(...). These tests can perform real executable discovery with config_mock and fail before the mocked executor or port behaviour is reached. Change each target to pytest_postgresql.factories.process._pg_exe.
Proposed fix
- patch("pytest_postgresql.factories._pg._pg_exe", return_value="/usr/bin/pg_ctl"),
+ patch("pytest_postgresql.factories.process._pg_exe", return_value="/usr/bin/pg_ctl"),Also applies to: 660-660, 698-698, 744-744
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_executor.py` at line 607, Update the patch target in the tests
using postgresql_proc so they patch the local binding consumed by
pytest_postgresql.factories.process: change each
pytest_postgresql.factories._pg._pg_exe target to
pytest_postgresql.factories.process._pg_exe at all four occurrences. Preserve
the existing mocked return value and test behavior.
1 participant
Summary by CodeRabbit
Bug Fixes
pg_ctlpaths before use.pg_configcommands consistently, including permission and subprocess errors.Documentation
postgresql_procrequires local server binaries; externally managed or containerised servers can usepostgresql_noproc.