[Bug]: `js_test` instruments Node for code coverage even if excluded by `--instrumentation_filter`

[gregjacobs]

What happened?

When running tests under bazel coverage (or bazel test --collect_coverage), Node running via js_test is being instrumented regardless of the --instrument_test_targets and --instrumentation_filter flags.

This is causing an issue for us where we have a Jest test suite that is running out of memory when Node is instrumented (using over 16gb somehow). The NODE_V8_COVERAGE environment variable is basically always set when coverage is enabled (https://github.com/aspect-build/rules_js/blob/v1.37.1/js/private/js_binary.sh.tpl#L362), whereas it should really only be set if both coverage is enabled and the target is supposed to be instrumented.

Probably need to use this feature in the js_test rule to determine when to instrument: https://bazel.build/rules/lib/builtins/ctx#coverage_instrumented

And this is actually a great use case for excluding Jest targets from instrumentation because Jest is usually set up to create its own coverage report anyway.

Version

Development (host) and target OS/architectures: Mac, Linux

Output of bazel --version: 6.2.0

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file: [email protected]

Language(s) and/or frameworks involved: JS

How to reproduce

Run bazel coverage //some/package:some_js_test_target --no_instrument_test_targets. Node will still be instrumented (via the NODE_V8_COVERAGE env var) and a coverage report will be generated.

Any other information?

Some docs:

• --instrument_test_targets: https://bazel.build/reference/command-line-reference#flag--instrument_test_targets

• --instrumentation_filter: https://bazel.build/reference/command-line-reference#flag--instrumentation_filter

[alexeagle]

@thesayyn added this coverage support so probably knows it best, but is already quite overcommitted so I don't think we have someone to look into it. Based on your research here do you have an idea what the fix should look like?

[gregjacobs]

@alexeagle Hey man, apologies, missed replying on this. I'll look into it soon and send a PR your way :)

[gregjacobs]

@alexeagle Hey Alex, apologies on the delay on this, but I'm now available and committed to this change!

Question: should I work on this on the 1.x branch or 2.x branch? We'd be crossing our fingers to be able to use this feature asap and we're still on 1.x, so I'm curious if you guys are still releasing the 1.x line because 2.x is still in the rc stage. Otherwise I'll try to update our project to the latest 2.0.0 rc.

Thanks, Greg

[gregjacobs]

Ok so I think this change to respect --instrumentation_filter needs to go into a 2.0.0rc (vs. a 1.x release) because it will be a breaking change.

Currently in 1.x, running bazel coverage alone (as opposed to bazel test) enables v8 coverage for js_test rules.

However, if we want to also respect Bazel's --instrumentation_filter, then rules_js users will also need to enable --instrument_test_targets so that ctx.coverage_instrumented() returns the correct values (so we can determine which targets to add v8 instrumentation to):

Command	ctx.coverage_instrumented()
bazel coverage //:test	False
bazel coverage //:test --instrumentation_filter=":test$"	False
bazel coverage //:test --instrumentation_filter=":test$" --instrument_test_targets	True
bazel coverage //:test --instrumentation_filter=":something_else$" --instrument_test_targets	False
bazel coverage //:test --instrument_test_targets	True

I think it's technically the correct Bazel behavior to require --instrument_test_targets in order to add instrumentation automatically for 'test' targets (as opposed to non-test targets), but what do you guys think? Is this breaking change okay to make in 2.0?