-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix detection of python and pip executables on Windows and simplify tests #10645
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix detection of python and pip executables on Windows and simplify tests #10645
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts GenericEnv executable discovery to correctly handle .exe-suffixed python and pip binaries on Windows and simplifies the tests to explicitly verify default and fallback resolution behavior across platforms. Class diagram for updated GenericEnv executable discoveryclassDiagram
class GenericEnv {
_child_env
find_executables() void
}
class Platform {
WINDOWS : bool
}
GenericEnv --> Platform : checks WINDOWS
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
test_env_finds_fallback_executables_for_generic_env,expected_executable/expected_pip_executableare now reassigned tomajor_*/default_*outside theifblocks, which means they can be updated even when no unlinking occurred and the corresponding files don’t exist, potentially desynchronizing the expectation from the actual state of the venv bin directory; consider keeping the reassignment inside the condition or otherwise only updating expectations when the unlink actually happens. - The Windows
.exehandling inGenericEnv.find_executablescurrently replaces all patterns with.exevariants; if mixed or non-suffixed executables are possible in some environments, it might be safer to extend or branch the patterns instead of overwriting them entirely. - The Windows
.exemapping comprehension[(f"{p[0]}.exe", f"{p[1]}.exe") for p in patterns]would be more readable with tuple unpacking, e.g.for py, pip in patterns, to make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_env_finds_fallback_executables_for_generic_env`, `expected_executable`/`expected_pip_executable` are now reassigned to `major_*`/`default_*` outside the `if` blocks, which means they can be updated even when no unlinking occurred and the corresponding files don’t exist, potentially desynchronizing the expectation from the actual state of the venv bin directory; consider keeping the reassignment inside the condition or otherwise only updating expectations when the unlink actually happens.
- The Windows `.exe` handling in `GenericEnv.find_executables` currently replaces all patterns with `.exe` variants; if mixed or non-suffixed executables are possible in some environments, it might be safer to extend or branch the patterns instead of overwriting them entirely.
- The Windows `.exe` mapping comprehension `[(f"{p[0]}.exe", f"{p[1]}.exe") for p in patterns]` would be more readable with tuple unpacking, e.g. `for py, pip in patterns`, to make the intent clearer.
## Individual Comments
### Comment 1
<location> `tests/utils/env/test_env.py:461-468` </location>
<code_context>
and venv._bin_dir.joinpath(major_executable).exists()
):
venv._bin_dir.joinpath(expected_executable).unlink()
- expected_executable = major_executable
+ expected_executable = major_executable
if (
venv._bin_dir.joinpath(expected_executable).exists()
and venv._bin_dir.joinpath(default_executable).exists()
):
venv._bin_dir.joinpath(expected_executable).unlink()
- expected_executable = default_executable
+ expected_executable = default_executable
</code_context>
<issue_to_address>
**suggestion (testing):** The fallback test no longer guards `expected_executable`/`expected_pip_executable` against missing files, which can make the test misleading.
With the assignments dedented and the final existence checks removed, `expected_executable`/`expected_pip_executable` are now always set to the major or default names, regardless of whether those files ever existed or were unlinked. As a result, the assertions may no longer exercise the real fallback path.
Consider either:
- Keeping the assignments inside the `if` blocks and restoring the final `if not exists` fallbacks, or
- Explicitly creating/removing executables in the temp venv so the chosen `expected_*` values match the actual fallback path under test.
That way the test validates the intended fallback order instead of relying on incidental filesystem state.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9d52048 to
f48e6da
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In
test_env_finds_the_correct_executables_for_generic_env,expected_pip_executablestill uses the versioned name on Windows even though the Windows-specific override topip.exewas removed, which likely makes the test inconsistent with the intended behavior described in the PR summary (and withpython.exehandling). - When building
patternsfor Windows inGenericEnv.find_executables, consider unpacking the tuples (e.g.,for py, pip in patterns) instead of indexing withp[0]/p[1]to improve readability and reduce the chance of index mistakes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_env_finds_the_correct_executables_for_generic_env`, `expected_pip_executable` still uses the versioned name on Windows even though the Windows-specific override to `pip.exe` was removed, which likely makes the test inconsistent with the intended behavior described in the PR summary (and with `python.exe` handling).
- When building `patterns` for Windows in `GenericEnv.find_executables`, consider unpacking the tuples (e.g., `for py, pip in patterns`) instead of indexing with `p[0]`/`p[1]` to improve readability and reduce the chance of index mistakes.
## Individual Comments
### Comment 1
<location> `tests/utils/env/test_env.py:459-462` </location>
<code_context>
-
- if not venv._bin_dir.joinpath(expected_pip_executable).exists():
- expected_pip_executable = default_pip_executable
+ venv._bin_dir.joinpath(minor_executable).unlink(missing_ok=True)
+ venv._bin_dir.joinpath(major_executable).unlink(missing_ok=True)
+ venv._bin_dir.joinpath(minor_pip_executable).unlink(missing_ok=True)
+ venv._bin_dir.joinpath(major_pip_executable).unlink(missing_ok=True)
venv = GenericEnv(parent_venv.path, child_env=VirtualEnv(child_venv_path))
</code_context>
<issue_to_address>
**suggestion (testing):** The fallback test now only exercises the final fallback to the default executable and no longer covers the intermediate `major`-only cases
The previous `test_env_finds_fallback_executables_for_generic_env` validated the full chain (minor → major → default) by varying which executables existed. The new version removes both minor and major variants up front, so it only checks the final default fallback and no longer exercises the "major-only" scenario.
To retain coverage while keeping things simple, consider splitting into focused or parametrized tests, e.g.:
- minor present → expect minor
- minor removed, major present → expect major
- minor & major removed → expect default (current behavior)
That way the resolver’s `major` fallback remains tested across platforms without reintroducing the more complex setup logic.
Suggested implementation:
```python
# In this scenario we explicitly remove both the minor and major shims, so we
# expect the resolver to fall back to the default executable names that were
# created at setup time.
expected_executable = default_executable
expected_pip_executable = default_pip_executable
```
To fully implement the test coverage suggested in your review comment, you will also need to:
1. Re-introduce or add two additional tests (or a parametrized test) alongside the existing fallback test, for example:
- **minor present → expect minor**: create the minor `python` and `pip` shims, do *not* remove them, and assert that `GenericEnv` resolves to `minor_executable` / `minor_pip_executable`.
- **minor removed, major present → expect major**: remove only the minor shims (e.g. `venv._bin_dir.joinpath(minor_executable).unlink(missing_ok=True)` and the pip variant), keep the major ones, and assert that `GenericEnv` resolves to `major_executable` / `major_pip_executable`.
2. Keep the current test (where both minor and major shims are removed) as the **“minor & major removed → expect default”** case, using the `expected_executable` / `expected_pip_executable` assignments updated above.
3. If you choose to use `pytest.mark.parametrize` instead of three separate tests, structure it roughly as:
```python
@pytest.mark.parametrize(
"remove_minor, remove_major, expected_executable, expected_pip_executable",
[
(False, False, minor_executable, minor_pip_executable),
(True, False, major_executable, major_pip_executable),
(True, True, default_executable, default_pip_executable),
],
)
def test_env_finds_fallback_executables_for_generic_env(...):
...
```
and adjust the unlink calls (`remove_minor`, `remove_major`) accordingly.
4. Remove or refactor any old single “chain” test that attempted to exercise minor→major→default in one body so that each scenario is clearly and independently asserted as above.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
f48e6da to
6dc7c19
Compare
The glob did not find anything on Windows in case of
self._child_envbeing set because the executables end with.exe. The fallback test did not test the fallback on Windows because specific pip executables were not deleted because there is nopython{major}.{minor}.exeon Windows.The bug is not very relevant because by default executables without version information in their names are used. It was just uncovered due to failing tests when trying to update virtualenv to 20.35 (because this version starts creating
python3.exeon Windows).Summary by Sourcery
Fix detection of Python and pip executables in generic virtual environments, including proper handling of Windows-specific .exe names and corresponding test coverage.
Bug Fixes:
Tests:
Summary by Sourcery
Fix detection and fallback selection of python and pip executables in generic virtual environments, particularly on Windows.
Bug Fixes:
Tests: