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

Simplify testing for new contributors #2568

Merged
merged 9 commits into from
Jul 12, 2018
1 change: 0 additions & 1 deletion .env
Original file line number Diff line number Diff line change
@@ -1,2 +1 @@
HELLO=WORLD
PYPI_VENDOR_DIR="./tests/pypi/"
42 changes: 42 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,45 @@ Please be aware of the following things when filing bug reports:
If you do not provide all of these things, it will take us much longer to
fix your problem. If we ask you to clarify these and you never respond, we
will close your issue without fixing it.

## Development Setup

To get your development environment setup, run:

```sh
pip install -e .
pipenv install --dev
```

This will install the repo version of Pipenv and then install the development
dependencies. Once that has completed, you can start developing.

The repo version of Pipenv must be installed over other global versions to
resolve conflicts with the `pipenv` folder being implicitly added to `sys.path`.
See pypa/pipenv#2557 for more details.

### Testing

Tests are written in `pytest` style and can be run very simply:

```sh
pytest
```

This will run all Pipenv tests, which can take awhile. To run a subset of the
tests, the standard pytest filters are available, such as:

- provide a directory or file: `pytest tests/unit` or `pytest tests/unit/test_cmdparse.py`
- provide a keyword expression: `pytest -k test_lock_editable_vcs_without_install`
- provide a nodeid: `pytest tests/unit/test_cmdparse.py::test_parse`
- provide a test marker: `pytest -m lock`

#### Package Index

To speed up testing, tests that rely on a package index for locking and
installing use a local server that contains vendored packages in the
`tests/pypi` directory. Each vendored package should have it's own folder
containing the necessary releases. When adding a release for a package, it is
easiest to use either the `.tar.gz` or universal wheels (ex: `py2.py3-none`). If
a `.tar.gz` or universal wheel is not available, add wheels for all available
architectures and platforms.
3 changes: 2 additions & 1 deletion pytest.ini
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
[pytest]
addopts = -n auto
norecursedirs = vendor patched
; Add vendor and patched in addition to the default list of ignored dirs
norecursedirs = .* build dist CVS _darcs {arch} *.egg vendor patched
2 changes: 1 addition & 1 deletion run-tests.bat
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ virtualenv R:\.venv
R:\.venv\Scripts\pip install -e . --upgrade --upgrade-strategy=only-if-needed
R:\.venv\Scripts\pipenv install --dev

SET RAM_DISK=R:&& SET PYPI_VENDOR_DIR=".\tests\pypi\" && R:\.venv\Scripts\pipenv run pytest -n auto -v tests --tap-stream > report.tap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PYPI_VENDOR_DIR setting is 100% necessary if you want your tests to pass, I'm not sure why you're removing it??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line handles it when running with just pytest. https://github.com/pypa/pipenv/blob/master/tests/integration/conftest.py#L71

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty confident that line doesn't actually do anything. Buildkite failures are due to the removal of this setting in the other script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, indeed haha, I must've been running it from the test directory and the implicit ./pypa in app.py worked. I had a different version with more explicitly loaded packages that I must've been thinking of. I'll double test that locally and push.

SET RAM_DISK=R: && R:\.venv\Scripts\pipenv run pytest -n auto -v tests --tap-stream > report.tap
3 changes: 0 additions & 3 deletions run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@

set -eo pipefail

# Set the PYPI vendor URL for pytest-pypi.
PYPI_VENDOR_DIR="$(pwd)/tests/pypi/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same rules apply. Please leave this setting alone.

export PYPI_VENDOR_DIR
export PYTHONIOENCODING="utf-8"
export LANG=C.UTF-8

Expand Down
4 changes: 3 additions & 1 deletion tests/integration/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from pipenv.vendor import requests
from pipenv.vendor import six
from pipenv.vendor import toml
from pytest_pypi.app import prepare_packages as prepare_pypi_packages

if six.PY2:
class ResourceWarning(Warning):
Expand All @@ -30,6 +31,8 @@ def check_internet():
WE_HAVE_INTERNET = check_internet()

