move cov to nodejs test

[Toxicable]

This moves coverage support from jasmine_node_test to nodejs_binary, meaning any test can collect coverage.
It also
This does not report any coverage to the user, it only collects coverage and stores it in V8's format somewhere in a tmpdir that's not reasonable accessible.
This also allows support for worker threads and process coverage collection.

The next step from here would be to do more integration work with bazel coverage but this lays down the ground work for collecting coverage correctly .

PR Checklist

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

PR Type

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

What is the current behavior?

Code coverage can only be collected via the jasmine_node_test rule

Issue Number: N/A

What is the new behavior?

Code coverage can be collected on run rule using nodejs_test

Does this PR introduce a breaking change?

• Yes

removes the coverage attribute, instead relying on the command being invoked with bazel coverage.

Other information

fixes #1218

[buildsize]

File name	Previous Size	New Size	Change
release.tar.gz	967.47 KB	970.1 KB	2.63 KB (0%)

[alexeagle]

@iirina any progress on letting third-party starlark rules participate in bazel coverage?

[alexeagle]

coverage only applies to test right? bazel gives you $TEST_TMPDIR but you should wait until you're inside the test action to try to read the environment

[Toxicable]

While only doing coverage under bazel test makes sense, we often run out test targets with bazel run when we're developing them, simply becuase it's the same command we use to run our nodejs targets.
However, if there's no way to get access to a tmpdir under bazel run then we'll have to do that, we'll have to ensure this is documented

[Toxicable]

Also note even with the bazel coverage integration, we'd still need this set since the v8 coverage format is only an intermediate format

[Toxicable]

since we're only doing coverage with bazel coverage as per the below comment, I changed this to be env_vars += "export NODE_V8_COVERAGE=$TEST_TMPDIR/$RANDOM\n" - not sure if $RANDOM is correct but it's close if not

[Toxicable]

@soldair Should be good for a review now.
I've reduced the scope on this to only be the brekaing changes, ill make another PR that adds in the coverage processing script.

[soldair]

lgtm

[Toxicable]

@alexeagle On this one do you want at least feature parity with jasmine_node_test#coverage=True
Before we merge this?

One path we could take:

• keep v8-coverage to do that but that means we don't fix @bazel/jasmine 0.38 depends on mem@1.1 which has a security issue #1218
• remove or deprecate the coverage attribute
• enabled the flag --coverage for jasmine_runner.js to be based off ctx.configuration.coverage_enabled instead of the attribute.

This only solve the issue about the breaking change of removing (or maybe deprecating) the coverage attribute.
Further work needs to be done to remove v8-coverage explained in detail more here: #931 (comment)
Also relates here: #867

TLDR:

• need Skylark support for aggrigating coverage reports accross targets
• need source map support to swap v8-coverage for c8 or even v8-to-istanbul

[Toxicable]

Took another look at this today
I was looking further at using bazel coverage for coverage aggregation.

Main issue is that COVERAGE_DIR isn't defiend when running bazel coverage
I think if that's defined then we can set NODE_V8_COVERAGE=$COVERAGE_DIR
Doing this would cause v8 to write it's coverage to this dir, then if we defined out LCOV_MERGE, it could collect all this v8 format coverage, convert it and aggregate it.

Here's a bunch of links to related topics/code that I found which I'll probably look at again next time.

https://github.com/bazelbuild/bazel/blob/f708b0f847ba6c1d2ccfd33c283a2dd81e0803e6/site/docs/skylark/rules.md#code-coverage-instrumentation
https://github.com/dropbox/dbx_build_tools/blob/3f68abae62135488e135af145a0b48e9030488ff/build_tools/py/py.bzl#L841
bazelbuild/bazel#10660
https://docs.google.com/document/d/1WRQTXQBV-m1_HDkAPvRSfGIrDW4pkI7ATPWtNdC8k9E/edit#heading=h.pt2fqnujvoh2
https://groups.google.com/forum/#!topic/bazel-discuss/LC97zBrMG84
https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_cc_coverage.sh
https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh
bazelbuild/bazel#6293
bazelbuild/bazel#8670
bazelbuild/bazel#5885

[Toxicable]

Progress update; TLDR: closer but still not quite there

Along with a huge help from @armooo
We were able to somewhat get bazel coverage working.

In it's current state:
Bazel runs the program with https://github.com/bazelbuild/bazel/blob/master/tools/test/collect_coverage.sh
Which gives us the env vars we need to collect coverage from targets.
The test target runs as expected and v8 writes it's coverage information.
Then collect_coverage tries to run out lcov_merger but runfiles fails and it exits without anything helpful.

I've added a new binary: lcov_merger which will be responsible for colelcting v8 coverage from a target, merging and converting to lcov for bazel to use later on.

Assuming we can solve the issues above we should be able to use --combined_report=lcov to produce a single coverage report at bazel-out/_coverage/_coverage_report.dat

We need to use the coverage_common.instrumented_files_info provider which i've added to nodejs_binary and ts_library, it will need to be added to any rule that provides source files which need to be instrumented

Once we have lcov_merger running we may run into some source map issues.

If you want to test it out yourself run npx bazel coverage packages/jasmine/test:coverage_test on this branch

[Toxicable]

This looks like the issue causing the runfiles to fail bazelbuild/bazel#4033

[Toxicable]

I've changed tac on tthis one a bit, i've moved the logic into the jasmine runner for combining and converting to lcov.
It's less portable doing it this way, since we'd need to implement it for each runner, but at least this provides a example.

However; it requires some hacks to c8 and v8-to-istanbul, around source maps, to get it working i've modified them manually.
So ill need to work on fixing those issues and maybe upstreaming something there.

Aside from that this now work with bazel coverage, however all the outpout from collect_coverage.sh is pretty annoying, might look seeing if we can turn off the set -x that it does.

Next piece is to figure out how with bazel we can get it to extract the coverage data out, it looked like we can use --combined-report=lcov but that resulted in an error

Internal error thrown during build. Printing stack trace: java.lang.NullPointerException
        at com.google.devtools.build.lib.bazel.coverage.BazelCoverageReportModule$1.getArgs(BazelCoverageReportModule.java:115)
        at com.google.devtools.build.lib.bazel.coverage.CoverageReportActionBuilder.generateCoverageReportAction(CoverageReportActionBuilder.java:246)
        at com.google.devtools.build.lib.bazel.coverage.CoverageReportActionBuilder.createCoverageActionsWrapper(CoverageReportActionBuilder.java:193)
        at com.google.devtools.build.lib.bazel.coverage.BazelCoverageReportModule$1.createCoverageReportActionsWrapper(BazelCoverageReportModule.java:97)

looks like there's an issue for it bazelbuild/bazel#6450

[Toxicable]

by not using exec do we still handle "stdin, stdout, signals, etc" still?

[Toxicable]

also if we're ditching exec could we compbine this with the one belonw, Line 260

[gregmagolan]

Yes. I think combining the two is reasonable. I don't think there is much value in running coverage when expected exit code != 0 but maybe someone will find that useful and it would simplify the code

[gregmagolan]

Good point re: the exec. We should look back at why the exec was there (trace back to the commit where it was added and see what it was fixing as I don't recall) and if there is anything special needed to pipe the io streams & signals. The non-exec non-zero exit code path had no special handling but that path is likely not used at all outside of this repo.

[Toxicable]

This looks like where it was introduced c124496

[Toxicable]

Looks like w'ell need to go back to the trap approah to handle SIGTERM, SIGINT, SIGQUIT. will throw that together and see what it looks like with that other refactor in

[gregmagolan]

Thanks @Toxicable . Was just looking at that commit and came to the same conclusion.

[Toxicable]

Modified it based off your changes and the ones in that MR, please check it well, my bash skills are not amazing

[Toxicable]

Just pushed up a few changes, please take another review @gregmagolan

1. I tried looking at the implementation of coverage_common but it's defiend in Java here https://github.com/bazelbuild/bazel/blob/7891d3b4be8549f762f32308a4085f802e7608e5/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/test/CoverageCommonApi.java
   But my wild guess is that we don't need to specify it on the ts_library since all the files are accessible through the data attribute?
   I think we should leave it off since it appears to work, but if we find a case where we need to add it, we can add it back then

2. My feeling here is that until we have a better solution we just map everything to .js, once we figure out what use cases this dosen't satisfy we could look at options here.

3. I think leaving this as is for now is going to be fine, we could revisit the API for that on a follow up.

[Toxicable]

@googlebot I consent.

[gregmagolan]

@Toxicable Can you plz rebase this to head? We can land this now as master is on the 2.0.0-next.0 track now.

[Toxicable]

@gregmagolan done!

[Toxicable]

CI failure looks like we broke something, not sure

packages/typescript/test/lit_plugin/bad.ts:9:4 - error TS2322: [lit] Unknown tag <unknown-element>. Did you mean <lit-element>?
  Check that you've imported the element, and that it's declared on the HTMLElementTagNameMap. If it can't be imported, consider adding it to the 'globalTags' plugin configuration or disabling the 'no-unknown-tag' rule.

9   <unknown-element></unknown-element>
     ~~~~~~~~~~~~~~~
//packages/typescript/test/lit_plugin:lit_plugin:1:1 - error TS2322: Expected a compilation error matching "TS2322: \\[lit\\] Unknown tag \"unknown-element\". Did you mean 'lit-element'?"

am able to reproduce it locally

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[gregmagolan]

Turns out that the failures you saw were from upgrading npm packages such as terser and ts-lit-plugin and rollup* to newer versions. Behavior of these npm packages changed slightly which causes some tests to fail. You must have turned your yarn.lock and regenerated it from scratch and the packages were marked ^x.x.x.

@alexeagle We should turn on renovate for npm packages as that would catch npm package upgrade failures. I tried configuring renovate to make npm update PRs a little while ago but still not working.

[gregmagolan]

In any case, this should go green now.

[alexeagle]

Thanks @Toxicable for giving coverage so much love!