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

First stab at getting tox to run tests and coverage for py37 and py38 #30

Closed
wants to merge 1 commit into from

Conversation

stevepiercy
Copy link
Member

See #28

@stevepiercy stevepiercy added the enhancement New feature or request label Aug 4, 2019
@stevepiercy stevepiercy mentioned this pull request Aug 4, 2019
@stevepiercy
Copy link
Member Author

The expected output is below assuming you don't have py38 installed (I don't yet).

I commented out lines in the files instead of deleting them. I am not 100% certain whether to remove them and would like feedback before deleting them.

tox will run tests on py37, py38, and run coverage.

I could also add flake8, black, build, or any other environment, which Windows users will enjoy since make files are kind of a challenge for them.

The purpose of this PR is not to have CI with Travis or Appveyor yet, which could come in a subsequent PR. It is only intended to allow tox to run successfully.

Steves-iMac:pyramid_openapi3 stevepiercy$ env/bin/tox
GLOB sdist-make: /Users/stevepiercy/projects/pyramid_openapi3/setup.py
py37 inst-nodeps: /Users/stevepiercy/projects/pyramid_openapi3/.tox/.tmp/package/1/pyramid_openapi3-0.3.0.zip
py37 installed: apipkg==1.5,atomicwrites==1.3.0,attrs==19.1.0,coverage==4.5.4,execnet==1.6.1,hupper==1.8.1,importlib-metadata==0.19,jsonschema==3.0.2,lazy-object-proxy==1.4.1,more-itertools==7.2.0,openapi-core==0.11.0,openapi-spec-validator==0.2.8,packaging==19.1,PasteDeploy==2.0.1,plaster==1.0,plaster-pastedeploy==0.7,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.2,pyramid==1.10.4,pyramid-openapi3==0.3.0,pyrsistent==0.15.4,pytest==5.0.1,pytest-cov==2.7.1,pytest-forked==1.0.2,pytest-xdist==1.29.0,PyYAML==5.1.2,six==1.12.0,strict-rfc3339==0.7,translationstring==1.3,venusian==1.2.0,wcwidth==0.1.7,WebOb==1.8.5,zipp==0.5.2,zope.deprecation==4.4.0,zope.interface==4.6.0
py37 run-test-pre: PYTHONHASHSEED='735715902'
py37 run-test: commands[0] | pytest
===================================================================== test session starts =====================================================================
platform darwin -- Python 3.7.1, pytest-5.0.1, py-1.8.0, pluggy-0.12.0
cachedir: .tox/py37/.pytest_cache
rootdir: /Users/stevepiercy/projects/pyramid_openapi3, inifile: setup.cfg, testpaths: pyramid_openapi3/tests
plugins: cov-2.7.1, forked-1.0.2, xdist-1.29.0
collected 21 items                                                                                                                                            

pyramid_openapi3/tests/test_add_formatter.py ..                                                                                                         [  9%]
pyramid_openapi3/tests/test_validation.py .......                                                                                                       [ 42%]
pyramid_openapi3/tests/test_views.py .........                                                                                                          [ 85%]
pyramid_openapi3/tests/test_wrappers.py ...                                                                                                             [100%]

====================================================================== warnings summary =======================================================================
.tox/py37/lib/python3.7/site-packages/venusian/__init__.py:1
  /Users/stevepiercy/projects/pyramid_openapi3/.tox/py37/lib/python3.7/site-packages/venusian/__init__.py:1: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/latest/warnings.html

---------- coverage: platform darwin, python 3.7.1-final-0 -----------
Name                                           Stmts   Miss     Cover   Missing
-------------------------------------------------------------------------------
pyramid_openapi3/__init__.py                      68      0   100.00%
pyramid_openapi3/exceptions.py                    23      0   100.00%
pyramid_openapi3/tests/__init__.py                 0      0   100.00%
pyramid_openapi3/tests/test_add_formatter.py       9      0   100.00%
pyramid_openapi3/tests/test_validation.py        124      0   100.00%
pyramid_openapi3/tests/test_views.py             164      0   100.00%
pyramid_openapi3/tests/test_wrappers.py           41      0   100.00%
pyramid_openapi3/tween.py                         31      0   100.00%
pyramid_openapi3/wrappers.py                      43      0   100.00%
-------------------------------------------------------------------------------
TOTAL                                            503      0   100.00%

