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

Enable coverage reporting via codecov #10

Merged
merged 1 commit into from Oct 30, 2018
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 30, 2018

No description provided.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

Problem with py37:

___________________________________ test_ok ____________________________________
    def test_ok():
        app = TestApp(CGIApplication({}, script='ok.cgi', path=[data_dir]))
>       res = app.get('')
tests/test_cgiapp.py:38: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.tox/py37-coverage/lib/python3.7/site-packages/paste/wsgilib.py:351: in raw_interactive
    app_iter = application(basic_environ, start_response)
.tox/py37-coverage/lib/python3.7/site-packages/paste/lint.py:170: in lint_app
    iterator = application(environ, start_response_wrapper)
.tox/py37-coverage/lib/python3.7/site-packages/paste/cgiapp.py:104: in __call__
    stderr=environ['wsgi.errors'])
.tox/py37-coverage/lib/python3.7/site-packages/paste/cgiapp.py:256: in proc_communicate
    stderr.write(data)
.tox/py37-coverage/lib/python3.7/site-packages/paste/lint.py:221: in write
    self.errors.write(s)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <paste.wsgilib.ErrorRaiser object at 0x7f3fb70ccbe0>
value = b'Error in atexit._run_exitfuncs:\nTraceback (most recent call last):\n  File "/home/travis/build/cdent/paste/.tox/py3...9, in exists\n    os.stat(path)\nTypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType\n'
    def write(self, value):
        if not value:
            return
        raise AssertionError(
>           "No errors should be written (got: %r)" % value)
E       AssertionError: No errors should be written (got: b'Error in atexit._run_exitfuncs:\nTraceback (most recent call last):\n  File "/home/travis/build/cdent/paste/.tox/py37-coverage/lib/python3.7/genericpath.py", line 19, in exists\n    os.stat(path)\nTypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType\n')

https://travis-ci.org/cdent/paste/jobs/448095432#L537

@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

Not sure where this exists(None) call is coming from.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

Fixed in coveragepy 4.5.2a1 (not released, ref: nedbat/coveragepy@58b210a).

@codecov-io
Copy link

codecov-io commented Oct 30, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@36e5b8b). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #10   +/-   ##
=========================================
  Coverage          ?   15.03%           
=========================================
  Files             ?      125           
  Lines             ?    14746           
  Branches          ?     2502           
=========================================
  Hits              ?     2217           
  Misses            ?    12492           
  Partials          ?       37
Flag Coverage Δ
#py27 14.68% <ø> (?)
#py35 14.71% <ø> (?)
#py36 14.71% <ø> (?)
#pypy 14.58% <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36e5b8b...a72dce4. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

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
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

.coveragerc Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
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}
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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).

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@cdent
Copy link
Collaborator

cdent commented Oct 30, 2018

I reckon we can go ahead and merge this, and work any other details later.

@cdent cdent merged commit 495ff97 into pasteorg:master Oct 30, 2018
@blueyed blueyed deleted the coverage branch October 30, 2018 12:19
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

3 participants