fix(env): fail on incompatible Python without venv creation#10941
Open
Yijian6 wants to merge 1 commit into
Open
fix(env): fail on incompatible Python without venv creation#10941Yijian6 wants to merge 1 commit into
Yijian6 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_create_venv_fails_if_current_python_is_not_supported_without_creating_venv, directly mutatingos.environ(deletingVIRTUAL_ENV) without restoring it can leak state across tests; consider usingmonkeypatch.delenvor similar to ensure the environment is reset. - The new error path when
virtualenvs.createis false still reuses the genericInvalidCurrentPythonVersionErrormessage; consider tailoring the message or adding a hint that venv creation is disabled so users understand why switching interpreters viaenv useis required.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_create_venv_fails_if_current_python_is_not_supported_without_creating_venv`, directly mutating `os.environ` (deleting `VIRTUAL_ENV`) without restoring it can leak state across tests; consider using `monkeypatch.delenv` or similar to ensure the environment is reset.
- The new error path when `virtualenvs.create` is false still reuses the generic `InvalidCurrentPythonVersionError` message; consider tailoring the message or adding a hint that venv creation is disabled so users understand why switching interpreters via `env use` is required.
## Individual Comments
### Comment 1
<location path="tests/utils/env/test_env_manager.py" line_range="1080-1081" />
<code_context>
+ mocked_python_register: MockedPythonRegister,
+) -> None:
+ config.config["virtualenvs"]["create"] = False
+ if "VIRTUAL_ENV" in os.environ:
+ del os.environ["VIRTUAL_ENV"]
+
+ poetry.package.python_versions = "^3.10"
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid mutating os.environ directly in tests to prevent cross-test interference
Directly deleting from `os.environ` can leak state into other tests if this test errors before cleanup. Use the `monkeypatch` fixture instead (e.g., `monkeypatch.delenv("VIRTUAL_ENV", raising=False)` or `monkeypatch.setenv`/`delenv`) so environment changes are automatically isolated and reverted.
Suggested implementation:
```python
def test_create_venv_fails_if_current_python_is_not_supported_without_creating_venv(
manager: EnvManager,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
mocked_python_register: MockedPythonRegister,
monkeypatch: "MonkeyPatch",
) -> None:
config.config["virtualenvs"]["create"] = False
monkeypatch.delenv("VIRTUAL_ENV", raising=False)
```
1. Ensure the `MonkeyPatch` type is imported at the top of the file, for example:
`from _pytest.monkeypatch import MonkeyPatch`.
2. If your project uses postponed evaluation of annotations (`from __future__ import annotations`) you can keep the type as a string; otherwise, you can drop the quotes and annotate as `monkeypatch: MonkeyPatch`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1080
to
+1081
| if "VIRTUAL_ENV" in os.environ: | ||
| del os.environ["VIRTUAL_ENV"] |
There was a problem hiding this comment.
suggestion (testing): Avoid mutating os.environ directly in tests to prevent cross-test interference
Directly deleting from os.environ can leak state into other tests if this test errors before cleanup. Use the monkeypatch fixture instead (e.g., monkeypatch.delenv("VIRTUAL_ENV", raising=False) or monkeypatch.setenv/delenv) so environment changes are automatically isolated and reverted.
Suggested implementation:
def test_create_venv_fails_if_current_python_is_not_supported_without_creating_venv(
manager: EnvManager,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
mocked_python_register: MockedPythonRegister,
monkeypatch: "MonkeyPatch",
) -> None:
config.config["virtualenvs"]["create"] = False
monkeypatch.delenv("VIRTUAL_ENV", raising=False)- Ensure the
MonkeyPatchtype is imported at the top of the file, for example:
from _pytest.monkeypatch import MonkeyPatch. - If your project uses postponed evaluation of annotations (
from __future__ import annotations) you can keep the type as a string; otherwise, you can drop the quotes and annotate asmonkeypatch: MonkeyPatch.
5516faf to
2aa1503
Compare
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.
Pull Request Check List
Resolves: #10226
When virtualenv creation is disabled, Poetry currently still tries to select a
compatible Python after detecting that the preferred/current Python does not
satisfy the project's constraint. Because no venv will be created, the command
then falls back to the system environment and can continue with the incompatible
Python despite logging that a compatible interpreter was selected.
This change turns that incompatibility into
InvalidCurrentPythonVersionErrorbefore compatible-interpreter discovery when
virtualenvs.createis false. Theexisting fallback behavior is preserved when Poetry can create a venv, and
explicit Python requests continue to raise
NoCompatiblePythonVersionFoundError.No breaking changes.
Validation:
.\.venv\Scripts\python.exe -m pytest tests\utils\env\test_env_manager.py -q.\.venv\Scripts\python.exe -m ruff check src\poetry\utils\env\env_manager.py tests\utils\env\test_env_manager.py.\.venv\Scripts\python.exe -m mypy src\poetry\utils\env\env_manager.py tests\utils\env\test_env_manager.py