Support integration test coverage system (coverageredesign)#4397
Support integration test coverage system (coverageredesign)#4397fmeum merged 2 commits intobazel-contrib:masterfrom
Conversation
fmeum
left a comment
There was a problem hiding this comment.
This looks great, thanks for picking up this long overdue to-do!
|
I'll review as soon as I can, but I probably won't have time until Friday, so sorry in advance for the delay! |
fd21cd4 to
45c71ae
Compare
45c71ae to
212bac4
Compare
|
@fmeum, another commit i'll need to add on top is setting the covermode to atomic if we're building outside of tests as https://pkg.go.dev/runtime/coverage#WriteCounters only works for coverage generated in that mode and that's the only non-test hook that I think we have. I was thinking of updating the go_context in the go_binary rule to mark that covered packages were non test related and use that info to set the |
|
Do you happen to know how go test gets around this requirement? Do you know how much slower atomic mode is? |
ref: I'm not sure of the performance differences but it seems like Go doesn't want this to happen for correctness concerns since non-test programs are likely to run concurrently. Their concern is documented in the ClearCounters API. Update: I asked Gemini to generate me sample benchmark code that I built with atomic vs set covermode and the result doesn't seem widely different, maybe slightly (< 10%) more expensive for atomic but this is not very rigorous. That said, existing uses of the covdata struct are probably unsafe as well and any user of non-test integration test coverage should assume their code will not be as performant. |
jayconrod
left a comment
There was a problem hiding this comment.
Sorry to take so long to review.
This looks really good. Thanks for picking this up. I don't really have feedback to add beyond what @fmeum has already said. This looks to be very close to what the go command does, which is mainly what I'm looking for.
|
I would say we go with atomic everywhere. Coverage runs are slow anyway and having them return accurate results is far more important than time savings. Different behavior for tests vs binaries would also be unexpected. |
|
@fmeum, sounds good. I just pushed a commit to this branch that enables atomic mode for all coverage cases. |
68f4881 to
e0dcb63
Compare
fmeum
left a comment
There was a problem hiding this comment.
Thanks a lot for figuring this out, I only left a few minor comments suggesting final cleanups before we can merge this.
051e384 to
0e70889
Compare
go1.25 removes the current runtime code coverage system adopted by rules_go (nocoverageredesign) https://go-review.googlesource.com/c/go/+/644997 which makes rules_go incompatible with the latest upcoming version of go. The change completely adopts the new integration test friendly coverageredesign system (https://go.dev/blog/integration-test-coverage) in rules_go. Below I describe how the current rules_go coverage system currently works, what changes in the new coverage redesign, and what needs to change as a result in rules_go. How nocoverageredesign currently works: - rules_go invokes `go tool cover` to generate instrumented source files that are then passed to the Go compiler (in compilepkg.go). Instrumented source files update generated package variable counters. - The rules_go generated test main (generate_test_main.go) calls testing.RegisterCover to set a global variable that tracks references to coverage generated package local variables that coverage instrumented code modify (ref: https://github.com/golang/go/blob/go1.24.5/src/testing/cover.go#L72) This replicates the behavior in go test's generated test main (cmd/go/internal/load/test.go) - rules_go specially adds a call to coverdata.RegisterFile call to all coverage instrumented files which connects each of the file coverage counter variables with a global object and enables rules_go to customize the file name that coverage variables are associated with. - It's important to note that because rules_go accesses to this global coverage struct, it's able to manipulate the file path name associated with coverage counter variables. This is important because the lcov format that bazel uses expect exec root relative files. - When a test exits, This global object is flushed in a call to `testing.coverReport()` to generate a coverage report. This reports gets written a file configured in test.coverprofile flag. - Via a special SetPanicOnExit0 hook, the coverage file set in test.coverprofile is coverted to a Bazel friendly Lcov format via logic in bzltestutil/lcov.go What changes in coverageredesign: - A new flexible system for configuring coverage behavior. Instead of a single esting.RegisterCover function, there is a new testdeps package that exposes hooks to configure behavior. - The global variable (https://github.com/golang/go/blob/go1.24.5/src/testing/cover.go#L72) that tracked all coverage data is completely removed. Instead, functions in https://pkg.go.dev/internal/coverage/cfile that are called when writing coverage reports on test exit are internally aware of and able to access coverage counter variable data. - Functionality to flush coverage counter variables write to an intermediate coverage directory in a binary format. Hooks configured in the testdeps package are responsible for translating this binary format into a human readable coverage output file. - 'go tool cover' takes a new package based configuration "pkgcfg" option that enables this new coverage instrumentation logic in coverage source files. - Coverage counters can be flushed on demand via the APIs of "runtime/coverage" This allows coverage to work outside of test situations where coverage used to rely on exit hooks generated into test mains to flush and write coverage data. What needs to change in rules_go as a result: - Use the new testdeps configuration system in the rules_go generated test main. We can following the example of the go test generated test main. ref: https://github.com/golang/go/blob/go1.24.5/src/cmd/go/internal/load/test.go#L986 - Invoke 'go tool cover' with the new -pkgcfg flag. We can follow the example of how the go toolchain invokes coverage redesign instrumentation in https://github.com/golang/go/blob/go1.24.5/src/cmd/go/internal/work/exec.go#L1932 - Not strictly necessary but removes technical debt, call bzltestutil.ConvertCoverToLcov inside of testdeps.CoverProcessTestDirFunc hook wrapper instead of the bzltestutil.LcovTestDeps SetPanicOnExit0 hook. - Adjust the output of lcov profile to make file path keys in coverage files execution root relative. Implementation Note: Unfortunately, we still need github.com/bazelbuild/rules_go/go/tools/coverdata dependencies in coverage source files because the lcov coverage format expects file paths associated with coverage data to be execution root relative. coverageredesign writes Go coverage file data in the format of `importpath/file_name` which is not execution root (e.g. `src/`) relative. During compilation is the only place we have the information to map `importpath>/<file_name` to an execution root relative path so we have to continue to use the trick of generating code that corrects the file path coverage mapping at runtime. This makes it hard for coverage to work outside of tests because depend on a special github.com/bazelbuild/rules_go/go/tools/coverdata dependency that can't be guaranteed because we don't control a generated test main in these situations. If Go coverage filepaths can be configured during invocations of the `go tool cover`, we can break this dependency. I've filed an upstream go ticket to descrbe this issue golang/go#74749. ref bazel-contrib#3513
coverageredesign enables the use of "runtime/coverage" which are APIs designed to enable expose runtime coverage data in non test programs. "runtime/coverage" explicitly only works with "atomic" covermode ref: https://github.com/golang/go/blob/6fbad4be75e7746512bbe55794694ed788ea5c5b/src/internal/coverage/cfile/apis.go#L39 In this light, always invoke `go tool cover` with covermode atomic as it would be unxpected for test and non-test coverage to behave differently. ref bazel-contrib#3513
0e70889 to
b530116
Compare
This version includes bazel-contrib/rules_go#4397 which addresses the build fialure we were seeing.
coverageredesign (bazel-contrib#4397) honors line directives while the world before coverageredesign did not. This is relevant in situations where you want coverage for generated code to map to source code in a user's workspace. An example of this is a go_library rule which takes a dependency on a code generator which generates a replacement file for a user source file. The replacement file is run in the final binary but the user wants to see covarege for the source file that was an input to the generated replacement file. ``` go_library( name = "go_default_library", srcs = [ "batch.go", "consumers.go", "edges.go", "token.go", ":codegen", ], ) codegen(name = "codegen, ...) ``` The Go compiler, because of Bazel, only knows about the generated code but coverage is only relevant for the source file that the code generator replaces. Given a generated file `bazel-out/k8-fastbuild/bin/src/example.org/gen_/report_gen.go` nocoverageredesign would emit that file path in coverage files. With coverageredesign, if a line directive in the generated code changes the file name to hello.go (e.g. //line hello.go:10) then the path in the runtime emitted coverage file would be `example.org/gen_/hello.go` which is not execution root relative and would be ignored by Bazel. The prevent this code from breaking in coverage redesign, we need to map the line directive file name to the execution root relative filepath that Bazel respects to get coverage for these types of files. This change extends the "srcPathMapping" trick used in https://github.com/bazel-contrib/rules_go/pull/4397/files to honor a mapping of line directive file names to map back to exec root relative paths that Bazel understands and requires for lcov mode.
|
@r-hang Could you remind me what's left for us to do so that go_binary can report coverage in non-Go test rules? |
This should work already. Here's an example #3513 (comment) In non-test test code there isn't a test main to always hook into so I declared the coverage data emission manually. |
go1.25 removes the current runtime code coverage system adopted by
rules_go (nocoverageredesign) https://go-review.googlesource.com/c/go/+/644997
which makes rules_go incompatible with the latest upcoming version of go.
The change completely adopts the new integration test friendly coverageredesign
system (https://go.dev/blog/integration-test-coverage) in rules_go.
Below I describe how the current rules_go coverage system currently works, what
changes in the new coverage redesign, and what needs to change as a result in
rules_go.
How nocoverageredesign currently works:
rules_go invokes
go tool coverto generate instrumented source files thatare then passed to the Go compiler (in compilepkg.go). Instrumented source
files update generated package variable counters.
The rules_go generated test main (generate_test_main.go) calls
testing.RegisterCover to set a global variable that tracks references to
coverage generated package local variables that coverage instrumented code
modify (ref: https://github.com/golang/go/blob/go1.24.5/src/testing/cover.go#L72)
This replicates the behavior in go test's generated test main (cmd/go/internal/load/test.go)
rules_go specially adds a call to coverdata.RegisterFile call to all coverage
instrumented files which connects each of the file coverage counter variables
with a global object and enables rules_go to customize the file name that
coverage variables are associated with.
It's important to note that because rules_go accesses to this global coverage struct,
it's able to manipulate the file path name associated with coverage counter variables.
This is important because the lcov format that bazel uses expect exec root relative files.
When a test exits, This global object is flushed in a call to
testing.coverReport()togenerate a coverage report. This reports gets written a file configured in
test.coverprofile flag.
Via a special SetPanicOnExit0 hook, the coverage file set in test.coverprofile
is coverted to a Bazel friendly Lcov format via logic in bzltestutil/lcov.go
What changes in coverageredesign:
A new flexible system for configuring coverage behavior. Instead of a single
esting.RegisterCover function, there is a new testdeps package that exposes hooks
to configure behavior.
The global variable (https://github.com/golang/go/blob/go1.24.5/src/testing/cover.go#L72)
that tracked all coverage data is completely removed. Instead, functions
in https://pkg.go.dev/internal/coverage/cfile that are called when
writing coverage reports on test exit are internally aware of and able
to access coverage counter variable data.
Functionality to flush coverage counter variables write to an intermediate
coverage directory in a binary format. Hooks configured in the testdeps package
are responsible for translating this binary format into a human readable coverage
output file.
'go tool cover' takes a new package based configuration "pkgcfg" option that
enables this new coverage instrumentation logic in coverage source
files.
Coverage counters can be flushed on demand via the APIs of "runtime/coverage"
This allows coverage to work outside of test situations where coverage
used to rely on exit hooks generated into test mains to flush and write
coverage data.
What needs to change in rules_go as a result:
Use the new testdeps configuration system in the rules_go generated test main.
We can following the example of the go test generated test main.
ref: https://github.com/golang/go/blob/go1.24.5/src/cmd/go/internal/load/test.go#L986
Invoke 'go tool cover' with the new -pkgcfg flag. We can follow the example of
how the go toolchain invokes coverage redesign instrumentation in
https://github.com/golang/go/blob/go1.24.5/src/cmd/go/internal/work/exec.go#L1932
Not strictly necessary but removes technical debt, call bzltestutil.ConvertCoverToLcov
inside of testdeps.CoverProcessTestDirFunc hook wrapper instead of the
bzltestutil.LcovTestDeps SetPanicOnExit0 hook.
Adjust the output of lcov profile to make file path keys in coverage files
execution root relative.
Implementation Note:
Unfortunately, we still need github.com/bazelbuild/rules_go/go/tools/coverdata
dependencies in coverage source files because the lcov coverage format
expects file paths associated with coverage data to be execution root
relative. coverageredesign writes Go coverage file data in the format of
importpath/file_namewhich is not execution root (e.g.src/) relative.During compilation is the only place we have the information to map
importpath/file_nameto an execution root relative path so we haveto continue to use the trick of generating code that corrects the
file path coverage mapping at runtime.
This makes it hard for coverage to work outside of tests because depend on a special
github.com/bazelbuild/rules_go/go/tools/coverdata dependency that can't be
guaranteed because we don't control a generated test main in these situations.
If Go coverage filepaths can be configured during invocations of the
go tool cover, we can break this dependency. I've filed an upstream go ticketto descrbe this issue golang/go#74749.
ref #3513