Skip to content

Travis: report coverage with all builds #2800

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 2 commits into from
Aug 30, 2018
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Sep 26, 2017

https://codecov.io/gh/pytest-dev/pytest/tree/coverage

TODO:

@blueyed blueyed changed the title Travis: report coverage with all builds, include codecov [WIP/RFC] Travis: report coverage with all builds, include codecov Sep 27, 2017
@blueyed
Copy link
Contributor Author

blueyed commented Sep 27, 2017

It seems to be problematic with py27/py26 (https://travis-ci.org/pytest-dev/pytest/builds/280216101).
I've triggered a full build now again.

Is this something we want to have in general?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

I'm in favor of improving our coverage testing. 👍

Your approach also seems to be possible to implement on AppVeyor.

Ultimately we should strive to obtain 100% coverage, even if by explicitly marking parts of the code which are not currently tested (like java specific parts). Even if we "cheat", we gain the benefit of demanding that new code from now on has 100% coverage.

.travis.yml Outdated
@@ -5,11 +5,15 @@ python:
# command to install dependencies
install:
- pip install --upgrade --pre tox
- |
pip install tox
Copy link
Member

Choose a reason for hiding this comment

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

tox is already installed in the line above, so this is not really needed

.travis.yml Outdated
@@ -5,11 +5,15 @@ python:
# command to install dependencies
install:
- pip install --upgrade --pre tox
- |
pip install tox
if [[ "${TOXENV#py}" != "$TOXENV" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Pardon my lack of bash-fu, what does this expression inside if does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TOXENV starts with "py".

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, and what does "${TOXENV#py}" do? (I tried googling it but it wasn't entirely obvious what to search for).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It substitutes "py" at the beginning.
Yes, shell programming can be ugly. See "Parameter Expansion" in bash's man page.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

tox.ini Outdated
commands =
pytest -ra test_pdb.py test_terminal.py test_unittest.py
{env:TOX_COVERAGE_RUN:} pytest -ra test_pdb.py test_terminal.py test_unittest.py
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the Travis build, py27/py36-trial seem to be missing this configuration

.coveragerc Outdated
@@ -1,4 +1,5 @@
[run]
source = _pytest
omit =
# standlonetemplate is read dynamically and tested by test_genscript
Copy link
Member

Choose a reason for hiding this comment

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

Oh this does not exist anymore and can be removed. 😁

Copy link
Contributor Author

@blueyed blueyed Sep 27, 2017

Choose a reason for hiding this comment

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

standlonetemplate? Fixed (not pushed).

Apart from that I think we should also include testing (the tests themselves). Anything else?

Copy link
Member

Choose a reason for hiding this comment

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

"standlonetemplate" was used by the removed genscript feature.

Apart from that I think we should also include testing (the tests themselves). Anything else?

Not sure, then xfail tests which are never meant to run because they reproduce issues not yet fixed will drop the coverage. In my head "coverage" does not include the tests. What's your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think unexpected missing coverage is a good sign of something being wrong.
Those xfail tests should be marked with "pragma: no cover" then.

.travis.yml Outdated
- |
pip install tox
if [[ "${TOXENV#py}" != "$TOXENV" ]]; then
export TOX_COVERAGE_RUN="coverage run -m"
Copy link
Member

Choose a reason for hiding this comment

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

At first glance I thought TOX_EXTRA_DEPS was a feature of tox itself. Perhaps we should add a PYTEST_ prefix to avoid confusion? While at it, perhaps there is a way to add extra dependencies using some tox feature?

Copy link
Member

Choose a reason for hiding this comment

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

Just remembered that we used _PYTEST_SETUP_SKIP_PLUGGY_DEP in setup.py for this type of "setup/buid only flag used internally".

_PYTEST_TOX_COVERAGE_RUN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will rename it to _PYTEST_TOX_COVERAGE_RUN and _PYTEST_TOX_EXTRA_DEPS then?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, thanks

@blueyed
Copy link
Contributor Author

blueyed commented Sep 27, 2017

Failures on py2, pypy and py33 are all:

=========================== short test summary info ============================
FAIL testing/code/test_excinfo.py::TestTraceback_f_g_h::()::test_traceback_cut_excludepath
FAIL testing/python/collect.py::TestModule::()::test_show_traceback_import_error[0]
FAIL testing/python/collect.py::TestModule::()::test_show_traceback_import_error[1]
FAIL testing/python/collect.py::TestTracebackCutting::()::test_traceback_argsetup

e.g. https://travis-ci.org/pytest-dev/pytest/jobs/280479131#L599

Any idea? (likely related to the assertions there not being true when run on coverage likely?!)

@blueyed
Copy link
Contributor Author

blueyed commented Sep 27, 2017

@nicoddemus
Can you enable the codecov bot user on https://codecov.io/gh/pytest-dev/pytest?

@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@pytest-dev pytest-dev deleted a comment from coveralls Sep 27, 2017
@nicoddemus
Copy link
Member

Can you enable the codecov bot user on https://codecov.io/gh/pytest-dev/pytest?

Done! If we will use codecov we should disable coveralls (after this is merged)

@blueyed
Copy link
Contributor Author

blueyed commented Sep 27, 2017

The problem with test_traceback_cut_excludepath is that the traceback entry looks different when run with -m, also when using python -m pytest (and we use coverage run -m).

<TracebackEntry _pytest/python_api.py:580> vs <TracebackEntry /home/user/Vcs/pytest/.tox/py27/lib/python2.7/site-packages/_pytest/python_api.py:580>.
Is that something that should get fixed for excinfo.traceback?
Looks like it uncovered an actual issue?!

@blueyed
Copy link
Contributor Author

blueyed commented Sep 27, 2017

This works around it:

diff --git i/testing/code/test_excinfo.py w/testing/code/test_excinfo.py
index f8f8a036..cc512a2d 100644
--- i/testing/code/test_excinfo.py
+++ w/testing/code/test_excinfo.py
@@ -162,9 +162,12 @@ def test_traceback_cut(self):
         assert len(newtraceback) == 1
 
     def test_traceback_cut_excludepath(self, testdir):
+        ptf = py.path.local(pytest.__file__)
+        if not ptf.exists():
+            pytest.skip('pytest.__file__ does not point to an actual file.')
+        basedir = ptf.dirpath()
         p = testdir.makepyfile("def f(): raise ValueError")
         excinfo = pytest.raises(ValueError, "p.pyimport().f()")
-        basedir = py.path.local(pytest.__file__).dirpath()
         newtraceback = excinfo.traceback.cut(excludepath=basedir)
         for x in newtraceback:
             if hasattr(x, 'path'):

Looks like an issue with Python 2 where pytest.__file__ is not correct in this case.

@nicoddemus
Copy link
Member

I gotta say I like codecov's report better. 😄

@blueyed blueyed changed the title [WIP/RFC] Travis: report coverage with all builds, include codecov [RFC] Travis: report coverage with all builds, include codecov Oct 5, 2017
@blueyed blueyed requested a review from a team October 5, 2017 00:53
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

There's some environments failing, could you take a look at those?

.travis.yml Outdated
pip install codecov coveralls
coverage xml
coverage report -m
coveralls
Copy link
Member

Choose a reason for hiding this comment

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

Should we drop coveralls if we are migrating to codecov?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is good to keep it for comparison, at least for a while.

tox.ini Outdated
commands =
pytest -n3 -ra --runpytest=subprocess {posargs:testing}
{env:TOX_COVERAGE_RUN:} pytest -n3 -ra --runpytest=subprocess {posargs:testing}
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 seeing this warning from tox:

WARNING:test command found but not installed in testenv
  cmd: /usr/bin/env
  env: /home/travis/build/pytest-dev/pytest/.tox/py27

Do we have to whitelist env?

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we just use pytest-cov instead?

Copy link
Member

Choose a reason for hiding this comment

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

pytest-cov does incomplete reports due to late plugin startup, its a longstanding pytest issue

tox.ini Outdated
coverage run --source=_pytest -m pytest testing
coverage report -m
coveralls
{env:TOX_COVERAGE_RUN:} {envpython} create_executable.py
Copy link
Member

Choose a reason for hiding this comment

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

We can drop coverage from this env I believe

tox.ini Outdated
@@ -19,29 +19,32 @@ envlist =
docs

[testenv]
commands = pytest --lsof -ra {posargs:testing}
commands = {env:TOX_COVERAGE_RUN:} pytest --lsof -ra {posargs:testing}
Copy link
Member

Choose a reason for hiding this comment

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

can we just introduce a coverage factor for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RonnyPfannschmidt
There is no way to have a command by default and then another for a factor, is there? (where the command comes not from the default testenv).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do it via setenv:

[testenv]
commands = {env:_TOX_COVERAGE_RUN:} pytest --lsof -ra {posargs:testing}
    coverage: coverage report -m --skip-covered
setenv =
    coverage: _TOX_COVERAGE_RUN=env COVERAGE_FILE={toxinidir}/.coverage coverage run -a -m
whitelist_externals =
    coverage: env

.travis.yml Outdated
@@ -5,11 +5,14 @@ python:
# command to install dependencies
install:
- pip install --upgrade --pre tox
- |
if [[ "${TOXENV#py}" != "$TOXENV" ]]; then
export TOX_COVERAGE_RUN="env COVERAGE_FILE=$PWD/.coverage coverage run -a -m"
Copy link
Member

Choose a reason for hiding this comment

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

at frist glance this "breaks" at least for the xdist envs

tox.ini Outdated
passenv = USER USERNAME
deps =
hypothesis>=3.5.2
nose
mock
requests
{env:TOX_EXTRA_DEPS:}
Copy link
Member

Choose a reason for hiding this comment

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

dito

@blueyed
Copy link
Contributor Author

blueyed commented Oct 12, 2017

@nicoddemus
See #2800 (comment).

@RonnyPfannschmidt
Copy link
Member

instead of falling out for pytest in some cases

lets use _pytest instead - its not being used as module

@nicoddemus
Copy link
Member

lets use _pytest instead - its not being used as module

You mean in the production code or the test?

@RonnyPfannschmidt
Copy link
Member

in testing/code/test_excinfo.py

coverage report -m
codecov --required -X gcov pycov search -f coverage.xml --flags ${TOXENV//-/ }

# Coveralls does not support merged reports.
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can drop coveralls then. Or would you rather do it in another PR @blueyed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought to keep it for comparison.

Copy link
Member

Choose a reason for hiding this comment

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

Oh OK. But I think we should remove it after a while to avoid confusion. 👍

appveyor.yml Outdated
@@ -1,53 +0,0 @@
environment:
Copy link
Member

Choose a reason for hiding this comment

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

I assume you removed AppVeyor so the builds are faster, please remember to bring it back before merging.

tox.ini Outdated
{env:_PYTEST_TOX_COVERAGE_RUN:} pytest --lsof -ra {env:_PYTEST_TEST_OPTS:} {posargs:testing}
coverage: coverage report -m --skip-covered
setenv =
coverage: _PYTEST_TOX_COVERAGE_RUN=coverage run -a -m
Copy link
Member

Choose a reason for hiding this comment

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

Why do we handle _PYTEST_TOX_COVERAGE_RUN and _PYTEST_TOX_EXTRA_DEP both here and in the travis file?

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 meant to allow for tox -e py36-coverage.
I've thought about/tried using only -coverage, but it is not trivial/feasible due to the many different testenvs.

Copy link
Member

Choose a reason for hiding this comment

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

I see, OK, thanks

@blueyed blueyed changed the title [RFC] Travis: report coverage with all builds, include codecov Travis: report coverage with all builds Aug 28, 2018
@blueyed
Copy link
Contributor Author

blueyed commented Aug 28, 2018

Squashed (old HEAD: 78d2c4d).
I think this is good from my side for now.

@blueyed
Copy link
Contributor Author

blueyed commented Aug 28, 2018

Meh.. AppVeyor will fail - still using TOXENV=coveralls..

I guess the same mechanism should be used with AppVeyor then? Basically the before_script and after_success from Travis would need to be ported then.
@nicoddemus can you do it? It should also add an extra flag with codecov then, "win" or "windows".

@nicoddemus
Copy link
Member

@blueyed sure I can give it a try. For now I suggest to comment out the TOXENV=coveralls in AppVeyor so we can get this merged.

@blueyed blueyed changed the base branch from features to master August 28, 2018 21:11
@pytest-dev pytest-dev deleted a comment from codecov bot Aug 28, 2018
@pytest-dev pytest-dev deleted a comment from codecov bot Aug 28, 2018
@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #2800 into master will increase coverage by 4.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
+ Coverage   92.18%   96.24%   +4.05%     
==========================================
  Files          52      108      +56     
  Lines       10009    23353   +13344     
==========================================
+ Hits         9227    22475   +13248     
- Misses        782      878      +96
Flag Coverage Δ
#doctesting 31.93% <ø> (?)
#nobyte 93.5% <ø> (?)
#numpy 31.48% <ø> (?)
#pexpect 53.58% <ø> (?)
#pluggymaster 95.09% <ø> (?)
#py27 94.61% <ø> (?)
#py34 93.9% <ø> (?)
#py35 93.9% <ø> (?)
#py36 94.82% <ø> (?)
#py37 94.07% <ø> (?)
#trial 34.15% <ø> (?)
#xdist 94.64% <ø> (?)
Impacted Files Coverage Δ
testing/test_warnings.py 98.43% <0%> (ø)
testing/test_pastebin.py 100% <0%> (ø)
testing/python/fixture.py 99.23% <0%> (ø)
testing/test_terminal.py 99.38% <0%> (ø)
testing/python/setup_plan.py 100% <0%> (ø)
testing/test_pdb.py 53.48% <0%> (ø)
testing/test_helpconfig.py 100% <0%> (ø)
testing/logging/test_fixture.py 100% <0%> (ø)
testing/test_modimport.py 90% <0%> (ø)
testing/test_assertion.py 98.21% <0%> (ø)
... and 69 more

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 d76fb83...417516c. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Aug 28, 2018

Rebased it on master (where the other ci improvements are already), and re-added coveralls for AppVeyor. Since it also runs codecov there it should be good for Windows for now.

@nicoddemus
Copy link
Member

Since it also runs codecov there

You mean coveralls right?

@blueyed
Copy link
Contributor Author

blueyed commented Aug 28, 2018

Both, since 4d19b94.

@blueyed
Copy link
Contributor Author

blueyed commented Aug 28, 2018

Fixed linting, squashed the keeping of coveralls for AppVeyor:

diff --git a/.travis.yml b/.travis.yml
index 9279334e..2c9deb3e 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -60,7 +60,7 @@ jobs:
       env: TOXENV=py27
     - env: TOXENV=py34
     - env: TOXENV=py36
-    - env: TOXENV=linting
+    - env: TOXENV=linting PYTEST_NO_COVERAGE=1

     - stage: deploy
       python: '3.6'

@blueyed

This comment has been minimized.

- Skips pypy for coverage, reports only py37 to coveralls
- tox: allow for TOXENV=py37-coverage
- tracks coverage in subprocesses, using coverage-enable-subprocess, and
  parallel=1
- removes usedevelop with doctesting to match `--source` being used with
  coverage
- keep coveralls for now, used with AppVeyor
@blueyed blueyed force-pushed the coverage branch 2 times, most recently from 9d883e1 to 794fd05 Compare August 29, 2018 20:40
doctesting: remove changedir

With coverage 5 we could use COVERAGE_RCFILE to make it find the
.coveragerc, or we could add `--rcfile` to _PYTEST_TOX_COVERAGE_RUN, but
I've thought that this should not be the job that has to test if
`--pyargs` actually works.
@blueyed
Copy link
Contributor Author

blueyed commented Aug 29, 2018

doctesting is working now (https://travis-ci.org/pytest-dev/pytest/jobs/422263747#L677), but I've changed the tox testenv quite a bit.

@nicoddemus nicoddemus merged commit f0e852b into pytest-dev:master Aug 30, 2018
@nicoddemus
Copy link
Member

Thanks a ton @blueyed!

@blueyed blueyed deleted the coverage branch August 31, 2018 00:25
@blueyed
Copy link
Contributor Author

blueyed commented Aug 31, 2018

Yay! :)

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.

5 participants