TESTS_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
PYPI_VENDOR_DIR = os.path.join(TESTS_ROOT, 'pypi')
prepare_pypi_packages(PYPI_VENDOR_DIR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes a lot of sense to me.



def pytest_runtest_setup(item):
Expand Down Expand Up @@ -68,7 +71,6 @@ def __enter__(self):
os.environ['PIPENV_DONT_USE_PYENV'] = '1'
os.environ['PIPENV_IGNORE_VIRTUALENVS'] = '1'
os.environ['PIPENV_VENV_IN_PROJECT'] = '1'
os.environ['PYPI_VENDOR_DIR'] = os.path.join(TESTS_ROOT, 'pypi')
if self.chdir:
os.chdir(self.path)
return self
Expand Down
10 changes: 8 additions & 2 deletions tests/integration/test_install_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,10 +359,16 @@ def test_install_venv_project_directory(PipenvInstance, pypi):
os.environ["WORKON_HOME"] = workon_home.name
if "PIPENV_VENV_IN_PROJECT" in os.environ:
del os.environ["PIPENV_VENV_IN_PROJECT"]

c = p.pipenv("install six")
assert c.return_code == 0
project = Project()
assert Path(project.virtualenv_location).joinpath(".project").exists()

venv_loc = None
for line in c.err.splitlines():
if line.startswith("Virtualenv location:"):
venv_loc = Path(line.split(":", 1)[-1].strip())
assert venv_loc is not None
assert venv_loc.joinpath(".project").exists()


@pytest.mark.deploy
Expand Down
59 changes: 33 additions & 26 deletions tests/pytest-pypi/pytest_pypi/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,51 +4,54 @@
import requests
from flask import Flask, redirect, abort, render_template, send_file, jsonify

PYPI_VENDOR_DIR = os.environ.get('PYPI_VENDOR_DIR', './pypi')
PYPI_VENDOR_DIR = os.path.abspath(PYPI_VENDOR_DIR)

app = Flask(__name__)
session = requests.Session()

packages = {}


class Package(object):
Copy link
Contributor Author

@JacobHayes JacobHayes Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main differences here are:

  • releases is now a dict of {"release name": "path to file"}
  • a new json property that tries to find an api.json in any folder containing one of the releases (replaces the implicit package dir in json_for_package)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will allow us to build out testing infrastructure which actually includes testing of the json api, which seems like it might be helpful....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was surprised I didn't have any tests to fix for the pytest-pypi package itself. 😅

"""docstring for Package"""
"""Package represents a collection of releases from one or more directories"""

def __init__(self, name):
super(Package, self).__init__()
self.name = name
self._releases = []
self.releases = {}
self._package_dirs = set()

@property
def releases(self):
r = []
for release in self._releases:
release = release[len(PYPI_VENDOR_DIR):].replace('\\', '/')
r.append(release)
return r
def json(self):
for path in self._package_dirs:
try:
with open(os.path.join(path, 'api.json')) as f:
return json.load(f)
except FileNotFoundError:
pass

def __repr__(self):
return "<Package name={0!r} releases={1!r}".format(self.name, len(self.releases))

def add_release(self, path_to_binary):
self._releases.append(path_to_binary)


def prepare_packages():
for root, dirs, files in os.walk(os.path.abspath(PYPI_VENDOR_DIR)):
path_to_binary = os.path.abspath(path_to_binary)
path, release = os.path.split(path_to_binary)
self.releases[release] = path_to_binary
self._package_dirs.add(path)


def prepare_packages(path):
"""Add packages in path to the registry."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are so straightforward and so much more readable. I've looked at tackling a lot of this before, basically spent about 5 minutes on it and ducked out because I broke everything... Thank you for the cleanup, this is very easy to follow and the changes are both logical and clear.

path = os.path.abspath(path)
if not (os.path.exists(path) and os.path.isdir(path)):
raise ValueError("{} is not a directory!".format(path))
for root, dirs, files in os.walk(path):
for file in files:
if not file.startswith('.') and not file.endswith('.json'):
package_name = root.split(os.path.sep)[-1]
package_name = os.path.basename(root)

if package_name not in packages:
packages[package_name] = Package(package_name)

packages[package_name].add_release(os.path.sep.join([root, file]))


prepare_packages()
packages[package_name].add_release(os.path.join(root, file))


@app.route('/')
Expand All @@ -74,22 +77,26 @@ def serve_package(package, release):
if package in packages:
package = packages[package]

for _release in package.releases:
if _release.endswith(release):
return send_file(os.path.sep.join([PYPI_VENDOR_DIR, _release]))
if release in package.releases:
return send_file(package.releases[release])

abort(404)


@app.route('/pypi/<package>/json')
def json_for_package(package):
try:
with open(os.path.sep.join([PYPI_VENDOR_DIR, package, 'api.json'])) as f:
return jsonify(json.load(f))
return jsonify(packages[package].json)
except Exception:
pass

r = session.get('https://pypi.org/pypi/{0}/json'.format(package))
return jsonify(r.json())


if __name__ == '__main__':
PYPI_VENDOR_DIR = os.environ.get('PYPI_VENDOR_DIR', './pypi')
PYPI_VENDOR_DIR = os.path.abspath(PYPI_VENDOR_DIR)
prepare_packages(PYPI_VENDOR_DIR)

app.run()
4 changes: 2 additions & 2 deletions tests/pytest-pypi/pytest_pypi/templates/package.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
<body>
<h1>Links for {{ package.name }}</h1>
{% for release in package.releases %}
<a href="{{ release }}">{{ release }}</a>
<a href="/{{ package.name }}/{{ release }}">{{ release }}</a>
<br>
{% endfor %}
</body>
</html>
</html>