============================================================ 21 passed, 1 warnings in 1.43 seconds ============================================================
py38 create: /Users/stevepiercy/projects/pyramid_openapi3/.tox/py38
SKIPPED: InterpreterNotFound: python3.8
coverage inst-nodeps: /Users/stevepiercy/projects/pyramid_openapi3/.tox/.tmp/package/1/pyramid_openapi3-0.3.0.zip
coverage installed: apipkg==1.5,atomicwrites==1.3.0,attrs==19.1.0,coverage==4.5.4,execnet==1.6.1,hupper==1.8.1,importlib-metadata==0.19,jsonschema==3.0.2,lazy-object-proxy==1.4.1,more-itertools==7.2.0,openapi-core==0.11.0,openapi-spec-validator==0.2.8,packaging==19.1,PasteDeploy==2.0.1,plaster==1.0,plaster-pastedeploy==0.7,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.2,pyramid==1.10.4,pyramid-openapi3==0.3.0,pyrsistent==0.15.4,pytest==5.0.1,pytest-cov==2.7.1,pytest-forked==1.0.2,pytest-xdist==1.29.0,PyYAML==5.1.2,six==1.12.0,strict-rfc3339==0.7,translationstring==1.3,venusian==1.2.0,wcwidth==0.1.7,WebOb==1.8.5,zipp==0.5.2,zope.deprecation==4.4.0,zope.interface==4.6.0
coverage run-test-pre: PYTHONHASHSEED='735715902'
coverage run-test: commands[0] | coverage combine
coverage run-test: commands[1] | coverage xml
coverage run-test: commands[2] | coverage report --show-missing --fail-under=100
Name                                           Stmts   Miss     Cover   Missing
-------------------------------------------------------------------------------
pyramid_openapi3/__init__.py                      68      0   100.00%
pyramid_openapi3/exceptions.py                    23      0   100.00%
pyramid_openapi3/tests/__init__.py                 0      0   100.00%
pyramid_openapi3/tests/test_add_formatter.py       9      0   100.00%
pyramid_openapi3/tests/test_validation.py        124      0   100.00%
pyramid_openapi3/tests/test_views.py             164      0   100.00%
pyramid_openapi3/tests/test_wrappers.py           41      0   100.00%
pyramid_openapi3/tween.py                         31      0   100.00%
pyramid_openapi3/wrappers.py                      43      0   100.00%
-------------------------------------------------------------------------------
TOTAL                                            503      0   100.00%
___________________________________________________________________________ summary ___________________________________________________________________________
  py37: commands succeeded
SKIPPED:  py38: InterpreterNotFound: python3.8
  coverage: commands succeeded
  congratulations :)

.coveragerc Outdated Show resolved Hide resolved
.coveragerc Outdated Show resolved Hide resolved
setup.py Outdated
@@ -68,6 +74,6 @@ def run(self): # noqa: D102
packages=find_packages(exclude=["tests"]),
package_data={"pyramid_openapi3": ["static/*.*"], "": ["LICENSE"]},
install_requires=["openapi-core", "openapi-spec-validator", "pyramid"],
extras_require={':python_version<"3.7"': ["importlib-resources"]},
extras_require={"testing": testing_extras, ':python_version<"3.7"': ["importlib-resources"]},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should then have .[testing] (or sth. similar) in Pipfile, under development section, so that we don't have two lists of development/testing dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know pipfile, but I added stuff in 9c18f73 for review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 1df7ebd per #30 (comment)

@zupo
Copy link
Collaborator

zupo commented Aug 4, 2019

The purpose of this PR is not to have CI with Travis or Appveyor yet [... snip ...]

We already have CircleCI enabled, or do we specifically need/want Travis/Appveyor?

@stevepiercy
Copy link
Member Author

We already have CircleCI enabled, or do we specifically need/want Travis/Appveyor?

CircleCI looks nice, but I'll defer to others who have more experience between it and Travis-CI.

Appveyor is used for testing and building on Windows. Does CircleCI offer that? Travis-CI does not.

@zupo
Copy link
Collaborator

zupo commented Aug 4, 2019

@zupo
Copy link
Collaborator

zupo commented Aug 4, 2019

Does CircleCI offer that?

AFAIK, not yet for everyone, they're in close beta for Windows builds.

@zupo
Copy link
Collaborator

zupo commented Aug 4, 2019

