Skip to content
This repository has been archived by the owner on May 13, 2019. It is now read-only.

Code coverage #31

Closed
santigimeno opened this issue Apr 15, 2016 · 20 comments
Closed

Code coverage #31

santigimeno opened this issue Apr 15, 2016 · 20 comments

Comments

@santigimeno
Copy link
Member

Having this included in our tooling would be great. There's a PoC from @evanlucas here: evanlucas/node@1445e3d with the status of current master here: https://nodejs-coverage-kvhhcskvtz.now.sh/. It only covers the js files but it looks like a good starting point.

Having this to identify the code not covered by tests can also be helpful to define areas where new contributors can help.

Ref: nodejs/build#225

@mhdawson
Copy link
Member

+1 from me in respect to wanted to get this run regularly in the CI and publishing the results. Maybe on nodejs.org as testcoverage.nodejs.org

@Fishrock123
Copy link

cc @chrisdickinson who worked on some amount of this quite some time back

@Trott
Copy link
Member

Trott commented Jun 24, 2016

Look what @addaleax did!

Some similarities to @evanlucas's thing on the JS side, so I guess there's some common tooling being used there.

@evanlucas
Copy link

what @addaleax has done is awesome. Much better than mine :]

@Fishrock123
Copy link

Oooh I call fixing the timers code coverage. :3

@cjihrig
Copy link

cjihrig commented Jun 24, 2016

Is there any way to combine coverage results across multiple runs? Windows coverage is reported as lower than it really is right now.

@addaleax
Copy link
Member

@evanlucas I’d actually say it looks incredibly similar :D (wasn’t aware of your approach when starting to code, though)

@cjihrig Probably. For the JS coverage, that should be trivial, for C++… I have no experience doing coverage with MSVS, and I don’t know how different the coverage output formats would be.

This is just a cronjob run on a single Linux machine, so coverage is definitely higher than it is indicated here.

@Fishrock123
Copy link

Fishrock123 commented Jun 24, 2016

@addaleax Not to pile more work on you, but I think we'll definitely need a way to recognize opt-out comments. Some spots in the codebase should be unreachable and the tooling will need to recognize that.

What tooling are you using for this? All custom, or?

@Fishrock123
Copy link

Also there are a couple places that I'm legitimately not sure are possible to reach in regular tests, such as the logic for _third_party_main

@addaleax
Copy link
Member

addaleax commented Jun 24, 2016

What tooling are you using for this? All custom, or?

istanbul for the JS side, gcc’s --coverage option (gcov) for C++, there’s nothing custom in there, sorry.

@addaleax
Copy link
Member

i.e. if we decide to stick with gcc’s coverage, http://stackoverflow.com/a/4745127/688904 should do the trick?

I agree that it might be nice to have a more flexible coverage tool that does things istanbul-style, but I’d have to do a bit more research on that.

@Fishrock123
Copy link

Oh if it's Istanbul then that already has support for ignoring code iirc.

@santigimeno
Copy link
Member Author

This is great. Excellent job!

@Trott
Copy link
Member

Trott commented Jun 24, 2016

Test coverage data bearing fruit: nodejs/node#7413

@mhdawson
Copy link
Member

Optimally I'd like this to be run regularly in the CI and potentially pushed to something like coverage.nodejs.org

I've used gcov in the past and although not ideal since it does not give platform specific coverage we ran on linux nightly and it resulted in good information to action. We could likely limit to a single run on master (assuming that is where we'd do most work to close coverage gaps) so the overhead would be 1 single additional compile on linux per day.

Do others think we should make nightly runs/publication on coverage.nodejs.org a goal ?

@mhdawson
Copy link
Member

mhdawson commented Jun 29, 2016

I should have also said I can volunteer to do some of the work to add jenkins jobs etc.

@addaleax
Copy link
Member

Right now the approach I use requires applying patches to the node core source code and build files… is that a problem? Does anybody have better ideas? I assume that if this is going to become a “blessed” feature of Node core, something more reliable would be nice (that doesn’t break e.g. when the patches don’t apply cleanly anymore).

@mhdawson
Copy link
Member

Could you list out the changes required to the source/build files at a high level? Depending on what they are, for example to add additional options to enable the gcov generation, we might able to build them into the build/configuration files guarded by an option like --enable-gcov or something like that

@addaleax
Copy link
Member

For the C++ coverage, that’s basically adding compiler flags, and that can probably be included in the build config quite easily.

For JS, right now it requires adding a more or less reliably called exit hook to the source for writing the coverage data to a file, creating a copy of the lib/ tree with the instrumented code, and including the files from that tree in the binary. This one could maybe be implemented as an option to js2c.py or something like that.

@mhdawson
Copy link
Member

This is being progressed in #36 I think we can close this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants