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

Conversation

JacobHayes
Copy link
Contributor

@JacobHayes JacobHayes commented Jul 11, 2018

Addresses some difficulties encountered when trying to get up to speed contributing to the project.

  • Fixes pytest.ini to ignore things like build dirs when looking for tests
  • Fixes resolving vendored test packages when running tests outside the repo root
  • Add some docs on development setup and testing

There are a few more structural decisions that this doesn't address:

  • repo pipenv shadowing global package when bootstrapping (Can't bootstrap development environment #2557)
  • The docker-compose image doesn't have python 3.7, so make test doesn't work
  • The test Dockerfile is outside this repo, so ^ can't be fixed as easily

@JacobHayes
Copy link
Contributor Author

Is there any way to see the output of buildkite without being added to the org?

A copy/paste would be much appreciated. :)

@@ -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.

@@ -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.

@techalchemy
Copy link
Member

techalchemy commented Jul 11, 2018

Buildkite failures:

tests/integration/test_cli.py:77: AssertionError
----------------------------- Captured stdout call -----------------------------
$ pipenv install requests==2.18.4
Installing requests==2.18.4…
Looking in indexes: http://127.0.0.1:41471/simple
Collecting requests==2.18.4
 
Creating a virtualenv for this project…
Pipfile: /tmp/pipenv-EoS69E-project/Pipfile
Using /root/.local/share/virtualenvs/pipenv-wInVs968-2.7/bin/python2.7 (2.7.15) to create virtualenv…
Already using interpreter /root/.local/share/virtualenvs/pipenv-wInVs968-2.7/bin/python2.7
Using real prefix '/usr'
New python executable in /tmp/pipenv-EoS69E-project/.venv/bin/python2.7
Also creating executable in /tmp/pipenv-EoS69E-project/.venv/bin/python
Installing setuptools, pip, wheel...done.
 
Virtualenv location: /tmp/pipenv-EoS69E-project/.venv
Creating a Pipfile for this project…
Error:  An error occurred while installing requests==2.18.4!
  Could not find a version that satisfies the requirement requests==2.18.4 (from versions: )
No matching distribution found for requests==2.18.4
 
$ pipenv graph --reverse
certifi==2018.04.16
  - pipenv==2018.7.1 [requires: certifi]
pip==10.0.1
  - pipenv==2018.7.1 [requires: pip>=9.0.1]
setuptools==40.0.0
  - pipenv==2018.7.1 [requires: setuptools>=36.2.1]
virtualenv==?
  - pipenv==2018.7.1 [requires: virtualenv]
virtualenv-clone==?
  - pipenv==2018.7.1 [requires: virtualenv-clone>=0.2.5]
wheel==0.31.1
 

----------------------------- Captured stderr call -----------------------------
127.0.0.1 - - [11/Jul/2018 18:27:28] "GET /simple/requests/ HTTP/1.1" 404 233

@techalchemy
Copy link
Member

(also unfortunately there is no way to give you access to this, I have tried to get this sorted but it's above my paygrade)

@JacobHayes
Copy link
Contributor Author

Ok, pushed a change to set the package dir explicitly and tested it from /, /tests and /tests/integration to verify. And yay, buildkite passed!

It's a bit larger of a change, but it removes the implicit dependency of setting the env var before pytest loads the plugin (which happens before conftest.py). pytest-pypi will still have a env-var based vendor dir if run as __main__, but otherwise it will need to be primed explicitly to be populated.

@@ -4,51 +4,56 @@
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

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. 😅

@techalchemy
Copy link
Member

Thanks for your efforts on this, feel free to reach out via your preferred real-time communication channel if you want to solicit specific feedback (I'm available on IRC/slack/whatever really)

Copy link
Member

@techalchemy techalchemy left a comment

Choose a reason for hiding this comment

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

I think these are valuable changes, thank you for tackling them and taking the time to explain and rectify some of these challenges. They have been kind of floating at the back of my mind for some time but I haven't had the time to focus on them.

@@ -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 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.

@@ -4,51 +4,56 @@
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
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....

@techalchemy
Copy link
Member

I need to make sure VSTS still builds against this before I can merge

@uranusjr
Copy link
Member

I run the tests on my local Windows machine. One failure:

_____________________ test_install_venv_project_directory _____________________
[gw5] win32 -- Python 3.7.0 c:\users\uranusjr\documents\programming\pipenv\.venv\scripts\python.exe

PipenvInstance = <class 'tests.integration.conftest._PipenvInstance'>
pypi = <pytest_pypi.serve.Server object at 0x0000027CF2455F28>

    @pytest.mark.install
    @pytest.mark.virtualenv
    def test_install_venv_project_directory(PipenvInstance, pypi):
        """Test the project functionality during virtualenv creation.
        """
        with PipenvInstance(pypi=pypi, chdir=True) as p:
            with temp_environ(), TemporaryDirectory(
                prefix="pipenv-", suffix="temp_workon_home"
            ) as workon_home:
                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()
E               AssertionError: assert False
E                +  where False = <bound method Path.exists of WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv/.project')>()
E                +    where <bound method Path.exists of WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv/.project')> = WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv/.project').exists
E                +      where WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv/.project') = <bound method PurePath.joinpath of WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv')>('.project')
E                +        where <bound method PurePath.joinpath of WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv')> = WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv').joinpath
E                +          where WindowsPath('C:/Users/uranusjr/Documents/programming/pipenv/.venv') = Path('C:\\Users\\uranusjr\\Documents\\programming\\pipenv\\.venv')
E                +            where 'C:\\Users\\uranusjr\\Documents\\programming\\pipenv\\.venv' = <pipenv.project.Project object at 0x0000027CF2483BE0>.virtualenv_location

