[ready] Add coverage testing support #271

puremourning · 2015-12-12T14:40:35Z

Add information on running coverage tests and tidy up the developer test documentation.
Run a new Travis build with coverage enabled.
Pass coverage data to coveralls.io.

Add information on running coverage tests and tidy up the developer test documentation.

vheon · 2015-12-12T14:44:04Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.

TESTS.md, line 35 [r1] (raw file):
The more I think about all completers needed to run the tests the more I'd like to have a Vagrantfile with a prebuild image to run them.

Comments from the review on Reviewable.io

micbou · 2015-12-12T15:11:35Z

Should we use coveralls for ycmd?

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

TESTS.md, line 6 [r1] (raw file):
.travis.yml instead of .travis.yaml.

Comments from the review on Reviewable.io

puremourning · 2015-12-12T15:25:05Z

Well, let's see what happens.

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2015-12-12T15:38:59Z

We need to create an account on the website, are we?

Anyway, this coverage tool is great: a score of 85%, not bad!

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

puremourning · 2015-12-12T15:44:46Z

Seemed to work for my fork. Looks like a neat tool. https://coveralls.io/github/puremourning/ycmd-1

Would require @Valloric to link the repo.

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-12T15:48:29Z

I'm not crazy about coveralls.io because once you have that information for every commit you push then it feels like you're doing the tests only because you don't want that number to go down instead of making the tests that matter and that bring value to the project. I'm not saying that having that number is a bad thing, that would be stupid, but the fact of having that number constantly would be counterproductive IMHO.

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

puremourning · 2015-12-12T16:10:04Z

Well, the reason I went looking for coverage tools is that i wanted augment my current testing efforts. I find coverage reports very useful to ensure that:

i'm testing what I think i'm testing
i don't have dead code
i have tests for all major features and error cases

Chasing a statistic for the sake of the statistic is certainly somewhat pointless, but a handful of tests covering 100% of the code is certainly better than no tests at all. And possibly better than 100 tests testing the same codepath. At least, IMO.

Anyway - I'm not too bothered either way. It is 2 lines in the travis setup to enable/disable it, but documenting how to do it manually was my goal - primarily because I will forget if i don't!

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-12T20:25:54Z

Well, the reason I went looking for coverage tools is that i wanted augment my current testing efforts. I find coverage reports very useful to ensure that:

i'm testing what I think i'm testing
i don't have dead code
i have tests for all major features and error cases
Chasing a statistic for the sake of the statistic is certainly somewhat pointless, but a handful of tests covering 100% of the code is certainly better than no tests at all. And possibly better than 100 tests testing the same codepath. At least, IMO.

I agree with you that testing is a really important thing (you're not alone :)) and I agree that having some number to have a sense if the way we test things is useful. I just wanted to point out that having that number constantly is not always a good thing.

Anyway - I'm not too bothered either way. It is 2 lines in the travis setup to enable/disable it

We could always try it and if we don't find it useful we could disable it.

documenting how to do it manually was my goal - primarily because I will forget if i don't!

👍

micbou · 2015-12-12T20:47:18Z

I think we should run coverage only for one build: Python 2.7 with CLANG_COMPLETER=true on Linux (or add a specific build for it).

Reviewed 1 of 2 files at r1, 1 of 2 files at r2.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-12T20:51:42Z

A specific build only for coverage seems a good thing (separation of the tests from the statistics) but wouldn't also be a waste of resources? we should basically re-run the tests, right?

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

micbou · 2015-12-12T21:08:57Z

It would be only 1/9 more resources and since Travis run builds in parallel, the duration would be nearly the same.

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

Valloric · 2015-12-12T23:33:07Z

@vheon I (and everyone else here) agree with your point that chasing that coverage number is silly. Of course it is. But coverage metrics are very, very useful when they can provide insight like "given a file, which lines are being exercised by tests"? Holy crap is that useful. I've found far too many instances of "hey I thought that was being tested" (when it wasn't) through such tools.

A general number showing "X% of this project is covered by tests" is also useful as a badge of quality. People looking at the project can feel more confident depending on it when the number is >80%. Hell, just seeing the coveralls.io badge in the README makes me have a higher opinion of any GitHub project (even if the computed number is low) because the people working on it at least ostensibly care about testing.

And if the badge has even a 1% positive influence encouraging people to write more tests, I'll gladly take that.

So WRT coveralls.io, I say we go for it. I created an account and turned it "on" for ycmd & YouCompleteMe. Do I need to do anything else?

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

TESTS.md, line 35 [r1] (raw file):
That's actually not a bad idea at all. Would make it easier to ramp up new people or even one-off contributors who are changing/adding something substantial.

Could you create an issue for it so we don't forget?

Comments from the review on Reviewable.io

Valloric · 2015-12-12T23:33:19Z

BTW

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-13T00:08:36Z