Tox fails for me like so:

pyramid_openapi3 (feature-add-tox)$ tox
GLOB sdist-make: /Users/zupo/work/pyramid_openapi3/setup.py
py37 create: /Users/zupo/work/pyramid_openapi3/.tox/py37
py37 inst: /Users/zupo/work/pyramid_openapi3/.tox/dist/pyramid_openapi3-0.3.0.zip
py37 installed: attrs==19.1.0,hupper==1.8.1,jsonschema==3.0.2,lazy-object-proxy==1.4.1,openapi-core==0.11.0,openapi-spec-validator==0.2.8,PasteDeploy==2.0.1,plaster==1.0,plaster-pastedeploy==0.7,pyramid==1.10.4,pyramid-openapi3==0.3.0,pyrsistent==0.15.4,PyYAML==5.1.2,six==1.12.0,strict-rfc3339==0.7,translationstring==1.3,venusian==1.2.0,WebOb==1.8.5,zope.deprecation==4.4.0,zope.interface==4.6.0
py37 runtests: PYTHONHASHSEED='2478745532'
py37 runtests: commands[0] | pytest
WARNING:test command found but not installed in testenv
  cmd: /usr/local/bin/pytest
  env: /Users/zupo/work/pyramid_openapi3/.tox/py37
Maybe you forgot to specify a dependency? See also the whitelist_externals envconfig setting.
dyld: Library not loaded: @executable_path/../.Python
  Referenced from: /Users/zupo/work/pyramid_heroku/.venv/bin/python3.7
  Reason: image not found
ERROR: InvocationError: '/usr/local/bin/pytest'
py38 create: /Users/zupo/work/pyramid_openapi3/.tox/py38
ERROR: InterpreterNotFound: python3.8
coverage create: /Users/zupo/work/pyramid_openapi3/.tox/coverage
coverage installdeps: coverage
coverage inst: /Users/zupo/work/pyramid_openapi3/.tox/dist/pyramid_openapi3-0.3.0.zip
coverage installed: attrs==19.1.0,coverage==4.5.4,hupper==1.8.1,jsonschema==3.0.2,lazy-object-proxy==1.4.1,openapi-core==0.11.0,openapi-spec-validator==0.2.8,PasteDeploy==2.0.1,plaster==1.0,plaster-pastedeploy==0.7,pyramid==1.10.4,pyramid-openapi3==0.3.0,pyrsistent==0.15.4,PyYAML==5.1.2,six==1.12.0,strict-rfc3339==0.7,translationstring==1.3,venusian==1.2.0,WebOb==1.8.5,zope.deprecation==4.4.0,zope.interface==4.6.0
coverage runtests: PYTHONHASHSEED='2478745532'
coverage runtests: commands[0] | coverage combine
No data to combine
ERROR: InvocationError: '/Users/zupo/work/pyramid_openapi3/.tox/coverage/bin/coverage combine'
_________________________________________________________________ summary __________________________________________________________________
ERROR:   py37: commands failed
SKIPPED:  py38: InterpreterNotFound: python3.8
ERROR:   coverage: commands failed

@stevepiercy
Copy link
Member Author

pyramid_openapi3 (feature-add-tox)$ tox

That looks like you activated your virtual environment, then ran tox. Try deactivating and calling tox by its path instead, e.g., env/bin/tox.

@digitalresistor
Copy link
Member

@zupo you need to make sure that you have multiple Python interpreters installed. It looks like you don't have Python 3.8 installed.

Not sure what system you are on, but on MacOS/Linux the tool pyenv may come in handy, it will allow you to install multiple versions of the Python interpreters and add them to your $PATH so that too can use them.

I don't see anything in the Makefile you've built for this project that runs tests under multiple versions of Python. I also see that you are using pipenv, pipenv and a distributable package don't really go well together... and I would recommend moving away from it.

@stevepiercy
Copy link
Member Author

@bertjwregeer pyenv has 3.8-dev for pyenv install -l, no 3.8 yet. I did pyenv install 3.8-dev and added it to my ~/.python-version, but tox still cannot find 3.8. I also have this in my .bash_profile:

# https://github.com/bertjwregeer/dotfiles/blob/master/bash_profile#L65-L79
export TOX_PATH=""

