-
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
Enable coverage reporting via codecov #10
Conversation
Problem with py37:
|
Not sure where this |
Fixed in coveragepy 4.5.2a1 (not released, ref: nedbat/coveragepy@58b210a). |
Codecov Report
@@ Coverage Diff @@
## master #10 +/- ##
=========================================
Coverage ? 15.03%
=========================================
Files ? 125
Lines ? 14746
Branches ? 2502
=========================================
Hits ? 2217
Misses ? 12492
Partials ? 37
Continue to review full report at Codecov.
|
Should be good now, but should get squash-merged, please. |
tox.ini
Outdated
deps = | ||
pytest | ||
pytest-cov | ||
# 4.5.2a1 for py37 fix | ||
coverage: https://github.com/nedbat/coveragepy/archive/58b210ad8998a9270f4ee6ff0c9054785f579b43.zip |
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.
This is not required anymore when using the "path" already.
The error was due to paste/
being used, but with tox it would use the package from .tox/…
anyway.
See nedbat/coveragepy#268.
The proper fix here would be to use a "src" based layout, i.e. move "paste" to "src/paste", but not sure if it is worth it.
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.
Given the outcome/review I will adjust this here.
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.
So what about this?
I would say to go back to just "coverage" here.
Will push this already.
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.
Yeah, go ahead and fix that in whatever way is right.
coverage: https://github.com/nedbat/coveragepy/archive/58b210ad8998a9270f4ee6ff0c9054785f579b43.zip | ||
coverage: pytest-cov | ||
setenv = | ||
coverage: PYTEST_ADDOPTS=--cov --cov-report=term-missing | ||
commands = | ||
py.test {posargs} |
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.
Could you add a human-oriented coverage tox env that allows someone to check coverage before pushing? Something like:
- rm .coverage and rm -r cover
- ran the tests with coverage turned on
- coverage html -d cover
Doesn't have to be in the pull request, could be a different one.
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.
Yes, could be useful, although tox -e py37-coverage
should be enough and then allows you to use your preferred reporting, e.g. I would rather use coverage report …
instead of the html report.
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.
..and pytest-cov does the reporting already to the terminal btw.
In general a Makefile might be better suited though, since otherwise with a separate env you get a duplicated one:
cover: PY=py37
cover:
tox -e $(PY)-coverage
.tox/$(PY)-coverage/bin/coverage html -d $@
Anyway, feedback via PRs is better in general though (since it covers all envs).
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.
Anyway, feedback via PRs is better in general though (since it covers all envs).
The thing I'm trying to encourage is people checking their coverage before they ever push because when someone finally makes a pull request they should think it is ready. Which tooling we have to encourage that doesn't really matter to me, just so long as such tooling is available.
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.
So I guess running e.g. tox -e py37-coverage
alone would be good then?
If you want to add it to docs, you can also mention to run coverage html -d cover
afterwards for an HTML report I guess.
I reckon we can go ahead and merge this, and work any other details later. |
No description provided.