too. I'm not firing homu because I don't know if we need to do anything else for coveralls.io so @puremourning if it is ready merge this yourself 👍

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

TESTS.md, line 35 [r1] (raw file):
👍

Comments from the review on Reviewable.io

micbou · 2015-12-13T02:20:31Z

This is how I added one coverage run on Travis and here the Travis results.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T17:30:54Z

I've implemented @micbou's approach of running an extra build just for coverage.

Review status: 1 of 5 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-13T17:44:11Z

Reviewed 2 of 4 files at r3.
Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions.

TESTS.md, line 76 [r3] (raw file):
Should this be in the test_requirements.txt?

TESTS.md, line 77 [r3] (raw file):
What happened to --skip-build?

Comments from the review on Reviewable.io

puremourning · 2015-12-13T17:46:29Z

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions.

TESTS.md, line 76 [r3] (raw file):
It isn't strictly required

TESTS.md, line 77 [r3] (raw file):
It isn't required. It's still optional.

Comments from the review on Reviewable.io

vheon · 2015-12-13T17:54:35Z

Reviewed 1 of 4 files at r3.
Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon · 2015-12-13T17:55:14Z

Should we add the coveralls.io badge to the README?

Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

micbou · 2015-12-13T19:07:31Z

Reviewed 3 of 4 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

run_tests.py, line 100 [r4] (raw file):
A print left behind.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T19:12:48Z

Review status: 5 of 6 files reviewed at latest revision, 2 unresolved discussions.

run_tests.py, line 100 [r4] (raw file):
thanks. fixed. sorry bout that.

Comments from the review on Reviewable.io

vheon · 2015-12-13T19:20:16Z

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2015-12-13T20:09:17Z

@puremourning Could you add the coveralls.io badge to the README as well? I want users to see we care about testing.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T20:10:22Z

i thought i did...

README.md

[![Coverage Status](https://coveralls.io/repos/Valloric/ycmd/badge.svg?branch=master&service=github)](https://coveralls.io/github/Valloric/ycmd?branch=master)

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2015-12-13T20:15:59Z

Bah, I'm blind. :)

Is this ready to merge?

BTW we might want to get in the habit of adding a "[READY]" prefix to PR titles when the author feels it's ready to be merged. Should save an RTT or two.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T20:16:58Z

Yes i think this is ready.

Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2015-12-13T20:18:49Z

@homu r+

Thank you for working on this!

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

homu · 2015-12-13T20:18:50Z

📌 Commit aae3bee has been approved by Valloric

homu · 2015-12-13T20:19:01Z

⚡ Test exempted - status

[ready] Add coverage testing support Add information on running coverage tests and tidy up the developer test documentation. Run a new Travis build with coverage enabled. Pass coverage data to coveralls.io.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/271)

Add coverage information to test documentation

041184a

Add information on running coverage tests and tidy up the developer test documentation.

Add coveralls.io support

796e516

puremourning changed the title ~~Add coverage information to test documentation~~ [tests] Add coverage testing support Dec 13, 2015

puremourning added 2 commits December 13, 2015 19:11

Only run coverage on a single special run

413af8b

Add coverage badge for Valloric/ycmd

aae3bee

puremourning force-pushed the tests-coverage-doc branch from 117f933 to aae3bee Compare December 13, 2015 19:12

puremourning changed the title ~~[tests] Add coverage testing support~~ [ready] Add coverage testing support Dec 13, 2015

homu merged commit aae3bee into ycm-core:master Dec 13, 2015

micbou mentioned this pull request Dec 13, 2015

[READY] Refactoring integration tests #270

Merged

puremourning deleted the tests-coverage-doc branch December 14, 2015 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ready] Add coverage testing support #271

[ready] Add coverage testing support #271

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

puremourning commented Dec 12, 2015

micbou commented Dec 12, 2015

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

Valloric commented Dec 12, 2015

Valloric commented Dec 12, 2015

vheon commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

vheon commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

Valloric commented Dec 13, 2015

puremourning commented Dec 13, 2015

Valloric commented Dec 13, 2015

puremourning commented Dec 13, 2015

Valloric commented Dec 13, 2015

homu commented Dec 13, 2015

homu commented Dec 13, 2015

[ready] Add coverage testing support #271

[ready] Add coverage testing support #271

Conversation

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

puremourning commented Dec 12, 2015

micbou commented Dec 12, 2015

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

puremourning commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

vheon commented Dec 12, 2015

micbou commented Dec 12, 2015

Valloric commented Dec 12, 2015

Valloric commented Dec 12, 2015

vheon commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

vheon commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

Valloric commented Dec 13, 2015

puremourning commented Dec 13, 2015

Valloric commented Dec 13, 2015

puremourning commented Dec 13, 2015

Valloric commented Dec 13, 2015

homu commented Dec 13, 2015

homu commented Dec 13, 2015