function tox {
    PYENV=$(type -p pyenv)

    if [ ""$TOX_PATH == "" -a -x $PYENV ]; then
        for pyv in $(pyenv versions --bare); do
            TOX_PATH="$(pyenv prefix $pyv)/bin:$TOX_PATH"
        done
    fi

    export TOX_PATH=$TOX_PATH

    env PATH="$TOX_PATH:$PATH" tox $@
}

I could just wait for a final 3.8, but it would be nice to have it working locally now for this and other projects. Is there something I missed?

@stevepiercy
Copy link
Member Author

Meh, circleci is now failing from that last commit. It's a pipenv thing, and I have no idea how to resolve it other than kick pipenv to the curb.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

@zupo you need to make sure that you have multiple Python interpreters installed. It looks like you don't have Python 3.8 installed.

If I understood @stevepiercy correctly, 3.8 is an optional build? And tox fails for my 3.7 build as well. The error message doesn't tell me where/how to start debugging.

I am using pyenv already, have 3.6 and 3.7 installed.

I don't see anything in the Makefile you've built for this project that runs tests under multiple versions of Python.

That's correct, because I haven't. I have very limited experience with adding support for multiple Python versions so after spending a few days on it and failing, I decided I'll go with one Python version for the first few releases, to get at least something out. I'd definitely like to add support for multiple Python versions, as long as that doesn't break my workflow too much, and definitely does not break CI or make it very slow.

I also see that you are using pipenv, pipenv and a distributable package don't really go well together... and I would recommend moving away from it.

Why is that? Some of the biggest libraries, such as Requests are using it. I understand that it's probably because they are both from the same author, but the teams that keep maintaining both today seem happy with it?

This section explains why/how to use Pipfile for library development: https://docs.pipenv.org/en/latest/advanced/#pipfile-vs-setup-py

It's a pipenv thing, and I have no idea how to resolve it other than kick pipenv to the curb.

Let me take a look.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

pyramid_openapi3 (feature-add-tox)$ tox

That looks like you activated your virtual environment, then ran tox. Try deactivating and calling tox by its path instead, e.g., env/bin/tox.

Nope, I haven't. I never activate virtualenvs, I hate that part of virtualenvs, too implicit and too easy to forget and muck things up.

tox is a globally installed executable:

pyramid_openapi3 (feature-add-tox)$ which tox
/usr/local/bin/tox

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

Meh, circleci is now failing from that last commit. It's a pipenv thing, and I have no idea how to resolve it other than kick pipenv to the curb.

You have to apply my whole commit, including Pipfile.lock. Or run pipenv lock yourself and commit the changes. I've configured CI to fail the build if someone forgets to update versions of dependencies in Pipfile.lock after introducing changes to Pipfile.

@stevepiercy
Copy link
Member Author

If I understood @stevepiercy correctly, 3.8 is an optional build?

Correct. 3.8 is still in beta.

However, 3.8 will be released soon, and we ought to run tests against it. I don't know yet the secret mix of configuration items to get it to work.

Why is that?

There's a good discussion on pipenv, poetry, and general packaging and distributing in this Pyramid issue.

Pylons/pyramid#3270

Or run pipenv lock yourself and commit the changes.

Ah, OK. Thanks for explaining. Updated in bc648ca.

I reckon that we should include the instructions you provided in a CONTRIBUTING.md file or similar.

@stevepiercy
Copy link
Member Author

I deleted my .tox/ directory and ran it again to give a better comparison to see where @zupo's tox failed.

Steves-iMac:pyramid_openapi3 stevepiercy$ env/bin/tox
GLOB sdist-make: /Users/stevepiercy/projects/pyramid_openapi3/setup.py
py37 create: /Users/stevepiercy/projects/pyramid_openapi3/.tox/py37
py37 inst: /Users/stevepiercy/projects/pyramid_openapi3/.tox/.tmp/package/1/pyramid_openapi3-0.3.0.zip
py37 installed: apipkg==1.5,atomicwrites==1.3.0,attrs==19.1.0,coverage==4.5.4,execnet==1.6.1,hupper==1.8.1,importlib-metadata==0.19,jsonschema==3.0.2,lazy-object-proxy==1.4.1,more-itertools==7.2.0,openapi-core==0.11.0,openapi-spec-validator==0.2.8,packaging==19.1,PasteDeploy==2.0.1,plaster==1.0,plaster-pastedeploy==0.7,pluggy==0.12.0,py==1.8.0,pyparsing==2.4.2,pyramid==1.10.4,pyramid-openapi3==0.3.0,pyrsistent==0.15.4,pytest==5.0.1,pytest-cov==2.7.1,pytest-forked==1.0.2,pytest-xdist==1.29.0,PyYAML==5.1.2,six==1.12.0,strict-rfc3339==0.7,translationstring==1.3,venusian==1.2.0,wcwidth==0.1.7,WebOb==1.8.5,zipp==0.5.2,zope.deprecation==4.4.0,zope.interface==4.6.0
py37 run-test-pre: PYTHONHASHSEED='2981290996'
py37 run-test: commands[0] | pytest
<snip>

Aha! The game is afoot!

tox is installed in my virtual environment. I bet that you have an older version of tox that has an incompatibility or bug that was fixed later. Check your version against mine, which is the latest release.

$ env/bin/tox --version
3.13.2 imported from /Users/stevepiercy/projects/pyramid_openapi3/env/lib/python3.7/site-packages/tox/__init__.py

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

I reckon that we should include the instructions you provided in a CONTRIBUTING.md file or similar.

Makefile knows when Pipfile.lock needs to be re-locked based on "last modification timestamp" of Pipfile and .installed. So if you do make run, make tests or make lint, then you don't have to remember to run pipenv lock. make will do it for you.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

I bet that you have an older version of tox that has an incompatibility or bug that was fixed later.

Ah, too obvious, sorry for the noise. Upgrading to latest tox indeed fixed the problem, 3.7 build now passes on my local.

@stevepiercy
Copy link
Member Author

stevepiercy commented Aug 5, 2019

Aha, it was pyenv configuration in my .bash_profile. See step 3 here: https://github.com/pyenv/pyenv#basic-github-checkout

I used brew on macOS to install pyenv, but that step is still required for this install method. For some reason I had it commented out. ¯\_(ツ)_/¯

All my Pythons run now. YAY!

  py37: commands succeeded
  py38: commands succeeded
  coverage: commands succeeded
  congratulations :)

@stevepiercy
Copy link
Member Author

This got lost in the discussion.

I could also add flake8, black, build, or any other environment

@zupo do you want any of these, too? There are examples in webob, pyramid, and waitress that I can repurpose.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

This got lost in the discussion.

I could also add flake8, black, build, or any other environment

@zupo do you want any of these, too? There are examples in webob, pyramid, and waitress that I can repurpose.

make lint already runs flake8, black and more:

Or were you asking something else?

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

Installed 3.8-dev with pyenv on my local, all tox builds now pass:

  py37: commands succeeded
  py38: commands succeeded
  coverage: commands succeeded
  congratulations :)

@stevepiercy
Copy link
Member Author

Makefile knows when Pipfile.lock needs to be re-locked based on "last modification timestamp" of Pipfile and .installed. So if you do make run, make tests or make lint, then you don't have to remember to run pipenv lock. make will do it for you.

OK, I didn't see that explanation in CONTRIBUTING.md or README.md. I ran tests via tox only, so I would never have done make tests.

At some point maybe we could replace make with tox, and for now we could keep them both around for a while to evaluate and compare them, and bring them to feature parity.

When a makefile is used, but no Windows version makefile is provided, essentially we're telling Windows users that they will have to figure out how to do anything in make on their own. I don't know much about developing with Python on Windows, but just looking at two makefiles for Sphinx tells me that maintaining two makefiles would be a challenge.

tox works across platforms with a single configuration file.

As far as the timestamp comparison in the makefile, maybe we can bring that step into tox? Of course, that's assuming that you want to continue using the pipenv ecosystem in the first place.

@stevepiercy
Copy link
Member Author

Or were you asking something else?

Nope, essentially which features in make would you like to see ported to tox? I'd like to take a stab at them, or you can, too. Then compare and contrast the tox and make ecosystems on Windows. 😜

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

Or were you asking something else?

Nope, essentially which features in make would you like to see ported to tox? I'd like to take a stab at them, or you can, too. Then compare and contrast the tox and make ecosystems on Windows. 😜

I'm pretty much sold into using make instead of language specific runner. Any project I work on, be it Python, JavaScript, Elm, Go, in all of them make tests runs test, and make run starts up the project, etc. I don't have to remember "how doI run tests here in this project?". And I don't have to consult (outdated) docs on how to run tests. It's always make test. Besides, when new people want to contribute a minor fix, they don't have to install a new tool, like tox. All Unix-es come with make.

