Skip to content

Provide the default lcov_merger to all Starlark tests#13983

Closed
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:fix-custom-test-coverage
Closed

Provide the default lcov_merger to all Starlark tests#13983
fmeum wants to merge 1 commit intobazelbuild:masterfrom
fmeum:fix-custom-test-coverage

Conversation

@fmeum
Copy link
Copy Markdown
Collaborator

@fmeum fmeum commented Sep 13, 2021

Since 5b216b2, all Starlark-defined test rules either need to set the _lcov_merger attribute or fail if run with bazel coverage. This is fixed by attaching the default lcov merger as a late-bound default to all Starlark rules via a new attribute and falling back to that attribute in the TestActionBuilder.

This commit ensures backwards compatibility is maintained with existing Starlark test rules that set the _lcov_merger attribute. This is also verified in the included integration test.

Fixes #13978.

@google-cla google-cla bot added the cla: yes label Sep 13, 2021
@fmeum fmeum changed the title Provide the lcov_merger to all Starlark tests Provide the default lcov_merger to all Starlark tests Sep 13, 2021
@fmeum fmeum force-pushed the fix-custom-test-coverage branch from 5d3508b to 737ad9d Compare September 13, 2021 10:08
@fmeum fmeum mentioned this pull request Sep 13, 2021
Closed
@fmeum
Provide the default lcov_merger to all Starlark tests
43191ee 
Since 5b216b2, all Starlark-defined test rules either need to set the
`_lcov_merger` attribute or fail if run with `bazel coverage`. This is
fixed by attaching the default lcov merger as a late-bound default to
all Starlark rules via a new attribute and falling back to that
attribute in the TestActionBuilder.

This commit ensures backwards compatibility is maintained with existing
Starlark test rules that set the `_lcov_merger` attribute. This is also
verified in the included integration test.

Fixes bazelbuild#13978.
@fmeum fmeum force-pushed the fix-custom-test-coverage branch from 737ad9d to 43191ee Compare September 13, 2021 11:55
@fmeum fmeum mentioned this pull request Sep 28, 2021
Closed
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Nov 24, 2021

@oquenchil Would you be able to take a look? The issue this resolves is labeled with team-Rules-CPP.

@fmeum fmeum mentioned this pull request Nov 28, 2021
Closed
9 tasks
@meteorcloudy meteorcloudy requested a review from c-mita November 29, 2021 08:46
@meteorcloudy
Copy link
Copy Markdown
Member

meteorcloudy commented Nov 29, 2021

@c-mita Can you help review this change?

@c-mita
Copy link
Copy Markdown
Member

c-mita commented Nov 29, 2021

Unfortunately, I can't accept this PR as is.
Internally, things are setup a little differently for coverage collection. We use a different collect_coverage.sh script and generally do not set the :lcov_attribute; this change does not play well with the internal setup.

I think the best (only?) way to handle this at the moment is by changing Bazel's collect_coverage.sh to simply do nothing if the LCOV_MERGER variable has not been set.

@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Nov 29, 2021

@c-mita I see the problem. While your proposed solution does resolve the linked issue, it would mean that there would still be no way for Starlark tests to access the merge as a late-bound attribute.

How about the following path forward:

  1. You make the required change to collect_coverage.sh to resolve the release blocking Custom test rule without _lcov_merger attribute always fails under bazel coverage #13978 and I close the current PR.
  2. Someone who is familiar with Google's internal coverage setup provides some guidance on either Expose CoverageOutputGenerator's label in a configuration #8736 or _lcov_merger late-bound test attribute #10642 on how a solution to these issues could look like that would have a chance to be accepted.
  3. I will look into providing such a solution as a separate PR.

@c-mita
Copy link
Copy Markdown
Member

c-mita commented Nov 30, 2021

I'm ok with this approach; I've got a change ready to unblock the build.
It isn't ideal, but it's a way forward.

@c-mita c-mita closed this Nov 30, 2021
@c-mita c-mita mentioned this pull request Dec 1, 2021
Closed
@fmeum fmeum deleted the fix-custom-test-coverage branch January 15, 2022 10:13
@fmeum fmeum mentioned this pull request Jan 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@c-mita c-mita Awaiting requested review from c-mita

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Custom test rule without _lcov_merger attribute always fails under bazel coverage

3 participants

@fmeum @meteorcloudy @c-mita