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

remove coveralls.io #5009

Merged
merged 1 commit into from Jul 1, 2015

Conversation

Projects
None yet
4 participants
@ev-br
Copy link
Member

ev-br commented Jul 1, 2015

It's disabled for a while now anyway (I wonder how). The full test suite regularly times out on submission to coveralls. Full suite runtime fluctuates from 35 to 50 minutes, but this likely out of our control, and this change is less intrusive than alternatives based on splitting the test suite into several runs.

Note that this only disables coveralls.io, not coverage itself. The latter still reports its findings in the CI log.

remove coveralls.io
It's disabled for a while now anyway (I wonder how). The full test suite regularly times out on submission to coveralls. Full suite runtime fluctuates from 35 to 50 minutes, but this likely out of our control, and this change is less intrusive than alternatives based on splitting the test suite into several runs.

Note that this only disables coveralls.io, not coverage itself. The latter still reports its findings in the CI log.

@ev-br ev-br added the maintenance label Jul 1, 2015

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Jul 1, 2015

Thanks @ev-br, forgot about removing this one. I hope it helps....

rgommers added a commit that referenced this pull request Jul 1, 2015

Merge pull request #5009 from scipy/ev-br-patch-1
MAINT: remove coveralls.io

@rgommers rgommers merged commit a95b10a into master Jul 1, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@rgommers rgommers added this to the 0.17.0 milestone Jul 1, 2015

@rgommers rgommers deleted the ev-br-patch-1 branch Jul 1, 2015

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Jul 1, 2015

@ev-br you managed to somehow push your branch to the scipy repo, from which you sent a PR. I deleted it now....

@ev-br

This comment has been minimized.

Copy link
Member Author

ev-br commented Jul 1, 2015

Ouch, sorry for the mess!
Note to self: this is what happens for a PR sent from a web interface. The error's clear in retrospect: need to do this from the fork, not the main repo.

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Oct 24, 2015

@rgommers @ev-br did this end up helping? It's a bit sad we can't use coveralls. But if it did indeed end up causing problems it makes sense. If it's unclear if it had an impact, I could open a PR to try to add it back in. The longest run I see on e.g. #5186 is about 26 minutes which should be pretty safe to add a little bit of time to. Maybe the builds got faster because of using container infrastructure in the meantime?

@pv

This comment has been minimized.

Copy link
Member

pv commented Oct 24, 2015

I think we could consider re-adding coveralls (I'm currently manually running runtests.py -g --coverage for a PR...). It's in use for asv, and I haven't seen upload problems there.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 24, 2015

I've found coveralls to be pretty disappointing to be honest:

  • the difference in coverage reported was usually 0.0% even when the actual coverage was different. Just because scipy was so large, so more or less coverage within a single function usually ends up being 0.0x% different.
  • coverage.io was slow
  • browsing to the output on coverage.io I usually got errors for individual files if they were larger than a couple of hundred lines.
@pv

This comment has been minimized.

Copy link
Member

pv commented Oct 24, 2015

Yes, it's true that their web interface is not good, and I'm not happy that it nowadays reports a failure if the coverage percentage decreases (so you get the red cross next to PRs).

The HTML report produced by Python's standard coverage module is much better.
Of course, it would be trivial to write a web service that just receives those reports, but someone would have to write and maintain it...

@ev-br

This comment has been minimized.

Copy link
Member Author

ev-br commented Oct 24, 2015

Maybe https://codecov.io/ is worth trying

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 24, 2015

codecov.io seems interesting. from the screenshot it looks like it would comment on PRs with the relevant part of the coverage report, which would be useful.

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Oct 24, 2015

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 24, 2015

Sounds good to me. If coveralls has improved a lot then I wouldn't be opposed to re-enabling it either. Maybe a good way to go is to enable whichever one you prefer on your own scipy fork? Then we can have a look at the results as they will be for this repo.

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Oct 24, 2015

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Dec 4, 2015

It took me over a month to click some buttons on Travis and Codecov, and change a few lines in .travis.yml but I finally got it done. I have a demo up on my repo, and you can see the results of a PR here (ignore the failures I killed some of the non-coverage builds):

larsoner#6
https://codecov.io/github/Eric89GXL/scipy?pr=6

Then once it's merged into master, subsequent PRs get notices like this (if they're turned on):

larsoner#7

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Dec 4, 2015

Looks like the first report link is dead -- I guess codecov gets rid of reports on PRs once they are merged, so the list of "branches" only includes those PRs that are still open.

In any case, I'm +1 for opening a PR into scipy master, as this seems to work well.

@larsoner

This comment has been minimized.

Copy link
Member

larsoner commented Dec 7, 2015

I also got an email from a codecov person asking for feedback. I mentioned that the grouping of branches with pull requests into a single menu is not as convenient as grouping the way e.g. Travis does it, and they informed me that they're working on an interface with better organization. So they seem responsive to feedback, too.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Dec 8, 2015

@Eric89GXL nice, that sounds promising. Browsing the coverage-highlighted code on codecov.io is much faster and more reliable then what it was on coveralls.io. A single comment by a bot that gets updated is probably less intrusive than repeated comments, so also sounds OK. So +1 from me.

larsmans added a commit to larsmans/scipy that referenced this pull request Feb 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.