Skip to content

Use Coveralls #118

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

Merged
merged 4 commits into from
Feb 20, 2017
Merged

Use Coveralls #118

merged 4 commits into from
Feb 20, 2017

Conversation

jakirkham
Copy link
Member

Fixes https://github.com/alimanfoo/zarr/issues/117

Displays line numbers missed by the test coverage. Reports coverage to Coveralls. Needs to have Coveralls enabled on the repo to work.

@jakirkham
Copy link
Member Author

An example of the coverage report from Coveralls can be seen in this report.

Copy link
Member

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

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

Thank you for this, happy to merge with slight change to tox setup.

tox.ini Outdated
py27,py34,py35: nosetests -v zarr
py36: nosetests -v --with-coverage --cover-erase --cover-min-percentage=100 --cover-package=zarr --with-doctest --doctest-options=+NORMALIZE_WHITESPACE zarr
py27: nosetests -v --with-coverage --cover-erase --cover-package=zarr zarr
py34,py35,py36: nosetests -v --with-coverage --cover-erase --cover-package=zarr --with-doctest --doctest-options=+NORMALIZE_WHITESPACE zarr
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 modify this so that the doctests are only run under PY36? There are some dict ordering issues which mean getting consistent doctest runs across all PY3.X versions is hard, I'm happy if doctests are only run for PY36.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are the dict ordering issues related to Python's salting of hashes? If so, I would have thought setting PYTHONHASHSEED to a fixed value would solve this issue. Though a more robust solution would probably be to use OrderedDicts. That said, given you have set PYTHONHASHSEED and it was working before, I'm a bit curious to know if there is more to the story.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't understand. It's something new in PY36, works fine in both PY34 and PY35. I know dict implementation has changed in PY36, guessing that has something to do with it.

Instead of just gathering coverage statistics on Python 3.6, gather
coverage on all Pythons so that it can be reported. Also drop the 100%
coverage constraint as there are better ways to enforce that with
Coveralls.
@jakirkham
Copy link
Member Author

Alright, I think this is ready on my end. Is Coveralls already enabled on this repo?

@alimanfoo
Copy link
Member

Thanks. Coveralls is enabled, could you add the badge to the README:

.. image:: https://coveralls.io/repos/github/alimanfoo/zarr/badge.svg?branch=master
:target: https://coveralls.io/github/alimanfoo/zarr?branch=master

@jakirkham
Copy link
Member Author

Of course. Added.

@jakirkham
Copy link
Member Author

Should add that Coveralls by default is enabled to leave comments and/or statuses on PRs/commits. My recommendation would be to turn off comments, but leave on the status.

Also one can add a coverage threshold. So this would be a good way to enforce 100%. Plus it is very visible as it can show up in the PR status, which makes it easy to see that coverage dropped for a PR and to go back and figure out why coverage dropped.

@alimanfoo alimanfoo merged commit cb6fba6 into zarr-developers:master Feb 20, 2017
@alimanfoo
Copy link
Member

Awesome, thank you!

@jakirkham jakirkham deleted the use_coveralls branch February 21, 2017 00:33
@alimanfoo alimanfoo modified the milestone: v2.2 Feb 24, 2017
@alimanfoo alimanfoo mentioned this pull request Oct 24, 2017
4 tasks
@alimanfoo alimanfoo added maintenance Work needed by a maintainer release notes done Automatically applied to PRs which have release notes. labels Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Work needed by a maintainer release notes done Automatically applied to PRs which have release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants