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

Generate coverage reports in LCOV format #3117

Merged
merged 8 commits into from
Apr 22, 2022
Merged

Conversation

fmeum
Copy link
Member

@fmeum fmeum commented Apr 14, 2022

What type of PR is this?

Feature

What does this PR do? Why is it needed?

With the new --//go/config:cover_format flag set to lcov, go cover coverage reports are converted to lcov. When combined with Bazel's --combined_report=lcov, this allows for Go coverage to show up in cross-language, cross-target coverage reports.

The combined report can be converted to HTML via e.g.

genhtml bazel-out/_coverage/_coverage_report.dat

Which issues(s) does this PR fix?

Fixes #140

Other notes for review

I haven't added anything to the user-facing docs yet, but will do so before this is merged.

@fmeum
Copy link
Member Author

fmeum commented Apr 14, 2022

@achew22 Would you be available to review this new feature?

@fmeum fmeum force-pushed the lcov branch 2 times, most recently from 415e74b to 31c45df Compare April 14, 2022 11:25
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

Only went half way through the PR. I will pick it up later this week.

go/tools/builders/compilepkg.go Outdated Show resolved Hide resolved
go/config/BUILD.bazel Outdated Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Show resolved Hide resolved
@fmeum
Copy link
Member Author

fmeum commented Apr 15, 2022

@linzhp I pushed one commit that addresses your comments and another one that removes the branch coverage information - it may be misleading and made the PR unnecessarily complicated.

go/config/BUILD.bazel Outdated Show resolved Hide resolved
@achew22
Copy link
Member

achew22 commented Apr 16, 2022

I think you'll need to update https://github.com/bazelbuild/rules_go/blob/master/tests/core/coverage/coverage_test.go to validate both the lcov and the old (gcov?) coverage formats, but then I'd be happy with this PR.

@fmeum
Copy link
Member Author

fmeum commented Apr 16, 2022

@achew22 Should I also update docs? If so, which ones?

@achew22
Copy link
Member

achew22 commented Apr 16, 2022

I just went through the repo and didn't see any documentation that I think needs to be update. There might need to be some changes made to the nogo coverage test as well, but if it passes I'm happy to merge

@fmeum
Copy link
Member Author

fmeum commented Apr 18, 2022

@achew22 I made the changes to the tests and also updated the message of the first commit.

Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

The logic looks good to me. Thanks for working on this.

go/config/BUILD.bazel Outdated Show resolved Hide resolved
go/config/BUILD.bazel Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Outdated Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Outdated Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Outdated Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Show resolved Hide resolved
@fmeum fmeum changed the title Optionally convert cover report to lcov Generate coverage reports in LCOV format Apr 20, 2022
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

We are getting close

go/tools/bzltestutil/lcov.go Show resolved Hide resolved
go/tools/builders/compilepkg.go Outdated Show resolved Hide resolved
go/tools/builders/generate_test_main.go Outdated Show resolved Hide resolved
go/tools/bzltestutil/lcov.go Outdated Show resolved Hide resolved
Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

LGTM. @achew22 feel free to merge if you don't have more concerns.

go/tools/bzltestutil/lcov.go Outdated Show resolved Hide resolved
The generated TestMain now converts the go cover coverage reports to
LCOV. When combined with Bazel's --combined_report=lcov, this makes Go
coverage show up in cross-language, cross-target coverage reports by
default.

The combined report can be converted to HTML via e.g.

genhtml bazel-out/_coverage/_coverage_report.dat

If the old behavior of generating cover reports is still desired, it
can be enabled via:

--@io_bazel_rules_go//go/config:cover_format="go_cover"
@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

It's strange that lcov cannot be produced in Uber's Go monorepo after applying this onto the master of rules_go:

  1. when I ran bazel coverage with or without --@io_bazel_rules_go//go/config:cover_format=lcov, it doesn't print out the location of coverage.dat.
  2. when I open the coverage.dat, it's empty
  3. when I ran bazel coverage --combined_report=lcov, the resulting bazel-out/_coverage/_coverage_report.dat is empty too
  4. bazel coverage --@io_bazel_rules_go//go/config:cover_format=go_cover works

@fmeum
Copy link
Member Author

fmeum commented Apr 20, 2022

@linzhp Did you try this with bazel test //...? Does it work for a smaller subset of tests?

@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

I only try it in one single test (running bazel test //... in Uber's Go monorepo would blow up my laptop).

@fmeum
Copy link
Member Author

fmeum commented Apr 20, 2022

Depending on the source structure, the instrumentation filter may be off. Do you get reports with --instrument_test_targets --instrumentation_filter=^//?

@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

The instrumentation filter is on:

bazel coverage --@io_bazel_rules_go//go/config:cover_format=lcov //some/internal/path:all
INFO: Writing tracer profile to '/var/folders/j3/f9m101qs7n1bxh0v__vkxwgm0000gn/T/bazel_20220420101656_2zytq'
INFO: Invocation ID: 511beacc-ebf1-45cb-97ae-612b4271f93d
INFO: Using default value for --instrumentation_filter: "^//some/internal/path[/:]".
INFO: Override the above default with --instrumentation_filter
INFO: Build options --@io_bazel_rules_go//go/config:cover_format and --instrumentation_filter have changed, discarding analysis cache.
INFO: Analyzed 4 targets (0 packages loaded, 11096 targets configured).
INFO: Found 3 targets and 1 test target...
INFO: Elapsed time: 0.550s, Critical Path: 0.08s
INFO: 11 processes: 10 disk cache hit, 1 internal.
INFO: Build completed successfully, 11 total actions
//some/internal/path:go_default_test (cached) PASSED in 0.0s

Executed 0 out of 1 test: 1 test passes.

I also tried:

bazel coverage --instrument_test_targets --instrumentation_filter=^// --@io_bazel_rules_go//go/config:cover_format=lcov //some/internal/path:all
INFO: Invocation ID: 170ef37d-ea63-4e0b-82e6-178ee691ca2f
INFO: Build options --instrument_test_targets and --instrumentation_filter have changed, discarding analysis cache.
INFO: Analyzed 4 targets (0 packages loaded, 11096 targets configured).
INFO: Found 3 targets and 1 test target...
INFO: Elapsed time: 1.883s, Critical Path: 1.47s
INFO: 11 processes: 2 internal, 9 darwin-sandbox.
INFO: Build completed successfully, 11 total actions
//some/internal/path:go_default_test    PASSED in 0.3s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 11 total actions

@fmeum
Copy link
Member Author

fmeum commented Apr 20, 2022

Which Bazel version are you using at Uber? Could you share any bits of the test log that do not originate in your own code? Maybe there is some helpful output there.

@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

We are using Bazel 5.1.0. The test.log is like:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //some/internal/path:go_default_test
-----------------------------------------------------------------------------
PASS
coverage: 61.4% of statements

The testlog directory of that test is like:

total 24
-r-xr-xr-x  1 zplin  wheel    0 Apr 20 09:44 baseline_coverage.dat
-r-xr-xr-x  1 zplin  wheel    0 Apr 20 10:39 coverage.dat
-r-xr-xr-x  1 zplin  wheel  228 Apr 20 10:39 test.cache_status
-r-xr-xr-x  1 zplin  wheel  248 Apr 20 10:39 test.log
drwxr-xr-x  2 zplin  wheel   64 Apr 20 10:39 test.outputs
drwxr-xr-x  2 zplin  wheel   64 Apr 20 10:39 test.outputs_manifest
drwxr-xr-x  2 zplin  wheel   64 Apr 20 10:39 test.raw_splitlogs
-rw-r--r--  1 zplin  wheel  165 Apr 20 10:39 test.xml

@fmeum
Copy link
Member Author

fmeum commented Apr 20, 2022

@linzhp Could you post the coverage.dat you get from a run with go_cover format? I think it's most likely that the conversion fails to handle some cases and/or that the code that translates import paths to repo-relative source file paths has issues.

@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

Are you on Bazel Slack?

@linzhp
Copy link
Contributor

linzhp commented Apr 20, 2022

I also tried removing this if statement and write to lcov unconditionally, but still got nothing in the coverage.dat.

@fmeum
Copy link
Member Author

fmeum commented Apr 21, 2022

A problem is the func TestMain(m *testing.M) { os.Exit(m.Run()) } pattern, as demonstrated by the failing integration test. The coverage profile is written at the end of m.Run(), but os.Exit() does not run deferred functions. I found a way to hack around this problem.

@fmeum fmeum force-pushed the lcov branch 2 times, most recently from eed987d to 2f3f3bb Compare April 21, 2022 15:47
@linzhp
Copy link
Contributor

linzhp commented Apr 21, 2022

The hack works in Uber. @achew22 @robfig @blico thoughts on the hack to support os.Exit(m.Run())? Without the hack, lcov will not work for old Go versions that don't call os.Exit automatically in TestMain, but the hack may break in future Go versions, requiring additional maintenance.

@fmeum
Copy link
Member Author

fmeum commented Apr 21, 2022

Just a note: The current minimum supported version of Go with rules_go seems to be 1.16 as cover.go doesn't build with Go 1.15. That aligns well with the hack, which works with Go 1.16+ since it requires the paniconexit0 flag.

@linzhp
Copy link
Contributor

linzhp commented Apr 21, 2022

TestMain no longer required calling os.Exit since Go 1.15: https://go.dev/doc/go1.15.

@achew22
Copy link
Member

achew22 commented Apr 21, 2022

WRT the test.paniconexit0 flag and the comment

// 1. It attains parity with "go test", which enables this feature by default.

Curious about this comment. Do you have a link to go with it? I'd like to see what go test is doing.

@fmeum
Copy link
Member Author

fmeum commented Apr 21, 2022

WRT the test.paniconexit0 flag and the comment

// 1. It attains parity with "go test", which enables this feature by default.

Curious about this comment. Do you have a link to go with it? I'd like to see what go test is doing.

It got added in golang/go@4f76fe8

@linzhp
Copy link
Contributor

linzhp commented Apr 22, 2022

@fmeum Can you review @abhinav 's improvement on this PR?

@abhinav
Copy link
Contributor

abhinav commented Apr 22, 2022

Thanks for that, @linzhp. I forgot to press the button posting this:

Per offline discussion with @linzhp, I made some edits to this PR in fmeum#1.
@fmeum and @linzhp, when you have a chance, please have a look.

This makes it implementation specific but obviates the need to have a
copy of the testDeps interface.

While at it, rename PatchTestDeps to LcovTestDeps since this is
lcov-specific.
We don't need this interface if we can use testdeps.TestDeps directly.
rename and move to the bottom of the file
The comment stated that the function was called with 'true' before m.Run
returns, but it's the opposite: it's called with true before running
tests, and with false after.
@fmeum
Copy link
Member Author

fmeum commented Apr 22, 2022

Thanks for the great improvements, @abhinav. I somehow thought that the testdeps package couldn't be referenced from a non-main package since it's under internal, but apparently that's just not true.

@linzhp linzhp merged commit c142604 into bazel-contrib:master Apr 22, 2022
@fmeum fmeum deleted the lcov branch April 22, 2022 21:48
copybaranaut pushed a commit to pixie-io/pixie that referenced this pull request Oct 5, 2022
Summary:
rules_go reports coverage in LCOV format by default as of version 0.32.0
See bazel-contrib/rules_go#3146 and bazel-contrib/rules_go#3117
So the coverage script was no longer doing the correct thing. This fixes the script.

Test Plan: Ran the coverage script locally, generated an HTML report. Ensured that we see cpp, go and js coverage.

Reviewers: zasgar, michelle

Reviewed By: zasgar

Signed-off-by: Vihang Mehta <[email protected]>

Differential Revision: https://phab.corp.pixielabs.ai/D12345

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

Successfully merging this pull request may close these issues.

Coverage of go test
4 participants