10000 TST: Integrate codecov testing by alyssaq · Pull Request #11567 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

TST: Integrate codecov testing #11567

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2018
Merged

TST: Integrate codecov testing #11567

merged 1 commit into from
Jul 20, 2018

Conversation

alyssaq
Copy link
Contributor
@alyssaq alyssaq commented Jul 14, 2018 •
edited
Loading

Added RUN_COVERAGE flag to specify whether to run code coverage and passed --cov-report=xml to pytest to output the code coverage report in XML. Report is uploaded to the codecov service.

I had to ignore depreciation warnings from virtualenv as it was erroring the tests in python3.6. Its still an open issue: pypa/virtualenv#1120

Part of scipy2018 code sprint 🐍

@charris
Copy link
Member
charris commented Jul 14, 2018

LGTM failure looks unrelated.

@charris charris changed the title BLD. WIP: scipy2018 sprint. Testing out codecov integration TST, WIP: scipy2018 sprint. Testing out codecov integration Jul 14, 2018
@charris charris changed the title TST, WIP: scipy2018 sprint. Testing out codecov integration TST, WIP: Integrate codecov testing Jul 14, 2018
@rgommers
Copy link
Member

I'm +1 to adding codecov - they've improved their performance and removed the very spammy "post comments with coverage results". It's still not ideal, but it has value.

@charris
Copy link
Member
charris commented Jul 14, 2018

The LGTM failure is a connection problem affecting all the recent PRs and can be ignored.

@alyssaq When you are done, please squash all the commits into one.

@tylerjereddy tylerjereddy mentioned this pull request Jul 14, 2018
Merged
@pv
Copy link
Member
pv commented Jul 14, 2018 •
edited
Loading

For numpy specifically, adding gcov support would be useful -- needs adding the --coverage compiler flags (to OPT=) and then doing something like this afterward: https://github.com/scipy/scipy/blob/master/.travis.yml#L203

Can be done later in separate PR, I guess though.

@tylerjereddy
Copy link
Contributor

@pv yup, @alyssaq noticed that in SciPy during the sprint

I think @stefanv wants to add something similar for scikit-image as well

@alyssaq alyssaq force-pushed the wip-coverage branch 2 times, most recently from e225f81 to 52f2d05 Compare July 15, 2018 04:57
@codecov-io
Copy link
codecov-io commented Jul 15, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #11567   +/-   ##
=========================================
  Coverage          ?    85.7%           
=========================================
  Files             ?      327           
  Lines             ?    81965           
  Branches          ?        0           
=========================================
  Hits              ?    70246           
  Misses            ?    11719           
  Partials          ?        0

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 a32749e...9530159. Read the comment docs.

@alyssaq alyssaq changed the title TST, WIP: Integrate codecov testing TST: Integrate codecov testing Jul 15, 2018
@pv
Copy link
Member
pv commented Jul 15, 2018 via email •
edited
Loading

tylerjereddy
tylerjereddy reviewed Jul 15, 2018
View reviewed changes
Copy link
Contributor
@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Looks like the coverage of Python files is being properly reported from this PR via codecov now (click through the link to take a look).

The landing page / README from the PR branch is properly displaying the codecov badge with % coverage.

Based on reviewer comments here, it looks like the main thing this PR needs now is the addition of codecov.yml with comment: false set so we don't get spammed.

It looks like C-level line coverage isn't being reported just yet, but as noted above this likely requires the appropriate gcc compiler flags. One thought here--will that eventually require a separate job on the CI matrix so that the full Python 3.6 CI job / tests with "normal" compiler flags isn't confounded with the coverage compiler settings?

@alyssaq
Copy link
Contributor Author
alyssaq commented Jul 15, 2018

Ive added codecov.yaml with comment: off. Not sure if we want to add the other settings similar to scipy: https://github.com/scipy/scipy/blob/master/codecov.yml

@rgommers
Copy link
Member

Not sure if we want to add the other settings similar to scipy: https://github.com/scipy/scipy/blob/master/codecov.yml

The Require 1% coverage, i.e., always succeed part we'll need (we never want CI failures for this), and the appveyor thing is probably doing something useful as well.

@alyssaq
Copy link
Contributor Author
alyssaq commented Jul 16, 2018

Ok. The appveyor and target: 1 to require 1% coverage has been added.

tylerjereddy
tylerjereddy reviewed Jul 18, 2018
View reviewed changes
Copy link
Contributor
@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

It looks like most reviewer comments have been addressed, as long as people are ok with adding the C coverage stuff in a later PR.

I have one remaining suggestion -- the file codecov.yaml in this PR should probably be renamed to one of the two officially supported file names--basically just remove the a in yaml extension to be safe for auto-detection.

@alyssaq
Copy link
Contributor Author
alyssaq commented Jul 18, 2018

@tylerjereddy Thanks for spotting that! Ive renamed it to code 8000 cov.yml

@charris
Copy link
Member
charris commented Jul 18, 2018

Both .travis.yml and .appveyor.yml are hidden, would that also work for codecov.yml?

@tylerjereddy
Copy link
Contributor

Both .travis.yml and .appveyor.yml are hidden, would that also work for codecov.yml?

Yes, from the codecov FAQ:

Can I name the file .codecov.yml?

Yes. You can name the file either codecov.yml or .codecov.yml. The file can be placed anywhere in your repository.

@charris
Copy link
Member
charris commented Jul 19, 2018

Mind, I would almost prefer the unhidden alternative, but it would be best to be consistent.

stefanv
stefanv reviewed Jul 19, 2018
View reviewed changes
tools/travis-test.sh

if [ -n "$RUN_COVERAGE" ]; then
# Upload coverage files to codecov
bash <(curl -s https://codecov.io/bash) -X gcov -X coveragepy
Copy link
Contributor

Choose a reason for hiding this comment

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

@tylerjereddy Is this preferred over installing codecov from pip?

@alyssaq
Copy link
Contributor Author
alyssaq commented Jul 19, 2018

According to codecov’s latest docs, they recommend the bash uploader as the “uploader to rule them all”

@stefanv
Copy link
Contributor
stefanv commented Jul 19, 2018

Great, thanks for clarifying!

@tylerjereddy
Copy link
Contributor

This looks ready to merge and there's an approving review from a core dev. The commit message might benefit from matching our documented standard for i.e., prefixing with TST or something similar, but beyond that looks like concerns are addressed.

@charris charris merged commit 2724b6c into numpy:master Jul 20, 2018
@charris
Copy link
Member
charris commented Jul 20, 2018

Thanks @alyssaq .

@pv
Copy link
Member
pv commented Jul 22, 2018

Didn't seem to turn the codecov comment off, see gh-11601

@tylerjereddy
Copy link
Contributor

@pv That PR (and the other recent PR codecov comment bot "victims") appear not to be rebased on the lastest master branch, so they naturally don't have the yml file disabling the comment activity. I checked out 3 such PRs and confirmed that they don't have pertinent commits yet.

Presumably the comment spam will gradually disappear as people start to rebase before opening PRs.

At least, that's my theory--seems sensible I think given that we match the official docs for disabling comments & do the same thing as SciPy in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerjereddy tylerjereddy tylerjereddy left review comments

@stefanv stefanv stefanv approved these changes

Assignees
No one assigned
Labels
05 - Testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@alyssaq @charris @rgommers @pv @tylerjereddy @codecov-io @stefanv
0