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

#128 Add Tox #130

Merged
merged 4 commits into from Dec 27, 2015
Merged

#128 Add Tox #130

merged 4 commits into from Dec 27, 2015

Conversation

benred42
Copy link

@benred42 benred42 commented Nov 5, 2015

Fixes #128. Adds Tox testing management with TravisCI support. I did not add Tox to any of your requirements files but opted to add documentation on how to install and use Tox to tests/README.rst.

@benred42
Copy link
Author

benred42 commented Nov 5, 2015

The flake8 tests are currently failing which is causing the build to fail, but I figured that fixing pep8 warnings was outside the scope of this PR.

@benred42 benred42 changed the title 128 add tox #128 Add Tox Nov 5, 2015
basepython = python3.4
commands = coverage run tests/runtests.py
coverage report -m --fail-under 50
deps = -rrequirements-tests.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Coverage shouldn't need a test env

Copy link
Author

Choose a reason for hiding this comment

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

You're right, it doesn't need one. Putting it in a test env is mostly a matter of convenience. This way when you run Tox locally on a branch it will give you back the coverage results as well without having to run coverage separately and it will still run coverage on Travis.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think coverage should be included as base test command. It is in requirerements-tests.txt, so we could use it easily in tox.

@ZuluPro
Copy link
Contributor

ZuluPro commented Nov 6, 2015

@benred42: Thanks for all, we really appreciate contributions !
I wanted to make it since a looooong time, but you did.

I thought a tox.ini like this one: https://github.com/ZuluPro/myblog/blob/master/tox.ini
Plus env:

What do you think about ?

@benred42
Copy link
Author

Sure, that looks good to me. I can put most of that together. I'm not familiar with Prospector, but it doesn't look difficult to use and certainly looks useful so it'll be nice to give it a try!

@benred42
Copy link
Author

@ZuluPro: Do my latest edits look more in line with what you were thinking for the project?

@ZuluPro
Copy link
Contributor

ZuluPro commented Nov 23, 2015

Yep that's great, but I think something like: https://github.com/ZuluPro/myblog/blob/master/tox.ini
(without DB dimension)

Can you use {posargs:coverage run {toxinidir}/myblog/manage.py test} as command ?
Without {posargs}, coveralls won't have your report.

And coverage --fail-under is done by Coveralls.

It would be great to have a Travis' job by Tox environment. If you don't want to do it, I'll make after your PR.

@benred42
Copy link
Author

You don't actually need to use the {posargs} form for Coveralls to get the coverage report, as far as I know. There was an issue, however, where we were getting a coverage version conflict between Tox (using coverage3.7.1) and Coveralls (using coverage4.0.2) that was resulting in no data being sent to Coveralls. Pinning coverage to 3.7.1 in .travis.yml like I've done fixes that, although a better solution might be to update requirements-tests.txt to ask for coverage>=3.7.1 instead of coverage<4. What do you think?

@ZuluPro
Copy link
Contributor

ZuluPro commented Nov 24, 2015

Ok I understand and

  • I really want {posargs} usage because it really helps to debug manually.
  • coverage>4 doesn't support Python 3.2 so I prefer to use coverage<4 for compatibility, it doesn't seem to cause trouble. Coveralls uses coverage>=3.6 so it's compatible.

@benred42
Copy link
Author

Ah, gotcha. So, I guess just leaving the line in Travis pinning coverage to 3.7.1 should be fine then?

@ZuluPro
Copy link
Contributor

ZuluPro commented Nov 24, 2015

Yep remove this line, decrease the number of commit.
I think I'll take you PR and make some little modifications.

It will be more simple.

@benred42
Copy link
Author

Sure, that's fine by me. Just be aware that removing pip install coverage==3.7.1 from .travis.yml will result in no data being sent to Coveralls.

@ZuluPro
Copy link
Contributor

ZuluPro commented Dec 17, 2015

@benred42
Thanks a lot for all,
I'm taking your branch and made some modifications.
Your patch really helps me, thanks again.

@benred42
Copy link
Author

You're very welcome! Glad to help.

@ZuluPro ZuluPro merged commit fcdb5f4 into jazzband:master Dec 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants