Skip to content
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

Exit collect_coverage.sh early if LCOV_MERGER is not set. #14352

Closed
wants to merge 1 commit into from

Conversation

c-mita
Copy link
Member

@c-mita c-mita commented Nov 30, 2021

Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.

Since all rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.

Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.

Fixes #13978

@c-mita c-mita requested a review from lberki as a code owner November 30, 2021 16:46
@google-cla google-cla bot added the cla: yes label Nov 30, 2021
@c-mita c-mita requested a review from comius November 30, 2021 16:46
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remind me again, why isn't it feasible to add the :lcov_merger attribute to all Starlark test rules?

src/test/shell/bazel/bazel_coverage_starlark_test.sh Outdated Show resolved Hide resolved
Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.

Since _all_ rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.

Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.
@c-mita
Copy link
Member Author

c-mita commented Dec 1, 2021

Remind me again, why isn't it feasible to add the :lcov_merger attribute to all Starlark test rules?

  1. It's a breaking change, since rules that already define _lcov_merger will break.
  2. It breaks things internally; rules within Google work a little differently w.r.t. setting up the lcov_merger tool (which is why the original PR, Provide the default lcov_merger to all Starlark tests #13983, wasn't viable). It's typical for Google's test rules to leave it undefined and let the different collect_coverage.sh handle things.

Other little minor issues arise, but the second point is the big one.

@lberki
Copy link
Contributor

lberki commented Dec 1, 2021

On second thought, there isn't any downside for this change, so LGTM. It would be nice to have an overarching design for adding coverage collection for any language, but that shouldn't be a blocker for this change.

@bazel-io bazel-io closed this in aa52f2d Dec 1, 2021
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Dec 1, 2021
Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.

Since _all_ rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.

Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.

Fixes bazelbuild#13978

Closes bazelbuild#14352.

PiperOrigin-RevId: 413369871
(cherry picked from commit aa52f2d)
meteorcloudy pushed a commit that referenced this pull request Dec 1, 2021
Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.

Since _all_ rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.

Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.

Fixes #13978

Closes #14352.

PiperOrigin-RevId: 413369871
(cherry picked from commit aa52f2d)

Co-authored-by: Charles Mita <[email protected]>
Bencodes pushed a commit to Bencodes/bazel that referenced this pull request Jan 10, 2022
Rules that provide InstrumentedFilesInfo but do not define the
_lcov_merger attribute will result in this script being called without
the required LCOV_MERGER environment variable being set.

Since _all_ rules now provide an InstrumentedFilesInfo provider by
default, but are not required to provide the attribute, this scenario
will be encountered far more frequently and should not be an error.

Because of the way coverage collection within Google differs to Bazel,
it isn't safe to change TestActionBuilder to simply not prepare the
collect_coverage.sh script the way it would if no coverage was to be
collected, so we have to change the script itself.

Fixes bazelbuild#13978

Closes bazelbuild#14352.

PiperOrigin-RevId: 413369871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom test rule without _lcov_merger attribute always fails under bazel coverage
2 participants