-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[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
Conversation
…ges` test as an mdtest
|
There was a problem hiding this 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, |
There was a problem hiding this comment.
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
`/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: ... | ||
``` |
There was a problem hiding this comment.
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
`/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:
`/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?
There was a problem hiding this comment.
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.
a76cd7c
to
4b0df37
Compare
8e53477
to
3397627
Compare
There was a problem hiding this 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.
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.
Summary
This PR does the following things:
python
configuration setting for mdtest (added in [red-knot] Allow settingpython
in mdtests #17221) so that it expects a path pointing to a venv'ssys.prefix
variable rather than the a path pointing to the venv'ssite-packages
subdirectory. This brings thepython
setting in mdtest in sync with our CLI--python
flag.pyvenv.cfg
file for you if you don't specify one. This makes it much more ergonomic to write an mdtest with a custompython
setting: red-knot will reject apython
setting that points to a directory that doesn't have apyvenv.cfg
file in itpyvenv.cfg
as Python source code if you do add a custompyvenv.cfg
file for your mock virtual environment in an mdtest. (You get a lot of diagnostics about Python syntax errors in thepyvenv.cfg
file, otherwise!)site-packages
packages when thesite-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 PRTest Plan
I verified that the new mdtest fails if I revert the changes to
resolver.rs
that were added in #17178