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

[RFC] add tox.ini / support for pytest #123

Merged
merged 1 commit into from May 20, 2017
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 22, 2016

This is rudimentary, but makes it a lot easier to run the tests.

Please consider adopting it and mention it in the docs.
It could also be used on Travis.

@coveralls
Copy link

coveralls commented Nov 22, 2016

Coverage Status

Coverage remained the same at 99.63% when pulling 730edf7 on blueyed:tox into 7c92536 on djangonauts:master.

@nemesifier
Copy link
Member

@blueyed excellent suggestion. I have always been lazy on this subject. I would like to go ahead on this, could you rewrite the Test section of the README and .travis.yml too?

Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

welcome changes!!

@blueyed
Copy link
Contributor Author

blueyed commented Nov 22, 2016

I've hoped that you would pick it up from here.
I can do it, of course - but not soonish.. ;)

@nemesifier
Copy link
Member

"Now is better than never, although never is probably better than right now" (cit.)

@blueyed
Copy link
Contributor Author

blueyed commented Nov 28, 2016

never is probably better than right now

I am taking this. Feel free to adopt it, of course.

@blueyed blueyed closed this Nov 28, 2016
@nemesifier
Copy link
Member

I don't understand why you closed it. Can it stay open as a reminder for when me or other contributors will have time to look into it?

@nemesifier nemesifier reopened this Nov 29, 2016
@coveralls
Copy link

coveralls commented Nov 29, 2016

Coverage Status

Coverage remained the same at 99.63% when pulling 730edf7 on blueyed:tox into 7c92536 on djangonauts:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.63% when pulling 730edf7 on blueyed:tox into 7c92536 on djangonauts:master.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 29, 2016

Can it stay open as a reminder for when me or other contributors will have time to look into it?

Sure. I understood it wrongly then in the beginning - and came across it when going through my open PRs. Sorry.

@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2017

I've updated it: it now allows you to use pytest/pytest-django through tox (used in one matrix entry on Travis).

I'll need to revisit myself, but would appreciate any comments/review on it already.

@blueyed blueyed changed the title Add tox.ini [RFC] add tox.ini / support for pytest May 15, 2017
@nemesifier
Copy link
Member

@blueyed great!

Could you squash the commits?

One question, why adding pytest to the build? Wouldn't that slow down the build with no practical advantage?

README.rst Outdated
@@ -606,6 +610,36 @@ These steps should do the trick:
- run ``python manage.py collectstatic``
- run ``python manage.py runserver``


Copy link
Member

Choose a reason for hiding this comment

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

could you remove this extra blank line?

setup.cfg Outdated
@@ -1,2 +1,6 @@
[bdist_wheel]
universal=1

[tool:pytest]
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 ignorant of pytest, what's this section for?

@blueyed blueyed mentioned this pull request May 15, 2017
@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2017

Could you squash the commits?

Sure, please never merge fixup! commits.. :)

One question, why adding pytest to the build? Wouldn't that slow down the build with no practical advantage?

Only for demo.. we could use pytest for only one / a few in the end.

@nemesifier
Copy link
Member

@blueyed can we keep it simple and avoid adding unneeded dependencies to the build please?

@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2017

What's the problem with having one build also pip-install pytest/pytest-django?
It is more comfortable to have pytest as a test runner, therefore I added it to tox / config for it, but then I think it should also get tested/used on CI to catch any problems with it.

@nemesifier
Copy link
Member

I don't want to use more resources than needed and I don't want to slow down the build unnecessarily. We are using a free service after all.

Developers can use pytest offline in their own dev environment.

@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2017

Do you really think an additional pip install pytest pytest-django about once per month is a problem for Travis?! (https://travis-ci.org/djangonauts/django-rest-framework-gis/pull_requests)

But given that, please comment on the build matrix I came up with otherwise - there are probably things to strip there then?!

@blueyed
Copy link
Contributor Author

blueyed commented May 15, 2017

Old (9 builds): https://travis-ci.org/djangonauts/django-rest-framework-gis/builds/229053544
New (11, but 10 with -pytest (re)moved): https://travis-ci.org/djangonauts/django-rest-framework-gis/builds/232514268; but covers Django 1.11.

To be clear: my idea was to use py36-dj110-pytest instead of py36-dj110 - which would result in 10 builds.

Also adds Django 1.11 to Travis, closes openwisp#132.
@blueyed
Copy link
Contributor Author

blueyed commented May 16, 2017

Squashed, and removed pytest from Travis. Also the config for it - still works through Tox.

@nemesifier
Copy link
Member

@blueyed thanks a lot 👍!

@blueyed blueyed deleted the tox branch May 20, 2017 15:58
@blueyed
Copy link
Contributor Author

blueyed commented May 20, 2017

@nemesisdesign
Thanks for merging it! :)

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

4 participants