search

aspect-build / rules_js

High-performance Bazel rules for running Node.js tools and building JavaScript projects
https://docs.aspect.build/rules/aspect_rules_js
Apache License 2.0
304 stars 105 forks source link

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

Open gregjacobs opened 8 months ago

gregjacobs commented 8 months ago

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:

alexeagle commented 8 months ago

@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 commented 6 months ago

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

gregjacobs commented 2 months ago

@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 commented 2 months ago

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?