tests\integration\test_install_basic.py:365: AssertionError
---------------------------- Captured stdout call -----------------------------
$ pipenv install six
Installing six…
Looking in indexes: http://127.0.0.1:3348/simple
Collecting six
  Downloading http://127.0.0.1:3348/six/six-1.11.0-py2.py3-none-any.whl
Installing collected packages: six
Successfully installed six-1.11.0

Adding six to Pipfile's [packages]…
Installing dependencies from Pipfile.lock (2a6c8a)…
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.

Creating a virtualenv for this project…
Pipfile: R:\9893bf199154429c87a8c5cb903351c0\Pipfile
Using c:\users\uranusjr\documents\programming\pipenv\.venv\scripts\python.exe (3.7.0) to create virtualenv…
Already using interpreter c:\users\uranusjr\documents\programming\pipenv\.venv\scripts\python.exe

Using base prefix 'C:\\Users\\uranusjr\\AppData\\Local\\Programs\\Python\\Python37'

c:\users\uranusjr\documents\programming\pipenv\.venv\lib\site-packages\virtualenv.py:1041: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses

  import imp

New python executable in R:\1bf6f8b890de4da391d12c1747a1b67e\9893bf199154429c87a8c5cb903351c0-fG1u2CNc\Scripts\python.exe

Installing setuptools, pip, wheel...done.


Virtualenv location: R:\1bf6f8b890de4da391d12c1747a1b67e\9893bf199154429c87a8c5cb903351c0-fG1u2CNc
Creating a Pipfile for this project…
Pipfile.lock not found, creating…
Locking [dev-packages] dependencies…
Locking [packages] dependencies…
Updated Pipfile.lock (2a6c8a)!

---------------------------- Captured stderr call -----------------------------
127.0.0.1 - - [13/Jul/2018 02:32:20] "GET /simple/six/ HTTP/1.1" 200 310
127.0.0.1 - - [13/Jul/2018 02:32:20] "GET /six/six-1.11.0-py2.py3-none-any.whl HTTP/1.1" 200 10702
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /simple/six/ HTTP/1.1" 200 310
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /simple/six/ HTTP/1.1" 200 310
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /six/six-1.11.0-py2.py3-none-any.whl HTTP/1.1" 200 10702
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /simple/six/ HTTP/1.1" 200 310
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /six/six-1.11.0-py2.py3-none-any.whl HTTP/1.1" 200 10702
127.0.0.1 - - [13/Jul/2018 02:32:29] "GET /six/six-1.11.0.tar.gz HTTP/1.1" 200 29860

@techalchemy
Copy link
Member

@uranusjr this is the exact same issue i've been showing you with the pythonfinder implementation. It installs the package without creating a virtualenv -- for all intents and purposes there should be a new virtualenv created before we do the installation.

I think this all relates to the definition of project = Project(which=which) in core.py. It doesn't re-evaluate what the relevant project is on every call so we keep using the old one during installation. Thoughts? Does this wind up working if you switch that to project = lambda: Project(which=which) ?

The Project class's implementation is a fucking mess. Avoid that.
@uranusjr
Copy link
Member

I give up. resorted to parsing the virtualenv’s location directly from command output. Much simpler. We’ll deal with the Project mess later.

@uranusjr uranusjr merged commit 9460500 into pypa:master Jul 12, 2018
@JacobHayes JacobHayes deleted the make-local-test-easy branch July 12, 2018 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants