-
Notifications
You must be signed in to change notification settings - Fork 19
Code coverage #31
Comments
+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 |
cc @chrisdickinson who worked on some amount of this quite some time back |
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. |
what @addaleax has done is awesome. Much better than mine :] |
Oooh I call fixing the timers code coverage. :3 |
Is there any way to combine coverage results across multiple runs? Windows coverage is reported as lower than it really is right now. |
@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. |
@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? |
Also there are a couple places that I'm legitimately not sure are possible to reach in regular tests, such as the logic for |
|
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. |
Oh if it's Istanbul then that already has support for ignoring code iirc. |
This is great. Excellent job! |
Test coverage data bearing fruit: nodejs/node#7413 |
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 ? |
I should have also said I can volunteer to do some of the work to add jenkins jobs etc. |
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). |
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 |
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 |
This is being progressed in #36 I think we can close this issue. |
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
The text was updated successfully, but these errors were encountered: