feat(@bazel/jasmine): add coverage reporting

[Toxicable]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Feature (please, look at the "Scope of the project" section in the README.md file)

What is the current behavior?

jasmine_node_test does not report code coverage

What is the new behavior?

jasmine_node_test does reports code coverage

Does this PR introduce a breaking change?

• Yes
• No

Example oputput:

:~/Documents/rules_nodejs/packages/jasmine$ bazel run test:coverage
INFO: Invocation ID: c4e9cd85-aad8-4fad-9537-71c8a665861c
INFO: Analysed target //test:coverage (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //test:coverage up-to-date:
  bazel-bin/test/coverage_loader.js
  bazel-bin/test/coverage.sh
INFO: Elapsed time: 0.190s, Critical Path: 0.01s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //test:coverage
-----------------------------------------------------------------------------
Randomized with seed 34724
Started
.


1 spec, 0 failures
Finished in 0.006 seconds
Randomized with seed 34724 (jasmine --random=true --seed=34724)

=============================== Coverage summary ===============================
Statements   : 0% ( 0/48 )
Branches     : 0% ( 0/20 )
Functions    : 0% ( 0/9 )
Lines        : 0% ( 0/47 )
================================================================================

[Toxicable]

While we do have it working so far I'm running into an issue where istanbul is istrumenting files that aren't apart of the sources.
Since we have a well define list of soruces that should be instrumented I think the best option would be to see if istanbul can implement an includes api rather than trying to use thei excludes api.

Without any exlucdes it'll auto discover these files, producing an incorrect sode coverage report.

"/home/f.wiles/Documents/rules_nodejs/third_party/github.com/buffer-from/index.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map-support/source-map-support.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/array-set.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/base64-vlq.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/base64.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/binary-search.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/mapping-list.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/quick-sort.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/source-map-consumer.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/source-map-generator.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/source-node.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/lib/util.js
/home/f.wiles/Documents/rules_nodejs/third_party/github.com/source-map/source-map.js
/home/f.wiles/.cache/bazel/_bazel_f.wiles/6b62bb60168067291695fe6dd86d865d/external/nodejs/bin/yarnpkg/bin/yarn.js
/home/f.wiles/.cache/bazel/_bazel_f.wiles/6b62bb60168067291695fe6dd86d865d/external/nodejs/bin/yarnpkg/lib/cli.js
/home/f.wiles/.cache/bazel/_bazel_f.wiles/6b62bb60168067291695fe6dd86d865d/external/nodejs/bin/yarnpkg/lib/v8-compile-cache.js
/home/f.wiles/Documents/rules_nodejs/packages/jasmine/src/jasmine_runner.js"

[alexeagle]

Seems like Istanbul hacking might not be needed anymore

In Node.js >=10.10.0 you can now set the environment variable NODE_V8_COVERAGE to a directory, this will result in V8 coverage reports being output in this location.
https://medium.com/the-node-js-collection/rethinking-javascript-test-coverage-5726fb272949

We could try that?

[Toxicable]

Oh wow, that looks awesome.
Yeah we'll have a look into that and see how we can intergrate it with the test runner.

It does mean we'll need to push a requirement on the nodejs version.
Could be done as a warning or maybe a flag to enable coverage collection + an error if the version isn't right.

[Toxicable]

@alexeagle Started inital work on v8-coverage.
the way im planning on doing it is by adding another .js file to the jasmine rule: jasmine_runner_host.js, this file will be the new entry point and will fork itself so it can set the NODE_V8_COVERAGE flag to a known directory for processing after tests and been executed.

However im running into issues where the sub process (jasmine_runner.js) is unable ro resolve "bazel" nodejs paths ie: Cannot find module 'npm_bazel_jasmine/test/foo_test.js' any ideas why this might happen?
Or if there's a better way to set the environment variable to a dir we know and and work on later on

[Toxicable]

I think this is ready for an inital review now

From looking at #312
I was able to execute the forked script, but it feels pretty hacky and I'm not sure if it'll work in other situations.

Executing tests from //test:coverage_test
-----------------------------------------------------------------------------
Randomized with seed 23022
Started
.


1 spec, 0 failures
Finished in 0.014 seconds
Randomized with seed 23022 (jasmine --random=true --seed=23022)

=============================== Coverage summary ===============================
Statements   : 90% ( 9/10 )
Branches     : 60% ( 3/5 )
Functions    : 100% ( 1/1 )
Lines        : 90% ( 9/10 )
================================================================================

[Toxicable]

So looks like the fork script hack only works when you run the test with bazel run if you use bazel test it fails to resolve the path.
Any advice how I can hook up these paths?

[alexeagle]

I don't see why you need to fork a process. We have control over the nodejs options passed to the original nodejs process that starts the tests. Can we set the flag there?

[Toxicable]

