-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
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.
|
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"]}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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. |
AFAIK, not yet for everyone, they're in close beta for Windows builds. |
Tox fails for me like so:
|
That looks like you activated your virtual environment, then ran tox. Try deactivating and calling tox by its path instead, e.g., |
@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 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. |
@bertjwregeer pyenv has 3.8-dev for
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? |
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. |
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
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.
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
Let me take a look. |
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.
|
You have to apply my whole commit, including |
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.
There's a good discussion on pipenv, poetry, and general packaging and distributing in this Pyramid issue.
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. |
I deleted my
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.
|
|
Ah, too obvious, sorry for the noise. Upgrading to latest tox indeed fixed the problem, 3.7 build now passes on my local. |
Aha, it was pyenv configuration in my 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!
|
This got lost in the discussion.
@zupo do you want any of these, too? There are examples in webob, pyramid, and waitress that I can repurpose. |
Or were you asking something else? |
Installed 3.8-dev with pyenv on my local, all tox builds now pass:
|
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 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
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. |
Nope, essentially which features in |
I'm pretty much sold into using 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. |
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. |
CONTRIBUTING.md is a copy-paste over from other Pylons projects. The instructions to run |
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 |
bc648ca
to
4cc9a23
Compare
Rebased on master from #26, in case you want to merge this for now. I can add other features to tox later. |
I'm fine with missing features, but I want some guarantee that 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? |
configure the makefile to run tox |
I understand the appeal of a Makefile in each project, but Having the |
How would I do the following with "tox via makefile"?
I'm not interested in the implementation at this moment, but how would tox commands that I would memorize look like? |
I think there is broader support for tox than make from both the Python and Pylons communities.
Can make run tests, flake8, Black, etc., across multiple Python versions from a single configuration file?
If I can figure out tox, then anyone can!
…--steve
On Aug 5, 2019 at 1:20 PM, Nejc Zupan ***@***.***> wrote:
I'm fine with missing features, but I want some guarantee that tox.ini keeps working in the future.
|
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. |
@zupo as you can see here: https://github.com/Pylons/webob/blob/master/tox.ini#L23 you can pass arguments to So to run a single test suite one can do:
For instance. So if your makefile can pass along arguments to As for running |
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
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. |
Can't you drop the Python versions against which you want to run tests, and use pytest args?
tox tests/test_module.py
It seems similar to make.
…--steve
On Aug 5, 2019 at 10:00 PM, Nejc Zupan ***@***.***> wrote:
>
>
> 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 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 linter very fast locally, on every commit/push and discover most of problems earlier.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
How does it pass that regex filter to pytest? Because the same arguments can be massed from your makefile directly to tox.
If pre-commit is installed, it already does it by file... you don't need a Makefile for that. |
That's what I'm hunting for. The whole pytest command is:
How can I do such an if-statement in tox?
|
Don't do that in Then you can change whatever arguments you pass Those of us that do development and run |
Ah, of course, 🤦♂. Tnx for the help! |
@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. |
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. |
I've added a Note that with Python 3.7, we now use |
I removed the black environment, moving it into the lint environment, and moved check-manifest and twine into lint as well. Next up, CircleCI. |
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. |
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 |
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. |
@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? |
@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. |
Now that 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 :(. |
See #28