Skip to content

[red-knot] Fix python setting in mdtests, and rewrite a site-packages test as an mdtest #17222

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

Merged
merged 4 commits into from
Apr 6, 2025

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 5, 2025

Summary

This PR does the following things:

  • Fixes the python configuration setting for mdtest (added in [red-knot] Allow setting python in mdtests #17221) so that it expects a path pointing to a venv's sys.prefix variable rather than the a path pointing to the venv's site-packages subdirectory. This brings the python setting in mdtest in sync with our CLI --python flag.
  • Tweaks mdtest so that it automatically creates a valid pyvenv.cfg file for you if you don't specify one. This makes it much more ergonomic to write an mdtest with a custom python setting: red-knot will reject a python setting that points to a directory that doesn't have a pyvenv.cfg file in it
  • Tweaks mdtest so that it doesn't check a custom pyvenv.cfg as Python source code if you do add a custom pyvenv.cfg file for your mock virtual environment in an mdtest. (You get a lot of diagnostics about Python syntax errors in the pyvenv.cfg file, otherwise!)
  • Rewrites the test added in Fix relative import resolution in site-packages packages when the site-packages search path is a subdirectory of the first-party search path #17178 as an mdtest, and deletes the original test that was added in that PR

Test Plan

I verified that the new mdtest fails if I revert the changes to resolver.rs that were added in #17178

@AlexWaygood AlexWaygood added testing Related to testing Ruff itself ty Multi-file analysis & type inference labels Apr 5, 2025
Copy link
Contributor

github-actions bot commented Apr 5, 2025

mypy_primer results

No ecosystem changes detected ✅

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. This was a bit more work than expected. Thank you

.map(|sys_prefix| {
PythonPath::SysPrefix(
sys_prefix.to_path_buf(),
SysPrefixPathOrigin::PythonCliFlag,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only noticed this now. The PythonCliFlag variant isn't very accurate because the source is either environment.python or --python. But that's unrelated to this PR

Comment on lines 261 to 271
`/src/.venv/lib/python3.13/site-packages/foo/__init__.py`:

```py
from .a import A as A
```

`/src/.venv/lib/python3.13/site-packages/foo/a.py`:

```py
class A: ...
```
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work on Windows, because on Windows the site-packages subdirectory is in a different path relative to sys.prefix (and red-knot knows this, which is what is causing the test to fail!). For the test to pass on Windows, this would need to be

Suggested change
`/src/.venv/lib/python3.13/site-packages/foo/__init__.py`:
```py
from .a import A as A
```
`/src/.venv/lib/python3.13/site-packages/foo/a.py`:
```py
class A: ...
```
`/src/.venv/Lib/site-packages/foo/__init__.py`:
```py
from .a import A as A
```
`/src/.venv/Lib/site-packages/foo/a.py`:
```py
class A: ...
```

(which is honestly more sane; I wish it were like this on non-Windows platforms too).

I'm wondering about adding a "magic path segment" like this, which mdtest would automatically replace with whatever the path to site-package is on the platform that the test is being run on, before it writes the file to the memory file system:

Suggested change
`/src/.venv/lib/python3.13/site-packages/foo/__init__.py`:
```py
from .a import A as A
```
`/src/.venv/lib/python3.13/site-packages/foo/a.py`:
```py
class A: ...
```
`/src/.venv/<path-to-site-packages>/foo/__init__.py`:
```py
from .a import A as A
```
`/src/.venv/<path-to-site-packages>/foo/a.py`:
```py
class A: ...
```

But this also feels like a certain amount of spiralling complexity. It might be best to say that tests which need to mock out site-packages should stay written in Rust rather than use mdtests?

Copy link
Member Author

@AlexWaygood AlexWaygood Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the "magic path segment" solution (and documented it) in 3397627. It's not too much added complexity, though I'm still not totally sure it's worth it. The alternative is just to close this PR, though (and maybe revert #17221), so I figured I'd push it to this PR so it can be reviewed. And it is nice to have as many tests as possible be mdtests.

@AlexWaygood AlexWaygood force-pushed the alex/rewrite-test-2 branch from a76cd7c to 4b0df37 Compare April 6, 2025 12:09
@AlexWaygood AlexWaygood marked this pull request as draft April 6, 2025 12:20
@AlexWaygood AlexWaygood force-pushed the alex/rewrite-test-2 branch from 8e53477 to 3397627 Compare April 6, 2025 12:27
@AlexWaygood AlexWaygood marked this pull request as ready for review April 6, 2025 12:33
@AlexWaygood AlexWaygood requested a review from MichaReiser April 6, 2025 12:33
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uff, this is annoying... but I remember that I ran into this before.

I do like your solution and, thanks for documenting it. An alternative is to have platform-specific tests (only run them on UNIX), but it's not that we want one or the other. It's more that we might want both and use magic paths if we want to test some generic venv behavior and platform-specific tests if we want to test platform-specific venv behavior.

@AlexWaygood AlexWaygood merged commit ac5d220 into main Apr 6, 2025
23 checks passed
@AlexWaygood AlexWaygood deleted the alex/rewrite-test-2 branch April 6, 2025 17:24
zanieb added a commit that referenced this pull request May 10, 2025
Following #17991, removes some of
#17222 which is no longer strictly
necessary. I don't actually think it's that ugly to have around? no
strong feelings on retaining it or not.
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request May 21, 2025
Following astral-sh#17991, removes some of
astral-sh#17222 which is no longer strictly
necessary. I don't actually think it's that ugly to have around? no
strong feelings on retaining it or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Related to testing Ruff itself ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants