search

nodejs / node-addon-api

Module for using Node-API from C++
MIT License
2.15k stars 460 forks source link

test: add basic coverage support #1451

Closed gabrielschulhof closed 6 months ago

gabrielschulhof commented 7 months ago

The result looks pretty decent at first blush:

image

AFAICT, this only works on Linux, and doesn't integrate with any online coverage tools.

BEGIN_COMMIT_OVERRIDE test: add basic coverage support (#1451) END_COMMIT_OVERRIDE

gabrielschulhof commented 7 months ago

Hmmm ... a lot of the lines are neither covered nor not covered, because they're ifdef-ed out. So, we'd have to build and test with a lot more variations (e.g. bindings_v5.node, binding_noexcept_v5.node, ... for NAPI_VERSION 5).

KevinEady commented 7 months ago

So where I left off on my investigations:

This gave me a 61% coverage, see here. What I found interesting (and touched on in last week's meeting) is that this cout line was marked as "Uncovered" yet is present inside the CI's action run log. This run also turned off compiler optimizations using -O0.

Not exactly sure how you are getting in the 90s on your coverage, so I tried again using your changes on a new PR. This also resulted in 64% code coverage, see here.

Maybe it has something to do with the functions being inlined? I dunno

legendecas commented 7 months ago

On the Feb 23 meeting, @KevinEady mentioned that he would verify the generated coverage report locally to see uncovered branches and discrepancies with the optimization on/off.

KevinEady commented 7 months ago

Some findings here https://github.com/KevinEady/node-addon-api/pull/6#issuecomment-1961987002

codecov-commenter commented 7 months ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

KevinEady commented 7 months ago

Hi team,

I've added the codecov.io service integration. You can see in the above comment that once this is merged, we'll start to see messages regarding coverage in new PRs. You can see the coverage for this branch on codecov.io.

Additionally, I did a test on my local fork with removing all the tests for threadsafe functions and async workers. You can see the comment on the PR with codecovs report here, with a -13.03% change. You can click through to codecov.io's hosted report and see detailed information.

We can discuss some configurations in the next meeting:

  1. Should coverage run on draft PRs? It currently does not, and that is core's approach, but our CI job run time is a fraction of that compared to core's (~10min vs ~1hr45min).
  2. Do we want to upload the HTML coverage report as an artifact of the CI build? I ended up using this while diagnosing the "discrepencies between gcovr html and codecov.io web reports" issue. Not sure if we should keep it, as core does not do this.
KevinEady commented 6 months ago

Coverage now runs on drafts as well, and removed the HTML report generation + upload