Lastly, I haven't touched Windows in over a decade. No-one in my company has a Windows machine and none of our software supports it. It sucks, but it's not realistic that I'll learn how to support Windows. I'll gladly let someone else take care of that.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

As far as the timestamp comparison in the makefile, maybe we can bring that step into tox? Of course, that's assuming that you want to continue using the pipenv ecosystem in the first place.

This stays the same even if we move to Poetry. I don't want to remember to update version pins when I change the dependency list. I want my tooling to do it.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

OK, I didn't see that explanation in CONTRIBUTING.md or README.md. I ran tests via tox only, so I would never have done make tests.

CONTRIBUTING.md is a copy-paste over from other Pylons projects. The instructions to run make tests are here https://github.com/pylons/pyramid_openapi3#running-tests

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

Just to be clear: I'm expressing (strong) personal opinions on tooling because I plan to be the main maintainer of this library for some time. And I want to make it as easy for me to do the work, so that I actually do it, and don't get depressed by losing limited free time on finding out how to use tools that I don't use daily otherwise. The moment other people start being more active (if it happens), my strong opinions go away. The plone.api package used to have a Makefile too, while I was still active on it, today they use tox.

@stevepiercy
Copy link
Member Author

Rebased on master from #26, in case you want to merge this for now. I can add other features to tox later.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

I'm fine with missing features, but I want some guarantee that tox.ini keeps working in the future.

One idea I have is to add it to CircleCI as a separate job. But I'm not sure I want to duplicate running all tests first with Makefile, then with tox. Any better approach?

@mmerickel
Copy link
Member

configure the makefile to run tox

@digitalresistor
Copy link
Member

digitalresistor commented Aug 5, 2019

I understand the appeal of a Makefile in each project, but tox really is somewhat of a standard in the Python community.

Having the Makefile call tox makes perfect sense. And then you always use tox locally too.

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

How would I do the following with "tox via makefile"?

  1. run only a subset of tests (make unit filter=foo at the moment, of which I am not a huge fan, but it is better than memorizing/copy-pasting long pytest commands)
  2. run flake8 only on changed files (make installs pre-commit hooks and then runs them on commits, pushes and make lint)

I'm not interested in the implementation at this moment, but how would tox commands that I would memorize look like?

@stevepiercy
Copy link
Member Author

stevepiercy commented Aug 5, 2019 via email

@zupo
Copy link
Collaborator

zupo commented Aug 5, 2019

Can make run tests, flake8, Black, etc., across multiple Python versions from a single configuration file?

Locally, nope, that's a limitation. I mean, it could be done, with quite some hackery, and that would be reimplementing tox in Makefile/bash syntax. Don't wanna go there.

That said, it's easy to run CI builds with different versions, using make.

@digitalresistor
Copy link
Member

@zupo as you can see here: https://github.com/Pylons/webob/blob/master/tox.ini#L23 you can pass arguments to tox to pass on to the program it ends up executing.

So to run a single test suite one can do:

tox -e py27,py37 tests/test_requests.py

For instance.

So if your makefile can pass along arguments to pytest or whatever other test runner you are using, you can pass along said arguments to tox instead.

As for running flake8 only on changed files... I question the utility thereof.

@zupo
Copy link
Collaborator

zupo commented Aug 6, 2019

tox -e py27,py37 tests/test_requests.py

That's my problem right there. I'll never be able to memorize that. It doesn't "read" well. So I'll refrain to copy/pasting form documentation, which will slow me down. Additionally, I'd have to spell out the entire path to the test file, however with make unit filter=foo that's a regex. It's gonna run any test that contains foo in the name.

As for running flake8 only on changed files... I question the utility thereof.

The use case is speed. I've come to realize that people I work with don't run linters if they are slow. "Meh, CI will run them" is the excuse. Which slows everyone down: them, because they have to wait for CI to report a stupid typo, and everyone else because the CI job queue is longer. It's better to run limited set of linters very fast locally, on every commit/push and discover most of problems earlier.

@stevepiercy
Copy link
Member Author

stevepiercy commented Aug 6, 2019 via email

@digitalresistor
Copy link
Member

make unit filter=foo that's a regex. It's gonna run any test that contains foo in the name.

