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

Fix coverage story and add coveralls.io. #85

Merged
merged 9 commits into from
Apr 23, 2018
Merged

Fix coverage story and add coveralls.io. #85

merged 9 commits into from
Apr 23, 2018

Conversation

icemac
Copy link
Member

@icemac icemac commented Apr 20, 2018

No description provided.

@icemac icemac requested a review from jamadden April 20, 2018 09:53
@jamadden
Copy link
Member

While I’m all for coverage measurements, it might be tricky here:

  • C-accelerated builds take about 3x longer (2minuts becomes 6)
  • Pure-python builds on CPython timeout after 10 minutes of no output; we’d have to increase verbosity to get around that (and then we might hit the log length limit). I don’t know how long they’ll take in actuality.
  • PyPy builds will probably do the same, only much worse (previous experience showed that PyPy under coverage is really slow. I want to say the zodb tests took about 20 minutes compared to 4, but maybe that’s better now)

Questions:

Since the entire module is implemented in C, is coverage actually telling us much in that case? If yes, do we need to pay the cost in all environments or is one sufficient?

Is the extra build time worth it for the pure-python case? If we can find an appropriate verbosity I would guess yes, but only on CPython.

@jamadden
Copy link
Member

Since the entire module is implemented in C, is coverage actually telling us much in that case?

Well actually we get pretty good coverage reports from the C builds because we explicitly duplicate many of the tests for both the 'Py' and non-'Py' classes.

This suggests that the non-'Py' tests could be skipped in the pure-Python case, since they're identical. That would cut the time quite a bit.

There may be something we can do here with coverage 4.5's new plugin-based configuration too.

@icemac
Copy link
Member Author

icemac commented Apr 20, 2018

@jamadden Thank you for looking into this PR. I decided to run coverage only on two builds to reduce the time overhead. (I did not expect it to be that much.)

The log length does not seem to be a problem increased verbosity adds only less than 5000 characters to it.

I have no experiences with coverage 4.5's new plugin-based configuration so I left this part out. 😁

Are you okay with the other changes?

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. Some very minor comments. It would also be nice to have a badge in README.

Thanks for doing this! I love coverage reports.

- pip install -e .[test,ZODB]
script:
- zope-testrunner --test-path=. --auto-color --auto-progress
- |
Copy link
Member

Choose a reason for hiding this comment

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

TIL

Copy link
Member Author

Choose a reason for hiding this comment

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

Me, too. 😀

tox.ini Outdated
@@ -40,7 +40,7 @@ basepython =
python2.7
commands =
coverage run -m zope.testrunner --test-path=. --auto-color --auto-progress []
coverage report
coverage report --fail-under=92
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this number can be bumped to 96% based on the coveralls.io reports.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got only 92 % using tox when using the C implementation. When I switched to to use the pure Python one it even fell to 89 %. So the combined coverage seems to have more impact than it seems. But I did not add combined coverage to to tox.ini as the tests are already slow.

tox.ini Outdated
@@ -40,7 +40,7 @@ basepython =
python2.7
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest this should be 3.6, to match .travis.yml

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

- os: linux
python: 3.6
env: PURE_PYTHON=1
env:
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me that we need entries in the matrix for C-extension-with-coverage and pure-python-with-coverage, since pure-python is the only thing that coverage can actually detect. I would imagine that the pure-python-with-coverage case would catch everything except for a tiny bit of code in _import_c_module and perhaps the _skip_ functions.

But hey, overall this whole PR only added just over a minute to the build time, so that's a very minor issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@icemac icemac merged commit c58b031 into master Apr 23, 2018
@icemac icemac deleted the coverage branch April 23, 2018 07:17
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

2 participants