-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Use installer instead of setuptools in test suite #11291
Conversation
"platlib": str(lib_install_dir), | ||
"scripts": str(bin_install_dir), | ||
}, | ||
interpreter=sys.executable, |
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 would mean that the executables/scripts from these installs will run with the same environment as the test runner (i.e. pytest), which is likely not what we want.
This should really be using the Python executable from the virtualenv. I think the right thing to do here would be to change up the virtualenv
fixture, with the object coming from that getting a new .install_from_wheel(path)
method.
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 think these scripts are never used. I'll remove the bin
dir and see if the tests pass.
Then we can do a refactoring in followup, after #11288.
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.
Yea, let's do that!
I might end up picking up the refactoring, as part of https://github.com/pradyunsg/pip/tree/improve-test-isolation. :)
338b502
to
8520de7
Compare
Alright, tests pass, at least on linux. I pushed some minor tweaks. |
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.
LGTM, assuming CI is happy and a typo is fixed. I'll green tick, trusting that @sbidoul will fix the typo before merging. :)
Instead of using a private setuptools api to install common wheels in "editable" mode, use 'installer' together with a .pth.
8520de7
to
3f5436c
Compare
@@ -109,7 +109,7 @@ def test_local_columns_flag(simple_script: PipTestEnvironment) -> None: | |||
assert "Package" in result.stdout | |||
assert "Version" in result.stdout | |||
assert "simple (1.0)" not in result.stdout | |||
assert "simple 1.0" in result.stdout, str(result) | |||
assert "simple 1.0" in result.stdout, str(result) |
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 test changes because pip list --local
does not show coverage
and setuptools
while it did before.
Therefore the column is smaller.
I think this is correct because coverage and setuptools are outside of the virtualenv, according to our implementation of BaseDistribution.local
.
In the test suite, instead of using a private setuptools api to install common wheels in "editable" mode, use
installer
together with a.pth
.closes #11256