Skip to content

Comments

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

Merged
AlexWaygood merged 4 commits intomainfrom
alex/rewrite-test-2
Apr 6, 2025
Merged

[red-knot] Fix python setting in mdtests, and rewrite a site-packages test as an mdtest#17222
AlexWaygood merged 4 commits intomainfrom
alex/rewrite-test-2

Conversation

@AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 5, 2025 •
edited
Loading

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
@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2025 •
edited
Loading

mypy_primer results

No ecosystem changes detected ✅

MichaReiser
MichaReiser approved these changes Apr 5, 2025
View reviewed changes
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

crates/red_knot_test/src/lib.rs
.map(|sys_prefix| {
PythonPath::SysPrefix(
sys_prefix.to_path_buf(),
SysPrefixPathOrigin::PythonCliFlag,
Copy link
Member

@MichaReiser MichaReiser Apr 5, 2025

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

AlexWaygood
AlexWaygood commented Apr 6, 2025
View reviewed changes
crates/red_knot_python_semantic/resources/mdtest/import/relative.md Outdated
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

@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.

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 •
edited
Loading

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
MichaReiser
MichaReiser approved these changes Apr 6, 2025
View reviewed changes
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 zanieb mentioned this pull request May 10, 2025
Merged
zanieb added a commit that referenced this pull request May 10, 2025
@zanieb
[ty] Remove vestigial pyvenv.cfg creation in mdtest (#18006)
7e8ba2b 
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
@zanieb @Glyphack
[ty] Remove vestigial pyvenv.cfg creation in mdtest (astral-sh#18006)
ac49809 
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

Reviewers

@MichaReiser MichaReiser MichaReiser approved these changes

@carljm carljm Awaiting requested review from carljm

@sharkdp sharkdp Awaiting requested review from sharkdp

@dcreager dcreager Awaiting requested review from dcreager

Assignees

No one assigned

Labels

testing Related to testing Ruff itself ty Multi-file analysis & type inference

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AlexWaygood @MichaReiser