Fix(env): Prioritize in-project venv over VIRTUAL_ENV #10689
Closed
+151
−6
Conversation
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
Reviewer's GuideAdjusts EnvManager environment selection to always prioritize an in-project .venv over any VIRTUAL_ENV setting and adds regression tests to lock in this precedence behavior, particularly for pipx-like scenarios and edge cases where VIRTUAL_ENV is set to '.' Updated class diagram for EnvManager prioritizing in-project venvclassDiagram
class EnvManager {
- poetry
- _env_cache
+ get(reload: bool) Env
+ in_project_venv_exists() bool
+ in_project_venv str
}
class Env {
<<interface>>
+ path str
}
class VirtualEnv {
+ VirtualEnv(path: str)
+ path str
}
EnvManager --> Env : returns
EnvManager ..> VirtualEnv : constructs
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/utils/env/test_env_manager_priority.py:36-45` </location>
<code_context>
+ def test_get_returns_in_project_venv_when_virtual_env_is_set(
</code_context>
<issue_to_address>
**suggestion (testing):** Add regression tests for cases where no in-project .venv exists to ensure VIRTUAL_ENV behavior is unchanged
Right now the tests only cover the new priority when `virtualenvs.in-project = True` and `.venv` exists. Please also add coverage for:
1. `virtualenvs.in-project = False` with `VIRTUAL_ENV` set → `EnvManager.get()` should continue to respect `VIRTUAL_ENV`.
2. `virtualenvs.in-project = True` with no `.venv` present and `VIRTUAL_ENV` set (including `"."`) → `EnvManager.get()` should fall back to the `VIRTUAL_ENV` environment.
These will confirm the early `in_project_venv_exists()` check only affects behavior when a real in-project venv is present.
Suggested implementation:
```python
def test_get_returns_in_project_venv_when_virtual_env_is_set(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
) -> None:
"""
When VIRTUAL_ENV is set (e.g., running via pipx) but an in-project
.venv exists, EnvManager.get() should return the in-project venv.
"""
from poetry.utils.env.env_manager import EnvManager
# Enable in-project virtualenvs and create an in-project .venv
config.set("virtualenvs.in-project", True)
project_venv = tmp_path / ".venv"
project_venv.mkdir()
env_manager = EnvManager(poetry, config, mocker.Mock())
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
# Even with VIRTUAL_ENV set, the in-project .venv should be preferred
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == project_venv
def test_get_respects_virtual_env_when_in_project_disabled(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
) -> None:
"""
When virtualenvs.in-project = False, EnvManager.get() should continue
to respect VIRTUAL_ENV and use that environment.
"""
from poetry.utils.env.env_manager import EnvManager
config.set("virtualenvs.in-project", False)
env_manager = EnvManager(poetry, config, mocker.Mock())
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == external_venv
def test_get_falls_back_to_virtual_env_when_in_project_enabled_but_no_venv(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""
When virtualenvs.in-project = True but no in-project .venv exists,
EnvManager.get() should fall back to the environment given by
VIRTUAL_ENV (including the special case VIRTUAL_ENV=".")
"""
from poetry.utils.env.env_manager import EnvManager
config.set("virtualenvs.in-project", True)
env_manager = EnvManager(poetry, config, mocker.Mock())
# No .venv created in the project directory
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
# 1. VIRTUAL_ENV points to an external venv path
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == external_venv
# 2. VIRTUAL_ENV is ".", which should resolve to the current working directory
monkeypatch.chdir(tmp_path)
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": "."},
clear=False,
):
env = env_manager.get()
assert env.path == tmp_path
```
1. Ensure `os` and `pytest` are imported at the top of `tests/utils/env/test_env_manager_priority.py` if they are not already:
- `import os`
- `import pytest`
2. If `EnvManager` is already imported at module level, you can remove the local `from poetry.utils.env.env_manager import EnvManager` imports inside the tests and use the existing import instead.
3. The constructor signature for `EnvManager` may differ in your codebase. Adjust `EnvManager(poetry, config, mocker.Mock())` to match the existing tests (for example, passing an `io` instance or using a shared `env_manager` fixture) to stay consistent with current patterns.
</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
+36
to
+45
| def test_get_returns_in_project_venv_when_virtual_env_is_set( | ||
| self, | ||
| poetry: Poetry, | ||
| config: Config, | ||
| mocker: MockerFixture, | ||
| tmp_path: Path, | ||
| mock_virtual_env: MagicMock, | ||
| ) -> None: | ||
| """ | ||
| When VIRTUAL_ENV is set (e.g., running via pipx) but an in-project |
There was a problem hiding this comment.
suggestion (testing): Add regression tests for cases where no in-project .venv exists to ensure VIRTUAL_ENV behavior is unchanged
Right now the tests only cover the new priority when virtualenvs.in-project = True and .venv exists. Please also add coverage for:
virtualenvs.in-project = FalsewithVIRTUAL_ENVset →EnvManager.get()should continue to respectVIRTUAL_ENV.virtualenvs.in-project = Truewith no.venvpresent andVIRTUAL_ENVset (including".") →EnvManager.get()should fall back to theVIRTUAL_ENVenvironment.
These will confirm the early in_project_venv_exists() check only affects behavior when a real in-project venv is present.
Suggested implementation:
def test_get_returns_in_project_venv_when_virtual_env_is_set(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
) -> None:
"""
When VIRTUAL_ENV is set (e.g., running via pipx) but an in-project
.venv exists, EnvManager.get() should return the in-project venv.
"""
from poetry.utils.env.env_manager import EnvManager
# Enable in-project virtualenvs and create an in-project .venv
config.set("virtualenvs.in-project", True)
project_venv = tmp_path / ".venv"
project_venv.mkdir()
env_manager = EnvManager(poetry, config, mocker.Mock())
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
# Even with VIRTUAL_ENV set, the in-project .venv should be preferred
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == project_venv
def test_get_respects_virtual_env_when_in_project_disabled(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
) -> None:
"""
When virtualenvs.in-project = False, EnvManager.get() should continue
to respect VIRTUAL_ENV and use that environment.
"""
from poetry.utils.env.env_manager import EnvManager
config.set("virtualenvs.in-project", False)
env_manager = EnvManager(poetry, config, mocker.Mock())
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == external_venv
def test_get_falls_back_to_virtual_env_when_in_project_enabled_but_no_venv(
self,
poetry: Poetry,
config: Config,
mocker: MockerFixture,
tmp_path: Path,
mock_virtual_env: MagicMock,
monkeypatch: pytest.MonkeyPatch,
) -> None:
"""
When virtualenvs.in-project = True but no in-project .venv exists,
EnvManager.get() should fall back to the environment given by
VIRTUAL_ENV (including the special case VIRTUAL_ENV=".")
"""
from poetry.utils.env.env_manager import EnvManager
config.set("virtualenvs.in-project", True)
env_manager = EnvManager(poetry, config, mocker.Mock())
# No .venv created in the project directory
external_venv = tmp_path / "external-venv"
external_venv.mkdir()
# 1. VIRTUAL_ENV points to an external venv path
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": str(external_venv)},
clear=False,
):
env = env_manager.get()
assert env.path == external_venv
# 2. VIRTUAL_ENV is ".", which should resolve to the current working directory
monkeypatch.chdir(tmp_path)
with mocker.patch.dict(
"os.environ",
{"VIRTUAL_ENV": "."},
clear=False,
):
env = env_manager.get()
assert env.path == tmp_path- Ensure
osandpytestare imported at the top oftests/utils/env/test_env_manager_priority.pyif they are not already:import osimport pytest
- If
EnvManageris already imported at module level, you can remove the localfrom poetry.utils.env.env_manager import EnvManagerimports inside the tests and use the existing import instead. - The constructor signature for
EnvManagermay differ in your codebase. AdjustEnvManager(poetry, config, mocker.Mock())to match the existing tests (for example, passing anioinstance or using a sharedenv_managerfixture) to stay consistent with current patterns.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where Poetry was not properly prioritizing in-project virtual environments (.venv) when virtualenvs.in-project = true, even when the VIRTUAL_ENV environment variable was set (e.g., when running Poetry via pipx).
Changes:
- Modified
EnvManager.get()to check for in-project venv existence before checking theVIRTUAL_ENVenvironment variable - Added comprehensive regression tests covering various scenarios with
VIRTUAL_ENVand in-project venv priority
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/poetry/utils/env/env_manager.py | Moved in-project venv check to execute before VIRTUAL_ENV environment variable check |
| tests/utils/env/test_env_manager_priority.py | Added new test suite with 4 regression tests for in-project venv prioritization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env = manager.get(reload=True) | ||
|
|
||
| assert env.path == venv_path | ||
| assert "." != str(env.path) |
There was a problem hiding this comment.
The assertion order is unconventional (literal on the left side). For consistency with other assertions in this file and standard Python practice, swap the operands to assert str(env.path) != '.'
Suggested change
| assert "." != str(env.path) | |
| assert str(env.path) != "." |
Author
|
Hi maintainers, I see that the CI check Failing test: My changes:
The failing test is in a completely different file and tests subprocess pipe handling, which is unrelated to the EnvManager environment selection logic I modified. I've fixed the pre-commit formatting issues (ruff and trailing whitespace), and my new regression tests all pass successfully. Could you please re-run the CI when you have a chance? I believe this is a flaky test failure. Thank you! |
Contributor
|
what makes you believe that this is related to #10610? I don't see any particular reason to prefer either the environment variable or the directory, in case of conflict - but changing it is going to break anyone who has been relying on the existing behaviour |
Member
|
I do not think this is a desired change.
This does not make sense to me. As far as I know, |
Author
|
Thank you for the feedback! After re-reading issue #10610 more carefully, I realize I misunderstood the root cause. You're absolutely right that I apologize for the confusion. I'll close this PR and take another look at the issue to understand the real problem better. Thanks for taking the time to review and explain this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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: #10610
When
virtualenvs.in-project = true, Poetry should prioritize the local.venveven ifVIRTUAL_ENVis set (e.g. when running viapipx). This PR modifiesEnvManagerto check for the in-project virtual environment before checking environment variables.Changes
EnvManager.get()to checkin_project_venv_exists()earlier.tests/utils/env/test_env_manager_priority.py.Verification
Added regression tests pass interactions with
VIRTUAL_ENVand.Existing tests pass.
Added tests for changed code.
Updated documentation for changed code.
Summary by Sourcery
Prioritize in-project virtual environments over externally provided VIRTUAL_ENV when resolving the active environment.
Bug Fixes:
Tests:
poetry env use.