fix(env): fail on incompatible Python without venv creation#10941
fix(env): fail on incompatible Python without venv creation#10941Yijian6 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.
| 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
radoering
left a comment
There was a problem hiding this comment.
This only improves the behavior for virtualenvs.use-poetry-python=true. If it is set to false, an incompatible version will still be used. Either this is the wrong spot for the fix or an additional change is required.
2aa1503 to
257c576
Compare
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