-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Python 3 support #363
Python 3 support #363
Conversation
@locustio @heyman @HeyHugo @cgbystrom @Jahaja Can you take a quick look at this and consider merging it? All the legwork has been put in and it shouldn't require much time to review. |
i'm doing a code review now. I'm not a maintainer, but I've contributed to locust before and I watch the project. Do the current maintainers need any help? @pmdarrow fwiw, the tests all pass for me locally against your branch (Ubuntu 15.04). |
Will do. What do you think about adding 3.1, 3.2, and 3.3? Is that overkill / unnecessary? |
I have 3.2, and 3.3 also installed here... so I'll add those to tox.ini and run the tests again. Those versions seem reasonable to add to your setup.py and tox.ini... afaik, travis supports 3.2+, so we get CI on those versions for nothing. |
9927b86
to
72aff70
Compare
I updated |
Looks like Flask doesn't support 3.2 (and the tests fail) so I'm dropping support for it. |
fccf2d6
to
1e87d5e
Compare
I tried this PR with --no-web and there is no more reports visible in command line during the run. There is error in logs: ' AttributeError : 'StdOutWrapper' object has no attribute 'flush'. |
@locustio what about this? |
@ogabrielson I'll take a look. It's too bad the automated tests didn't catch that... |
Does anybody need a help to review this PR and then merge it to master? |
@deniscostadsc see #355 for the discussion about why PRs aren't being reviewed. |
@pmdarrow I'd consider this PR the highest priority aside from any major bug fixes that come up. I'm in the process of reviewing/testing, but I found a couple of minor issues that mean it is not yet ready for merging. I think what we should do is merge your branch into a branch in this repository that we would all have access to. |
@justiniso, could you list the issues? |
Yep, sorry! Stdout no-ops: 548ffe7 I updated gevent version to their officially supported version (rather than release candidate). Note that 3.5 is not supported and consequently we should not support it either (3.4 is the latest supported). Even with 3.4, I'm getting this error when trying to start up the slave:
|
gevent 1.1.1 supports 3.5, even having wheels for windows 👍 |
@justiniso Great! Do you know of any other existing issues before this could be merged? Not at Pycon unfortunately. Next year hopefully :). |
On travis.yml just version 3.5 of Python is declared But maybe, tox is dealing with this in the correct way, creating all venvs. But you won't be able to see it on Travis UI. Why not create a matrix on Travis instead of using tox? Do you want to run all possibilities locally? |
Ah, yes, tox takes care of picking the correct version. And I guess the ability to easily run tests for all versions is convenient. |
@deniscostadsc you are correct, tox (running under Python 3.5) is creating all the venvs for each version of Python we want to support. And you are also correct, it lets devs test against all supported versions of Python locally. |
The setuptools / unittest test runner is very flaky on Python 3. This also encourages a single way to run tests: unit2 discover (called by tox or run directly).
It’s redundant now that pyzmq is defined in `install_requires`.
@justiniso I cherry-picked your commit that explicitly declares pyzmq. Are people still experiencing issues with gevent or was that resolved with gevent 1.1.1? |
@pmdarrow if you're referring to my issue with gevent, that is fixed by the commit you just cherry-picked. If there are other issues, I'm not aware of them. My testing has been on linux and osx (unfortunately don't have a Windows machine to test with). As far as I'm aware, there are no other issues preventing merge. |
@justiniso Ok. What should we do about releasing a new version of Locust? I'm guessing none of us "new" contributors have access to https://pypi.python.org/pypi/locustio? |
Right, @heyman would have to do that. I would go with versioning as a pre-release 0.8a1 to flag as a potentially unstable version. It would be nice to let it soak with early adopters and let them report any issues with integrations, etc. before fully rolling it out. |
I'll merge this and then create another PR with the release changes. |
🙌 |
🙌 |
Woho, great work! It sounds like a good idea to do a pre-release alpha first. |
👏 Awesome guys. Looking forward to using this in py3 |
nice!
|
Great work. Would be great to get this on pypi as it would help the automation process quite a bit. |
@ryankennedyio The latest master has been released to PyPI as a pre release, and can be installed by specifying the version explicitly ( |
When will it come the main release rather than pre-release? |
Added support for Python 3 and running tests against all supported versions of Python with tox.
Borrowed heavily from #328 but cleaned things up & simplified some changes in the hopes that it will be more likely to be merged.