Skip to content
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

Test Suite: Tests are not using the latest setuptools #5732

Closed
pfmoore opened this issue Aug 24, 2018 · 14 comments
Closed

Test Suite: Tests are not using the latest setuptools #5732

pfmoore opened this issue Aug 24, 2018 · 14 comments
Labels
C: tests Testing and related things type: enhancement Improvements to functionality

Comments

@pfmoore
Copy link
Member

pfmoore commented Aug 24, 2018

I'm struggling with running the tests while implementing #5407 (https://travis-ci.org/pfmoore/pip/jobs/420017719). The failure "error: 'egg_base' must be a directory name" is a bug in setuptools, that is fixed in the latest version, 40.2.0. But after a lot of debugging, I've found that the build environment used by test_pip_wheel_with_pep518_build_reqs_no_isolation has setuptools 39.1.0 installed. I've no idea why - there's no mention of that version in the pip sources, and as far as I can see that version isn't installed on my PC.

So I'm somewhat baffled as to where the test suite is getting that version from. If anyone has any suggestions, I'd really appreciate it. Ultimately, this is a bug in the test suite as it doesn't seem to be using the latest version of setuptools, but in the short term my main concern is that I can't progress on the PEP 517 implementation until I get this sorted.

@pypa/pip-committers any suggestions? Or anyone else?

@dstufft
Copy link
Member

dstufft commented Aug 24, 2018

Is it the version that's bundled with virtualenv?

@dstufft
Copy link
Member

dstufft commented Aug 24, 2018

@pradyunsg
Copy link
Member

IIRC, The common_wheels fixture downloads the latest versions of setuptools and wheel. I think you should be able to "pip install --find-links" from that for the specific tests.

Perhaps even make this into decorator?

@pfmoore
Copy link
Member Author

pfmoore commented Aug 24, 2018

Oh good catch. So I guess:

  1. The test suite should make sure it upgrades setuptools in a virtualenv.
  2. We need to get a better version of setuptools bundled with virtualenv.

Alternatively, I wonder whether the setuptools PEP 517 wrapper should be split out into a separate project. I'm starting to feel that locking PEP 517 behaviour behind "you need a very recent version of setuptools" might be a big ask. Particularly as the (current) plan is to make "having pyproject.toml enough to trigger PEP517. In real life, though, build isolation means people will transparently get the latest setuptools, so it probably won't be an issue in practice.

Longer term, the way the test suite manages environments is really clumsy. It's basically impossible to depend on a specific version of setuptools - and even more so if you want an unreleased version. And debugging is even worse, as the environments are aggressively dropped when the tests complete, so you can't do anything postmortem. But I don't know if I have the energy to do anything about that, so it's really just me venting 😞

@pfmoore
Copy link
Member Author

pfmoore commented Aug 24, 2018

I think it might actually just be a bug in test_pip_wheel_with_pep518_build_reqs_no_isolation. It doesn't install setuptools at all, which is just wrong - even in the current pip. It's just that some weirdness with the test environments means that some version of setuptools ends up leaking into the supposedly-clean environment... Maybe. My head hurts.

I've just pushed a commit that fixes that, we'll see how it works. (Can't run the tests locally at the moment, my PC is dying 😢)

@pfmoore
Copy link
Member Author

pfmoore commented Aug 24, 2018

Looks like that worked. So it turned out to be a shallow bug in the test, not a bug in the test infrastructure. Phew. Thanks for the help.

@pradyunsg
Copy link
Member

It's just that some weirdness with the test environments means that some version of setuptools ends up leaking into the supposedly-clean environment... Maybe. My head hurts.

The virtualenvs for tests get the version of setuptools bundled in virtualenv installed in them. FWIW, in the pre-PEP 517/518 world, it didn't really matter which version it was as long as it wasn't very broken (which was mostly true for the bundled version of setuptools). Until PEP 517/518, there was basically no compelling reason for pip to be testing against the latest setuptools (or in development).

You're working on something that's showing all of these rough edges here. :)

@pfmoore
Copy link
Member Author

pfmoore commented Aug 24, 2018

Yeah. Because PEP 517 requires a recent version of setuptools, not just "any old version", it needs to be made explicit. I wouldn't have been so confused if the test hadn't simply assumed setuptools was present - after all it explicitly installs wheel, and (I thought) virtualenv does that by default (but the internal function we use to build the virtualenv fixture doesn't install wheel by default, and I didn't know that).

