Skip to content

Allow collecting coverage for go_binary executed as child process#4461

Merged
fmeum merged 6 commits intobazel-contrib:masterfrom
dzbarsky:coverage
Oct 8, 2025
Merged

Allow collecting coverage for go_binary executed as child process#4461
fmeum merged 6 commits intobazel-contrib:masterfrom
dzbarsky:coverage

Conversation

@dzbarsky
Copy link
Copy Markdown
Contributor

@dzbarsky dzbarsky commented Oct 1, 2025

What type of PR is this?
feature

What does this PR do? Why is it needed?
Adds a shim to go_binary when built in coverage mode to collect coverage info. Useful for integration testing.

Other notes for review
Added integration test

Manual testing:

bazel coverage foo:all --cache_test_results=no --instrument_test_targets --instrumentation_filter=//... --combined_report=lcov
genhtml -o coverage_html /private/var/tmp/_bazel_dzbarsky/2c76d2042b2c36d4458efe14b865e618/execroot/io_bazel_rules_go/bazel-out/darwin_arm64-fastbuild/testlogs/foo/foo_test/coverage.dat && open coverage_html/index.html
image image

Fixes #3513

@dzbarsky dzbarsky changed the title Coverage hacks WIP Coverage hacks Oct 1, 2025
@dzbarsky dzbarsky changed the title WIP Coverage hacks Allow collecting coverage for go_binary executed as child process Oct 2, 2025
@fmeum
Copy link
Copy Markdown
Member

fmeum commented Oct 3, 2025

Not sure about macOS yet, but I could get the Linux integration test to pass with these changes:

diff --git a/tests/core/coverage/coverage_test.go b/tests/core/coverage/coverage_test.go
index 22009b3c..7cc6e54b 100644
--- a/tests/core/coverage/coverage_test.go
+++ b/tests/core/coverage/coverage_test.go
@@ -50,8 +50,14 @@ go_library(
     name = "a",
     srcs = ["a.go"],
     importpath = "example.com/coverage/a",
-    deps = [":b"],
+    deps = [
+        ":b",
+        "@io_bazel_rules_go//go/runfiles",
+    ],
 	data = [":a_binary"],
+    x_defs = {
+        "aBinaryRlocationPath": "$(rlocationpath :a_binary)",
+    },
 )
 
 go_binary(
@@ -107,28 +113,25 @@ func main() {
 -- a_test.go --
 package a
 
-import (
-    "path/filepath"
-	"os"
-	"os/exec"
-	"testing"
-)
+import "testing"
 
 func TestA(t *testing.T) {
 	ALive()
 }
 
 func TestBinary(t *testing.T) {
-	cmd := exec.Command(filepath.Join(os.Getenv("RUNFILES_DIR"), "bazel_testing", "a_binary"))
-	err := cmd.Run()
-	if err != nil {
-		t.Fatal(err)
-	}
+    RunBinary()
 }
 -- a.go --
 package a
 
-import "example.com/coverage/b"
+import (
+	"os/exec"
+    "example.com/coverage/b"
+    "github.com/bazelbuild/rules_go/go/runfiles"
+)
+
+var aBinaryRlocationPath string
 
 func ALive() int {
 	return b.BLive()
@@ -138,6 +141,18 @@ func ADead() int {
 	return b.BDead()
 }
 
+func RunBinary() {
+    p, err := runfiles.Rlocation(aBinaryRlocationPath)
+	if err != nil {
+		panic(err)
+	}
+	cmd := exec.Command(p)
+	err = cmd.Run()
+	if err != nil {
+		panic(err)
+	}
+}
+
 -- b.go --
 package b
 
@@ -219,6 +234,8 @@ func testCoverage(t *testing.T, expectedCoverMode string, extraArgs ...string) {
 		extraArgs,
 		"--instrumentation_filter=-//:b",
 		"--@io_bazel_rules_go//go/config:cover_format=go_cover",
+		"--test_output=errors",
+		"--test_env=VERBOSE_COVERAGE=1",
 		":a_test",
 	)...)
 

@fmeum
Copy link
Copy Markdown
Member

fmeum commented Oct 3, 2025

For macOS, you could try adding apple_support to the MODULE.bazel file used by bazel_testing. The default Unix toolchain is completely unmaintained on macOS.

@dzbarsky
Copy link
Copy Markdown
Contributor Author

dzbarsky commented Oct 3, 2025

For macOS, you could try adding apple_support to the MODULE.bazel file used by bazel_testing. The default Unix toolchain is completely unmaintained on macOS.

thanks, i'll give it a shot. I'm confused why this wasn't an issue before when building the go_test, only with the go_binary...

@dzbarsky dzbarsky force-pushed the coverage branch 2 times, most recently from 0260c1c to 13b6993 Compare October 3, 2025 15:16
@dzbarsky
Copy link
Copy Markdown
Contributor Author

dzbarsky commented Oct 3, 2025

For macOS, you could try adding apple_support to the MODULE.bazel file used by bazel_testing. The default Unix toolchain is completely unmaintained on macOS.

I came back this morning and couldn't reproduce the issue :(

I guess this should be good to go once we resolve the last issue from golang/go@40b3c0e

external/rules_go~/go/tools/bzltestutil/coverage_visitor.go:176:39: too many arguments in call to d.format.EmitTextual
	have (nil, io.Writer)
	want (io.Writer)
	```

@dzbarsky dzbarsky force-pushed the coverage branch 6 times, most recently from 025bb18 to f6f63a9 Compare October 3, 2025 20:01
@dzbarsky dzbarsky force-pushed the coverage branch 2 times, most recently from 683b9a6 to 8cb50f5 Compare October 5, 2025 14:52
@dzbarsky
Copy link
Copy Markdown
Contributor Author

dzbarsky commented Oct 8, 2025

I believe all feedback is addressed now and this should be good to go!

Copy link
Copy Markdown
Collaborator

@jayconrod jayconrod left a comment

Choose a reason for hiding this comment

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

I took a quick look. I'm mostly stamping as LGTM, but let me know if you'd like a more detailed review. @fmeum has a much more informed opinion on the topic of coverage than I do.

@fmeum fmeum merged commit 56929da into bazel-contrib:master Oct 8, 2025
1 check passed
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.

Add support for "Coverage profiling support for integration tests"

5 participants