@alexeagle Sure, all we need is to set the environment variable. I couldn't find how to do that with nodejs_binary

[Toxicable]

Quick update here:

I tried doing this without using a forked script but it would appear the coverage file is only written once the node process exits. Meaning this must be done from some other process somehow.

This is what it looks like in that setup for reference
https://github.com/Toxicable/rules_nodejs/blob/169400aa4abbfc10ffd8d078910941d416496e28/packages/jasmine/src/jasmine_runner.js

[Toxicable]

Ignore what I said above, turns out I was all wrong, you can produce coverage without a new process.
I switched to a new lib: v8-coverage which internally starts the Nodejs inspector module.
Can be previewed with bazel run test:coverage_test, gives the output below.

Fabians-MacBook-Air:jasmine fabianwiles$ bazel run test:coverage_test
...
Executing tests from //test:coverage_test
-----------------------------------------------------------------------------
Debugger listening on ws://127.0.0.1:57032/c20d1245-cce8-42e5-b956-437c8b21dddf
For help, see: https://nodejs.org/en/docs/inspector
Randomized with seed 12938
Started
.
1 spec, 0 failures
Finished in 0.008 seconds
Randomized with seed 12938 (jasmine --random=true --seed=12938)

=============================== Coverage summary ===============================
Statements   : 80% ( 8/10 )
Branches     : 75% ( 3/4 )
Functions    : 100% ( 0/0 )
Lines        : 80% ( 8/10 )
================================================================================

[Globegitter]

@Toxicable if you want to be able to set the env variable before the target starts there is a generic solution, you can use the command rule from here: https://github.com/atlassian/bazel-tools/blob/master/multirun/README.md

Plus I also opened an issue: bazelbuild/bazel#7364 as I (and some of our devs on the team) expected this to be part of bazel in the same way as one can set args generically.

[Globegitter]

Also are there any plans to integrate this with the bazel coverage command? If it is even in a state where one can do that. I do know official coverage support is still a bit of a WIP.

[Toxicable]

@Globegitter Ahhh, thanks for the heads up on the command rule.
However, for this situation we'd still need to run some JS after the jasmine_runner has exited, since it would appear that using the NODE_V8_COVERAGE environment variable only writes the coverage once it exits the process. So we would still have to run some JS host that forks into jasmine_runner.
I think the current solution should work, aslong as there's no weird interactions with other apis due to using the inspector api.

As for bazel coverage - I'd definitely like to integrate with it and from here it should easy to do so really since we can go to any coverage format, I just don't know how, I havn't been able to find any docs on how to do so.
So for now using text-summary will work as a stop gap, I'm not sure if we want to merge it in this state without bazel coverage integration though.

[alexeagle]

I'll discuss with the Bazel team this week about adding proper bazel coverage command. I'll leave it to @Globegitter and @gregmagolan about whether to merge this with a text summary or wait for guidance.

[Globegitter]

Imo this is useful to already get merged in without bazel coverage support. Just a few comments/questions I left from my side. And as I can see this is now always turned on, right? Might be worth making configurable as not everyone might want the coverage support active. Also might be worth allowing the regex for finding the coverage files to be configurable, but also happy to keep it simple for now and it is always easy to add if anyone would need this.

[Globegitter]

Why do we expect this to be passed in by the user?

[Globegitter]

Why is this removed?

[Globegitter]

Maybe we can add this to gitignore

[Globegitter]

Did you mean to call it this, or maybe a typo?

[Globegitter]

The variable name I mean btw.

[Globegitter]

Any particular reason to wrap this in a nodejs_binary compared to how it was before?

[Globegitter]

I don't see this file. Leftover?

[Toxicable]

@Globegitter Thanks for the review. Will do a tidy up tonight.

