-
Notifications
You must be signed in to change notification settings - Fork 522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move cov to nodejs test #1135
move cov to nodejs test #1135
Conversation
This comment has been minimized.
This comment has been minimized.
3b565be
to
a444b3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iirina any progress on letting third-party starlark rules participate in bazel coverage?
internal/node/node.bzl
Outdated
@@ -155,6 +155,10 @@ def _nodejs_binary_impl(ctx): | |||
if k in ctx.var.keys(): | |||
env_vars += "export %s=\"%s\"\n" % (k, ctx.var[k]) | |||
|
|||
if ctx.attr.coverage: | |||
# TODO: use proper path for tmpdir | |||
env_vars += "export NODE_V8_COVERAGE=/tmp/$RANDOM\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note even with the bazel coverage
integration, we'd still need this set since the v8 coverage format is only an intermediate format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
4f58839
to
632c3ff
Compare
415ae84
to
0c9713d
Compare
@soldair Should be good for a review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
0c9713d
to
b7b3c64
Compare
@alexeagle On this one do you want at least feature parity with One path we could take:
This only solve the issue about the breaking change of removing (or maybe deprecating) the TLDR:
|
b7b3c64
to
11a40f6
Compare
Progress update; TLDR: closer but still not quite there Along with a huge help from @armooo In it's current state: I've added a new binary: Assuming we can solve the issues above we should be able to use We need to use the Once we have If you want to test it out yourself run |
This looks like the issue causing the runfiles to fail bazelbuild/bazel#4033 |
bc9a059
to
18f33a0
Compare
I've changed tac on tthis one a bit, i've moved the logic into the jasmine runner for combining and converting to lcov. However; it requires some hacks to c8 and v8-to-istanbul, around source maps, to get it working i've modified them manually. Aside from that this now work with 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
looks like there's an issue for it bazelbuild/bazel#6450 |
c426285
to
c921f4b
Compare
# handled by the node process. | ||
# If we had merely forked a child process here, we'd be responsible | ||
# for forwarding those OS interactions. | ||
exec "${node}" "${LAUNCHER_NODE_OPTIONS[@]:-}" "${USER_NODE_OPTIONS[@]:-}" "${MAIN}" ${ARGS[@]+"${ARGS[@]}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by not using exec do we still handle "stdin, stdout, signals, etc" still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also if we're ditching exec
could we compbine this with the one belonw, Line 260
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like where it was introduced c124496
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Toxicable . Was just looking at that commit and came to the same conclusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified it based off your changes and the ones in that MR, please check it well, my bash skills are not amazing
5a35eb8
to
7c6aae7
Compare
Just pushed up a few changes, please take another review @gregmagolan
|
7c6aae7
to
bdd7e76
Compare
@googlebot I consent. |
bdd7e76
to
a57d890
Compare
@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. |
a57d890
to
34601c6
Compare
@gregmagolan done! |
9055b31
to
3c4efec
Compare
CI failure looks like we broke something, not sure
am able to reproduce it locally |
3c4efec
to
a9f3d0f
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
59586ab
to
6d11fae
Compare
BREAKING CHANGE: jasmine_node_test not longer has the `coverage` attribute
6d11fae
to
cf7dbd5
Compare
Turns out that the failures you saw were from upgrading npm packages such as @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. |
In any case, this should go green now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Toxicable for giving coverage so much love!
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
PR Type
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?
removes the
coverage
attribute, instead relying on the command being invoked withbazel coverage
.Other information
fixes #1218