You're working on something that's showing all of these rough edges here. :)

Oh, boy do I know it :-) It's fun and educational (most of the time) though!

@pradyunsg
Copy link
Member

pradyunsg commented Aug 27, 2018

Longer term, the way the test suite manages environments is really clumsy. It's basically impossible to depend on a specific version of setuptools - and even more so if you want an unreleased version.

I think it's pretty straightforward -- just install the version you want the moment the test starts.

debugging is even worse, as the environments are aggressively dropped when the tests complete, so you can't do anything postmortem.

There's a custom --keep-tmpdir option that can be passed to pytest, though, which can be used to prevent pip's test suite from deleting the tests. Like tox -e py36 -- ... --keep-tmpdir.

I feel we should improve pip's documentation related to testing; there's quite a few "little things that make your life easier if you know them" while working with the test suite.

@pfmoore
Copy link
Member Author

pfmoore commented Aug 27, 2018

(Note: This isn't actually that important, as I was mainly just venting based on frustration. However...)

I think it's pretty straightforward -- just install the version you want the moment the test starts.

It's really not. For example, pip sets up a build environment, and installs setuptools into that on your behalf. You can't install setuptools when the test starts and affect that - you have to alter the contents of the common_wheels fixture. As that's downloaded off the internet, you can't put an unpublished setuptools in there without further changes. (And changing the test suite to help debug your test failure is a great way to introduce opportunities for extra bugs 😢)

And in the case I actually had a problem with, the issue was that the test incorrectly failed to install setuptools, and "accidentally" got whatever virtualenv dumped in there, so from a review of the test it looked like it had no setuptools, rather than the wrong version.

Basically, I wasted the best part of 2 weeks trying to work out why I wasn't getting the results I expected. I don't count that as "pretty straightforward" :-(

There's a custom --keep-tmpdir option that can be passed to pytest

I know, I added it. But that doesn't keep the build environment, which is deleted by different code. Nor does it keep the virtualenv fixture's virtualenv directory, which is also deleted by that fixture itself. Both of those contained data I needed to review when I was trying to work out why I was getting the wrong setuptools.

Basically, I think there's a lot of places where the test suite has a lot of complexity. I don't want to say "unnecessary" complexity, although I suspect that is the case, but I do think there's a good case for reviewing whether the test scaffolding can be simplified. I haven't measured it (I'm not sure I'd know how to go about doing so, even) but I suspect that the number of subprocesses fired off, and the number of temporary directories created and deleted, in a single run of pip's test suite, are huge. That likely contributes to both the runtime of the test suite and the difficulties people can have with diagnosing test failures.

I'm not offering at the moment to do anything to improve the test suite, so I don't want anyone to think I'm criticising here - but nevertheless, I do think it's naive to think that we couldn't make significant improvements in the test suite, with sufficient time and motivation.

@pradyunsg pradyunsg added the S: needs triage Issues/PRs that need to be triaged label Sep 12, 2018
@pradyunsg
Copy link
Member

@pfmoore Is there anything actionable here?

@pfmoore
Copy link
Member Author

pfmoore commented Sep 18, 2018

@pradyunsg well the basic question (where did the version of setuptools that was being used for the tests come from) has been answered. But I do think there's still an actionable issue here - the tests don't uniformly ensure that they are using the latest version of setuptools. That's bad, as it's the cause of confusion and wasted time as evidenced here, as well as meaning the tests can trigger already-fixed bugs in setuptools (again as happened here).

The required action is to update the test suite (probably via one of the many "set up the test environment" fixtures) to ensure a consistent and up to date base environment. It's low priority and requires a relatively large amount of effort, so it's not likely to happen soon, but I'm OK with that. I'd prefer the issue to remain open as a reminder of the need, though.

@pradyunsg pradyunsg added type: enhancement Improvements to functionality C: tests Testing and related things and removed S: needs triage Issues/PRs that need to be triaged labels Sep 18, 2018
@pradyunsg
Copy link
Member

Sounds fair to me. :)

@pradyunsg
Copy link
Member

pradyunsg commented Sep 2, 2022

We're definitely using the latest setuptools in our tests now.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: tests Testing and related things type: enhancement Improvements to functionality
Projects
None yet
Development

No branches or pull requests

3 participants