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

Tests using internet but not declared as such #3630

Closed
jayvdb opened this issue Mar 20, 2019 · 7 comments · Fixed by #4449
Closed

Tests using internet but not declared as such #3630

jayvdb opened this issue Mar 20, 2019 · 7 comments · Fixed by #4449
Labels
Category: Tests Relates to tests. good first issue Issues suitable as a newcomer to get familiar with Pipenv!

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Mar 20, 2019

I am packaging pipenv for openSUSE, which means the tests must run without internet.

For the most part the internet detection and skipping works great, however I believe the following use the internet without having the internet marker.

  • test_download_file
  • test_pipenv_clean_pip_no_warnings
  • test_environment_variable_value_does_not_change_hash
  • test_run_in_virtualenv
  • test_lock_with_incomplete_source

Only the first is in unit tests, and it is very clearly intentionally needing to use internet. If the internet marker isnt suitable for unit tests, perhaps a simple requests mock would be sufficient?

The others are integration tests, and it is a bit harder to verify these are supposed to be using the internet, as they should probably be intercepted by pytest_pypi.

Also while on the subject, it would be worth skipping test_system_and_deploy_work if the location of system packages isnt writable, otherwise that also fails during rpmbuild as it tries to uninstall build dependencies such as certifi and fails with :

[pipenv.exceptions.InstallError]: ["Could not install packages due to an EnvironmentError: [Errno 13] Permission denied: '__init__.cpython-37.opt-1.pyc'", 'Consider using the `--user` option or check the permissions.']
@frostming
Copy link
Contributor

Thanks for reporting this, it would be great to improve our test suites and PRs are welcomed.

@uranusjr uranusjr added good first issue Issues suitable as a newcomer to get familiar with Pipenv! Category: Tests Relates to tests. labels Mar 21, 2019
@jayvdb
Copy link
Contributor Author

jayvdb commented Mar 26, 2019

Another five pipenv failures when offline documented at sarugaku/requirementslib#152

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 3, 2019

test_pipenv_clean_pip_no_warnings and test_run_in_virtualenv both using run pip .. , and I guess that pip is not instructed to use the fake simple index provided by pytest_pypi.

If so, that feels a bit like a bug actually, as the venv's pipenv has been told to use an alternative pypi index, and so pip inside of the venv "should" be using the same index. I believe that is possible by setting envvars. I'll let others chew on that, or toss it out if it is silly.

I guess test_lock_with_incomplete_source needs internet because it is using test.pypi.org. If so, then the test test_lock_after_update_source_name (not in the last release) will need the same treatment.

But I do not understand why test_environment_variable_value_does_not_change_hash uses the internet. The test is supposed to be use the pypi fake index.

It fails at this assert

              c = p.pipenv('install')
              assert c.return_code == 0

I've tried changing the package being installed, e.g. to tablib, and it doesnt appear to make any difference.

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 3, 2019

As an aside, it would be nice to have an automated way of ensuring the internet marker is added, e.g. a job with internet disabled and using the internet marker to skip properly declared tests. However it doesnt seem to be possible atm using pytest voodoo. e.g. miketheman/pytest-socket#25 and kevin1024/vcrpy#425

Probably needs to be done at the OS level. e.g. a sudo job with resolv.conf edited during the test run.

@GPHemsley
Copy link
Contributor

test_convert_deps_to_pip and test_download_file were marked as needing internet in de5cdf5.

GPHemsley added a commit to GPHemsley/pipenv that referenced this issue May 29, 2020
@GPHemsley
Copy link
Contributor

It appears the tests affected by this issue fall into two categories: they either directly specify a 'url' or 'git' value in a custom Pipfile, or they call 'pip' directly.

By my estimation, the tests that match one of those scenarios but are missing a needs_internet marker are:

  • test_pipenv_clean_pip_no_warnings
  • test_pipenv_clean_pip_warnings
  • test_environment_variable_value_does_not_change_hash
  • test_lock_with_incomplete_source
  • test_lock_after_update_source_name
  • test_get_source
  • test_many_indexes
  • test_run_in_virtualenv_with_global_context
  • test_run_in_virtualenv
  • test_mirror_lock_sync
  • test_pipenv_clean_windows

I'm not sure the easiest way for me to confirm that these tests are indeed all reaching out to the internet when they run.

@GPHemsley
Copy link
Contributor

@jayvdb I would recommend filing separate individual issues for the following:

Also while on the subject, it would be worth skipping test_system_and_deploy_work if the location of system packages isnt writable, otherwise that also fails during rpmbuild as it tries to uninstall build dependencies such as certifi and fails with :

[pipenv.exceptions.InstallError]: ["Could not install packages due to an EnvironmentError: [Errno 13] Permission denied: '__init__.cpython-37.opt-1.pyc'", 'Consider using the `--user` option or check the permissions.']

test_pipenv_clean_pip_no_warnings and test_run_in_virtualenv both using run pip .. , and I guess that pip is not instructed to use the fake simple index provided by pytest_pypi.

If so, that feels a bit like a bug actually, as the venv's pipenv has been told to use an alternative pypi index, and so pip inside of the venv "should" be using the same index. I believe that is possible by setting envvars. I'll let others chew on that, or toss it out if it is silly.


As an aside, it would be nice to have an automated way of ensuring the internet marker is added, e.g. a job with internet disabled and using the internet marker to skip properly declared tests. However it doesnt seem to be possible atm using pytest voodoo. e.g. miketheman/pytest-socket#25 and kevin1024/vcrpy#425

Probably needs to be done at the OS level. e.g. a sudo job with resolv.conf edited during the test run.

frostming added a commit that referenced this issue Sep 1, 2020
Fix #3630 Adapt test cases to prefer the mocked PyPI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Tests Relates to tests. good first issue Issues suitable as a newcomer to get familiar with Pipenv!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants