fix(ci): platform-aware pipx detection test (Windows matrix)#461
Draft
curdriceaurora wants to merge 1 commit into
Draft
fix(ci): platform-aware pipx detection test (Windows matrix)#461curdriceaurora wants to merge 1 commit into
curdriceaurora wants to merge 1 commit into
Conversation
`tests/cli/test_doctor.py::TestInstallMethodDetection::test_detect_pipx_via_pipx_home_env` hardcoded POSIX-style paths (`/custom/pipx`, `/custom/pipx/venvs/...`) while `_detect_install_method()` builds its comparison prefix with `os.path.join(pipx_home, "venvs") + os.sep`. On Windows this produces `/custom/pipx\venvs\` which never `startswith`-matches a forward-slash `exe_path`, so the test returns 'pip' instead of 'pipx'. Fix: build `custom_home` and `fake_exe` with `os.sep` + `os.path.join` so the same join logic is exercised on both POSIX and Windows. Verified cross-platform with the ntpath/posixpath modules (POSIX → 'pipx', Windows → 'pipx', old inputs on Windows → 'pip' reproduces the failure). Restores the green status of the "CI Full Matrix / Test Windows (Python 3.12)" job (run 26871404355).
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
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
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.
Summary
Restores green status of the CI Full Matrix / Test Windows (Python 3.12) job, which has been the sole failing matrix slot on recent scheduled runs (e.g. run 26871404355 on commit
dd13452).Failure detected
Single deterministic test failure, only on Windows + Python 3.12:
All four other matrix jobs (Linux py3.11, Linux py3.12, macOS py3.12, coverage gate) pass on the same commit.
Root cause
The test (lines 254–262 of
tests/cli/test_doctor.py) hardcoded POSIX-style paths:But the production code
_detect_install_method()insrc/cli/doctor.py:119builds its comparison prefix with the platform's join + separator:On Windows this evaluates to
"/custom/pipx\venvs\"(mixed separators becausepipx_homeis forward-slash butos.sep == "\\"). Thestartswithcheck against a pure forward-slashexe_pathalways fails, so the function returns"pip"and the assertion blows up.This is a test-side bug — the production code's behaviour on real Windows installs (where both
PIPX_HOMEandsys.executableuse\\) is correct. The other tests in the same class already use either platform-mockedos.path.expanduseroros.path.expanduser(...)directly, which is why they pass on Windows.Fix
Build the test inputs with
os.sep+os.path.joinso the same separator semantics are exercised on POSIX and Windows:custom_home="/custom/pipx", prefix"/custom/pipx/venvs/"→ matches ✓custom_home="\\custom\\pipx", prefix"\\custom\\pipx\\venvs\\"→ matches ✓Verification
Local cross-platform simulation using
posixpathandntpathmodules:os.sep-aware)pipx✓pipx✓/)pipx✓pip✗ (reproduces failure)Other local checks:
ruff check tests/cli/test_doctor.py— passesruff format --check— already formattedscripts/check_test_hardcoded_paths.pyfull-suite scan) — cleanThe Linux/macOS jobs of the same Matrix workflow already passed on the unchanged production code, so they will continue to pass under the new test inputs (POSIX path → same
startswithmatch).Trade-offs
PIPX_HOMEwould itself use\\). The fix is scoped to the test simulating realistic per-platform inputs.PIPX_HOME-based pipx executable is detected as"pipx"; only the input construction is now platform-aware.Impacted areas
tests/cli/test_doctor.py— single test function (test_detect_pipx_via_pipx_home_env), +6/-2.Links
Generated by Claude Code