How does it pass that regex filter to pytest? Because the same arguments can be massed from your makefile directly to tox.

on every commit/push and discover most of problems earlier.

If pre-commit is installed, it already does it by file... you don't need a Makefile for that.

@zupo
Copy link
Collaborator

zupo commented Aug 6, 2019

Because the same arguments can be massed from your makefile directly to tox.

That's what I'm hunting for. The whole pytest command is:

  • pytest pyramid_openapi3/ --cov=pyramid_openapi3 --cov-branch --cov-report html --cov-report xml:cov.xml --cov-report term-missing --cov-fail-under=100 -k . for make unit
  • pytest pyramid_openapi3/ -k foo for make unit filter=foo -> it's also about not doing coverage when filtering, to speed up tests

How can I do such an if-statement in tox?

If pre-commit is installed, it already does it by file... you don't need a Makefile for that.

make lint runs all hooks. pre-commit runs only the fastest hooks (so that people don't skip it with --no-verify)

@digitalresistor
Copy link
Member

Don't do that in tox but modify your Makefile to call tox...

Then you can change whatever arguments you pass tox.

Those of us that do development and run tox locally can do so then without issues. I'm willing to eat the extra cost of time to run flake8 across the whole project, rather than a subset, especially since I want to know if me removing a file would cause an import elsewhere to fail...

@zupo
Copy link
Collaborator

zupo commented Aug 7, 2019

Don't do that in tox but modify your Makefile to call tox...

Ah, of course, 🤦‍♂.

Tnx for the help!

@stevepiercy
Copy link
Member Author

@zupo, I'm fine with merging this PR for now.

I would open a new issue with a checklist of features to add to the tox configuration (flake8, black, build, isort, other?), and refer to this PR.

I'm busy through the end of this week, but I might have some downtime during the evenings to play around.

@zupo
Copy link
Collaborator

zupo commented Aug 7, 2019

@zupo, I'm fine with merging this PR for now.

I'm not, because there are no guards against me (who will not run tox regularly) breaking it by mistake. We need to add a job to CircleCI to confirm tox config is good.

@stevepiercy
Copy link
Member Author

I've added a build environment to tox, pulling bits and pieces from Pyramid and WebOb, in 7153169.

Note that with Python 3.7, we now use twine check dist/* instead of readme_renderer. See https://stackoverflow.com/questions/46682793/how-to-ensure-that-readme-rst-is-valid/46687472#46687472

@stevepiercy
Copy link
Member Author

I removed the black environment, moving it into the lint environment, and moved check-manifest and twine into lint as well.

Next up, CircleCI.

@stevepiercy
Copy link
Member Author

I found something from the author of tox-pyenv to get CircleCI run multiple versions of Python with pyenv. I might try it this weekend, but if someone else wants to have go, be my guest.

@stevepiercy
Copy link
Member Author

It feels like the wrong approach to get tox to run on multiple Python versions when using separate docker images. I got the idea from https://discuss.circleci.com/t/how-to-test-multiple-versions-by-triggering-jobs-with-a-shell-function/11305/15. However it seems that this is the recommended approach.

I think we want a single Docker image on which we can install multiple Pythons then just run tox. How do I find a Docker image that has tox, pyenv, tox-pyenv, and multiple Pythons installed?

@stevepiercy
Copy link
Member Author

Oh, and I have given up on trying to get this to work aside from the one approach that feels wrong to me in d653e6e.

@zupo
Copy link
Collaborator

zupo commented Aug 30, 2019

@stevepiercy: what if we have two docker images, each for one version of python, and then run tox for only that python version on that particular image?

@stevepiercy
Copy link
Member Author

@zupo that is exactly what I described in my previous comment, even though it feels wrong to me.

It's also too much work to maintain a Docker image that has all the dependencies (tox, pyenv, tox-pyenv, and multiple Pythons).

I haven't had the chance to clean up this PR and rebase it on master, but I think I can get it done over the US holiday weekend.

@zupo
Copy link
Collaborator

zupo commented Oct 26, 2019

Now that master has CircleCI configuration for running builds against Python 3.7 and 3.8, there is even less reason to add tox, so I'm gonna close this one for now.

I hope that by working on other Pylons projects in the future, I'll learn enough tox to start liking it. I tried learning it again today and again got frustrated :(.

@zupo zupo closed this Oct 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants