[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: rules_js@1.37.1

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 :)