Avoid using requests unnecessarily in tests #3670
Conversation
|
I have at least another test which should have this type of change, so I'll add it here shortly. |
|
Note in At a latter stage, we need to better understand just how much of pipenv fails when de-vendoring. Probably after the next release, when all distros re-do their de-vendoring, we can focus their attention on those failures which they like to skip and hope for the best. I can think of a few basic tests which could be added, such as installing specific versions of |
|
I believe this is all of the changes needed to resolve this issue. The only other test failures I have are listed at #3630 , and I am fairly confident they are not the same problem as this issue. Im happy to revise this patch heavily based on feedback. I probably havent made the best choices of alternative packages, and there are probably changes needed to match the local style, etc. |
jayvdb
force-pushed
the
test_include_editable_packages-not-requests
branch
from
ec5f5fd to
39248c2
Compare
| # Ensure the --pypi-mirror parameter hasn't altered the Pipfile or Pipfile.lock sources | ||
| assert len(p.pipfile["source"]) == 1 | ||
| assert len(p.lockfile["_meta"]["sources"]) == 1 | ||
| assert "https://pypi.org/simple" == p.pipfile["source"][0]["url"] | ||
| assert "https://pypi.org/simple" == p.lockfile["_meta"]["sources"][0]["url"] | ||
|
|
||
| c = p.pipenv("run python -m requests.help") | ||
| c = p.pipenv("run python -m django --version") |
There was a problem hiding this comment.
Note Django was used here to continue using the run python -m .. syntax, and there are not many packages in tests/pypi/ that have a __main__. We could add another lighter package with a __main__ to the pypi archive .
Or this could be changed to using run python -c 'import x' which is almost as good, with a slightly less understandable error if the test fails. But I think 99% of the people likely to encounter an error here are going to understand the backtrace caused by import x failing.
| @@ -119,16 +119,16 @@ def test_install_without_dev(PipenvInstance, pypi): | |||
| six = "*" | |||
|
|
|||
| [dev-packages] | |||
| pytz = "*" | |||
| tablib = "*" | |||
There was a problem hiding this comment.
This replacing of pytz with tablib is a slightly different from the others, which are replacing requests.
pytz is not a dependency of pipenv, however it is a dependency of the test runner, so it is also added in PYTHONPATH in order to get the builds working in rpmbuild.
I can workaround this by creating a special venv with only the real dependencies of pipenv, and using that in PYTHONPATH. But that is quite a bit more effort, and I doubt many distro maintainers would accept a much more complicated testing rig just so this one test passes - they would likely prefer to just mark this test as a skip.
Note I do not expect pipenv devs to always be avoiding requests and other dependencies in these tests. It would be nice if they were avoided, especially requests, but it is not possible to predict in advance all of the dependencies, especially all of the test rig dependencies.
I think the responsibility of creating fixup patches like this one falls on the de-vendor-ers after each release.
Also we should also be working on creating a job which blocks internet access so that the 'internet' marker becomes 100% reliable here -- so that de-vendor-ers dont feel that some of the failures they see might be due to 'no internet', and hopefully they investigate any failures more before shipping broken package updates to users.
jayvdb
force-pushed
the
test_include_editable_packages-not-requests
branch
from
39248c2 to
8604d58
Compare
|
oops I triggered azure's abuse detection :o Sorry this took so long but there were a lot of issues to untangle, once I can run the tests here I will address the specific comments. Generally speaking I have no issues swapping out one library for another and if you have implementations for moving things offline I am more than happy to merge them. |
techalchemy
added
Category: Tests
|
Glad to see pipenv has green builds again. I'll keep a close eye on this PR and happy to change it as needed. If I have spare time after this, my focus will be to create new versions of these tests which are written so they fail when de-vendoring and directly demonstrate the unsolved problems resulting from de-vendoring, with a pytest mark so they are easy to skip as a group. |
|
@jayvdb Please merge and we are good to go. |
|
ok, on it. |
This replaces requests with tablib in test_include_editable_packages as in a de-vendored environment the locking fails with requests. Continuation of pypa#3644
These tests fail when using requests in a de-vendored installation because requests is part of the pre-installed image. tablib has fewer constraints on its dependencies, however its dependency tree is complicated enough to approximately test the same functionality, and allows the graph tests to pass even when de-vendored.
Using requests in tests fails when pipenv is de-vendored. In this test, using any package with multiple versions in the local pypi data will suffice to show the functionality does work, but not with requests, or any devendored package. Continuation of pypa#3644
pytz exists in the build environment, which will typically be added to the PYTHONPATH when running the tests in rpmbuild. The alternative is to create a virtualenv containing only the pipenv dependencies, which is cumbersome to do when devendoring. As the pytz library here is not critical to the logic of the test method, replace it with tablib to reduce unintentionally errors when packaging. Continuation of pypa#3644
jayvdb
force-pushed
the
test_include_editable_packages-not-requests
branch
from
daf69c6 to
2f5150a
Compare
|
Thanks for the work, merging. |
|
Hey thanks again for all the effort on this, I know this was a lot of work |
|
@jayvdb if you have any notes on your decisionmaking process it might be helpful to begin updating our contributing guidelines to factor in your process into how we decide what to use on future tests |
This replaces requests with tablib in tests where using requests
in a de-vendored environment causes the tests to fail unnecessarily.
Continuation of #3644
Thank you for contributing to Pipenv!
The issue
What is the thing you want to fix? Is it associated with an issue on GitHub? Please mention it.
Always consider opening an issue first to describe your problem, so we can discuss what is the best way to amend it. Note that if you do not describe the goal of this change or link to a related issue, the maintainers may close the PR without further review.
If your pull request makes a non-insignificant change to Pipenv, such as the user interface or intended functionality, please file a PEEP.
The fix
How does this pull request fix your problem? Did you consider any alternatives? Why is this the best solution, in your opinion?
The checklist
news/directory to describe this fix with the extension.bugfix,.feature,.behavior,.doc..vendor. or.trivial(this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.