Right now it's on no matter what. There is a small perf overhead ~20% with this.
Maybe we should provide an attribute to opt-in incase there's issues we havn't seen with it so far, then later on make a breaking change to switch to opt-out to encourage people to use the coverage report.
Although im not sure how we'd pass down extra flags from nodejs_test into the runner, we'd have to convert this into a actual rule definition right? (i've always struggled to make those work)

Im not sure what you mean by

regex for finding the coverage files to be configurable,

This is the part where we filter out the coverage collected from external source files
https://github.com/bazelbuild/rules_nodejs/pull/553/files#diff-b7f7068522dfd058878f39fba971463aR87
which is just a direct file match, I don't see why you'd want it to be configurable though, this will always pick up all user source files that were in the test, is there case where you'd only want a subset of that?

[Globegitter]

Yeah I think we should have coverage turned off by default for now and then later we can switch it on. One way I can see it work is by adding a coverage parameter to the jasmine_node_test macro and if that is set you can pass a custom arg via args to the nodejs_test and then that can be checked in the runner.

[Globegitter]

About the file match, yeah the static one you have works for idiomatic bazel usage, but I was thinking of code bases like our own where we have common js code in sloppy file group targets that have a bunch of files grouped together even if an app might just need a subset. So in that case we might want to be able to specify our own regex to filter out specific files. But as I said I think it is good to keep it simple and promote idiomatic use. So I think it makes sense to just leave it as is.

[Toxicable]

@Globegitter Ah yes, I see your point, makes sense.

I've implemented the coverage flag and disabled it by default.
Your arguments idea works well here
You can re-enable it with coverage = True in your jasmine_node_test

One remaining point is that I'm using the dep "@npm//v8-coverage" - should we make a npm_bazel_jasmine_deps yarn_install to get this dep from instead?
I've noticed there's been a bit of change about where these deps are coming from lately so not sure on the best option.

[Globegitter]

I am also not familiar with the new process yet, but as far as I understand, once this is merged a new npm package has to be built and then one just adds the latest @bazel/jasmine to their package.json and will get the dependencies via normal npm resoultion. Maybe @alexeagle or @gregmagolan can fill in on how the release / dependency process is currently working.

[gregmagolan]

Yes @Globegitter you are correct. /packages/jasmine gets packaged up by //packages/jasmine:npm_package and released as @bazel/jasmine. With yarn_install or npm_install, the npm package installs the npm_bazel_jasmine workspace via the generated @npm://install_bazel_workspaces.bzl and it also pulls in transitive npm deps such as the jasmine npm package.

[gregmagolan]

For CI & developing & testing locally I made a number shell scripts that build the packages before running the e2e test and examples. You can call ./scripts/build_packages.sh jasmine to build the jasmine package and its is copied out of the bazel-out folder into /dist/jasmine. The e2e tests and examples then link to this package in their package.json.

[gregmagolan]

So, if @bazel/jasmine requires the v8-coverage npm package then that just needs to be added as a dependency to /packages/jasmine/package.json and it will be pulled into a user's node_modules transitively when the install @bazel/jasmine.

Hmmmm. I think I need to spend some time to document this in DEVELOPING.md :)

[Toxicable]

Alrighty, I think we're good to go here now.
I added a bit of a summary to the commit message description of this feature and it's current status.
Also debugging still appears to work even with the internal call the inspector API
v8-coverage is definitely in the package.json so deps should be good once in user land

[Globegitter]

Thanks for all the quick responses @Toxicable. The only thing that would be nice is some e2e test that actually verifies that the coverage report is in the output / is not in the output dependent on the setting. But I would be inclined to get it merged already with this basic smoke test. What do you think @gregmagolan?

[gregmagolan]

This looks great @Toxicable. Thanks for the nice addition. Left a few comments. Sorry but you'll need a rebase. We got some jasmine fixes in this morning.

[Toxicable]

@gregmagolan Thanks for the review, I've addressed all your points, let me know if there's anything else.
With the exception of adding an e2e test for the text-summary output. Since in my next PR ill be turning that off text-summary and going to lcov in an attempt to intergrade with bazel coverage

[Globegitter]

@gregmagolan @Toxicable One thing I forgot is documentation, I assume skydoc should pick up the added comment about coverage automatically, but is there anything automatically updating the docs? Looking at https://bazelbuild.github.io/rules_nodejs/jasmine_node_test/jasmine_node_test.html#jasmine_node_test I can not see the new coverage attribute commented.

[gregmagolan]

@gregmagolan @Toxicable One thing I forgot is documentation, I assume skydoc should pick up the added comment about coverage automatically, but is there anything automatically updating the docs? Looking at https://bazelbuild.github.io/rules_nodejs/jasmine_node_test/jasmine_node_test.html#jasmine_node_test I can not see the new coverage attribute commented.

We typically update the docs with each release so the docstring from jasmine_node_test.bzl will get updated at that point:

def jasmine_node_test(
        ...
        **kwargs):
    """Runs tests in NodeJS using the Jasmine test runner.

    To debug the test, see debugging notes in `nodejs_test`.

    Args:
      name: name of the resulting label
      srcs: JavaScript source files containing Jasmine specs
      data: Runtime dependencies which will be loaded while the test executes
      deps: Other targets which produce JavaScript, such as ts_library
      expected_exit_code: The expected exit code for the test. Defaults to 0.
      tags: bazel tags applied to test
      jasmine: a label providing the jasmine dependency
      coverage: Enables code coverage collection and reporting
      **kwargs: remaining arguments are passed to the test rule
    """

which includes the short description of coverage.

@alexeagle Has done more work on docs and some things have changed since moving to separate npm packages such as @npm//jasmine. I don't know if we have the docs from each package hosted at bazelbuild.github.io yet. The docs there may still be from the legacy /internal/jasmine_node_test rule.