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

Repoint --coverage_report_generator default to CoverageOutputGenerator #10403

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Dec 11, 2019

From #6450 (comment):

The default value of the --coverage_report_generator flag is broken and we
didn't get to fixing it. It should be an easy change though, PRs are always
welcome. :)

This change does that, but using
@bazel_tools//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Main.
(CoverageOutputGenerator supersedes LcovMerger per #7412.)

Resolves #6450.

From #6450 (comment):

> The default value of the --coverage_report_generator flag is broken and we
> didn't get to fixing it. It should be an easy change though, PRs are always
> welcome. :)

This change does that, but using
@bazel_tools//tools/test/CoverageOutputGenerator/java/com/google/devtools/coverageoutputgenerator:Main.
(CoverageOutputGenerator supersedes LcovMerger per #7412.)

Resolves #6450.
@ulfjack
Copy link
Contributor

ulfjack commented Dec 11, 2019

Hi Ryan, thanks for the patch. I have been looking at issues around coverage for the last few days, and I think we might want to move in the opposite direction. I have a change to redirect everything to @remote_coverage_tools//:lcov_merger and @remote_coverage_tools//:coverage_report_generator. That'll make it easier for users to replace these tools, e.g., by setting --override_repository to provide a different @remote_coverage_tools repository.

My change is here:
https://bazel-review.googlesource.com/c/bazel/+/123156

Unfortunately, there's still a problem which I haven't had the time to debug yet. WDYT?

@ulfjack ulfjack self-requested a review December 11, 2019 23:36
@ulfjack ulfjack self-assigned this Dec 11, 2019
@ghost
Copy link
Author

ghost commented Dec 12, 2019

Thanks for taking a look.

My change is here:
https://bazel-review.googlesource.com/c/bazel/+/123156

Unfortunately, there's still a problem which I haven't had the time to debug yet. WDYT?

As an outsider, I'm not in a good position to weigh in on the @remote_coverage_tools topic. After reading #4685, it seems like the right thing to do, and so I can close this PR.

@ghost ghost closed this Dec 12, 2019
@ghost ghost deleted the beasleyr-vmw/6450-coverage_report_generator_new_default branch December 12, 2019 13:43
@ghost
Copy link
Author

ghost commented Dec 12, 2019

I had a peek at the Gerrit link, and while it indicated a merge conflict I was able to cleanly apply to master @ b494074.

@ulfjack
Copy link
Contributor

ulfjack commented Dec 12, 2019

Yeah, we configured Gerrit to always prevent merges. Did you try the patch? Does it work?

@ghost
Copy link
Author

ghost commented Dec 12, 2019

Yeah, we configured Gerrit to always prevent merges. Did you try the patch? Does it work?

Ah, gotcha. As far as #6450 is concerned, yes, the patch works. (I can use --combined_output=lcov w/o specifying any add'l flags, and IntelliJ's coverage UI works w/ a vanilla .bazelrc.)

This pull request was closed.
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.

Bazel coverage with --combined_report=lcov fails
3 participants