Skip to content

coverage: Don't panic if flag.CommandLine is reassigned - #4384

Merged
fmeum merged 2 commits into
bazel-contrib:masterfrom
abhinav:abg/cover-no-panic
Jun 27, 2025
Merged

fmeum merged 2 commits into
bazel-contrib:masterfrom
abhinav:abg/cover-no-panic

Conversation

@abhinav

@abhinav abhinav commented Jun 26, 2025

Copy link
Copy Markdown
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

When run with go test -cover, the following test does not panic:

-- foo.go --
package foo

func foo() int {
	return 42
}
-- foo_test.go --
package foo

import (
	"flag"
	"testing"
)

func TestReassign(t *testing.T) {
	flag.CommandLine = flag.NewFlagSet("test", flag.ExitOnError)
	_ = foo()
}

However, with bazel coverage, it does panic:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x20 pc=0x100f8dbe4]

goroutine 1 [running]:
github.com/bazelbuild/rules_go/go/tools/bzltestutil.ConvertCoverToLcov()
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:39 +0x54
github.com/bazelbuild/rules_go/go/tools/bzltestutil.lcovAtExitHook()
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:181 +0x1c
github.com/bazelbuild/rules_go/go/tools/bzltestutil.LcovTestDeps.SetPanicOnExit0({{}, 0x18?}, 0x4b?)
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:175 +0x24
testing.(*M).after(0x1400007c280?)
        GOROOT/src/testing/testing.go:2374 +0x74
testing.(*M).Run(0x1400007c280)
        GOROOT/src/testing/testing.go:2185 +0x940
main.main()
        bazel-out/darwin_arm64-fastbuild/bin/go_default_test_/testmain.go:157 +0x7a4

The cause is that the test reassigned flag.CommandLine
and did not restore the original value.
While that's bad behavior, go test does not explode,
so neither should bazel coverage.

This change fixes the issue by grabbing a reference to
flag.CommandLine before the test runs.

Which issues(s) does this PR fix?

Fixes #4383

When run with `go test -cvoer`, the following test does not panic:

```
-- foo.go --
package foo

func foo() int {
	return 42
}
-- foo_test.go --
package foo

import (
	"flag"
	"testing"
)

func TestReassign(t *testing.T) {
	flag.CommandLine = flag.NewFlagSet("test", flag.ExitOnError)
	_ = foo()
}
```

However, with `bazel coverage`, it does panic:

```
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x20 pc=0x100f8dbe4]

goroutine 1 [running]:
github.com/bazelbuild/rules_go/go/tools/bzltestutil.ConvertCoverToLcov()
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:39 +0x54
github.com/bazelbuild/rules_go/go/tools/bzltestutil.lcovAtExitHook()
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:181 +0x1c
github.com/bazelbuild/rules_go/go/tools/bzltestutil.LcovTestDeps.SetPanicOnExit0({{}, 0x18?}, 0x4b?)
        external/io_bazel_rules_go/go/tools/bzltestutil/lcov.go:175 +0x24
testing.(*M).after(0x1400007c280?)
        GOROOT/src/testing/testing.go:2374 +0x74
testing.(*M).Run(0x1400007c280)
        GOROOT/src/testing/testing.go:2185 +0x940
main.main()
        bazel-out/darwin_arm64-fastbuild/bin/go_default_test_/testmain.go:157 +0x7a4
```

The cause is that the test reassigned `flag.CommandLine`
and did not restore the original value.
While that's bad behavior, `go test` does not explode,
so neither should `bazel coverage`.

This change fixes the issue by grabbing a reference to
`flag.CommandLine` before the test runs.
@abhinav

abhinav commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

Windows test failed. Is there any way to access the generated artifact?

@fmeum

fmeum commented Jun 26, 2025

Copy link
Copy Markdown
Member

Windows test failed. Is there any way to access the generated artifact?

bazel coverage doesn't work on Windows, so the bazelci config for Windows needs to explicitly exclude the new test. You can search for some other coverage test's name to find the locations you need to update.

@fmeum
fmeum merged commit bf5bfda into bazel-contrib:master Jun 27, 2025
gh-worker-dd-mergequeue-cf854d Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Aug 31, 2026
### What does this PR do?

This PR ports #52373 onto the current `main` branch and makes Datadog the sole code-coverage backend.

#### Remove the redundant Codecov integration

**Before:** CI uploaded coverage to both Codecov and Datadog. This required a Codecov secret, configuration, helper code, tests, and documentation badges.

**Now:** Codecov configuration, credentials, upload steps, helpers, tests, and badges are removed. Datadog Code Coverage remains the supported backend.

**Impact:** There is one less external integration and secret to maintain, without losing coverage reporting.

#### Separate coverage-cache management from report submission

**Before:** S3 cache management was coupled to the coverage upload step. Reusing that step for additional jobs would also make them pull or push the shared cache, even when they already generated complete reports.

**Now:** CI has separate operations for:

- Managing the S3 coverage cache.
- Uploading a report to Datadog.

Impacted-package jobs run both operations. Flavor and Bazel jobs upload their reports without modifying the shared cache.

**Impact:** Fast test jobs still merge cached coverage from unaffected packages, while additional uploaders cannot overwrite or race on cache entries they do not own.

#### Submit previously unreported Linux flavor coverage

The following jobs already generated `coverage.out`, but only uploaded JUnit results and retained coverage as a CI artifact:

- `tests_flavor_iot_linux-x64`
- `tests_flavor_dogstatsd_linux-x64`
- `tests_flavor_heroku_linux-x64`

They now upload their coverage reports to Datadog.

**Impact:** Coverage from flavor-specific code paths is now included in the aggregate coverage view instead of remaining only in GitLab artifacts.

A shared `.linux_flavor_tests` template also removes duplicated job configuration.

#### Submit Bazel coverage reports

The following jobs generated named coverage artifacts but did not submit them to any backend:

- `bazel:coverage:linux-amd64`
- `bazel:coverage:linux-arm64`
- `bazel:coverage:macos-amd64`
- `bazel:coverage:macos-arm64`

They now pass their `coverage-bazel-*.out` reports to the shared Datadog upload step.

**Impact:** Bazel coverage is visible in Datadog and can be compared with the legacy gotestsum coverage while the repository transitions toward Bazel-based testing.

#### Fix the GPU coverage submission

**Before:** `tests_gpu` attempted to upload coverage, but `gocovmerge` produced a zero-byte profile. Datadog rejected it because a Go coverprofile must begin with a `mode:` header. The upload was non-fatal, so the job remained green even though no report was accepted.

**Now:** If a merged profile is empty, the coverage helper adds the configured Go coverage mode header. Non-empty profiles are left unchanged.

**Impact:** The GPU job now submits a syntactically valid report and is recorded by Datadog instead of failing silently.

#### Fix Windows coverage submission

**Before:** `tests_windows-x64` generated coverage and merged the S3 cache correctly, but its upload attempted to execute `datadog-ci.exe`, which was unavailable in the build image. Direct PowerShell resolution of `datadog-ci` also failed, while the job remained green because uploads are non-fatal.

**Now:** A dedicated Invoke task resolves the platform-specific `datadog-ci` command with `shutil.which` and executes it through Python. This follows the same command-resolution pattern already used successfully by Windows JUnit uploads.

**Impact:** Windows coverage now reaches Datadog and contributes to the same commit-level aggregate as Linux and macOS coverage.

#### Preserve E2E coverage without Codecov coupling

**Before:** E2E profile conversion also read Codecov-specific metadata and uploaded to Codecov before Datadog processed the converted profiles.

**Now:** E2E coverage folders are converted directly into Go coverprofiles for Datadog. Conversion no longer depends on Codecov metadata.

**Impact:** Existing E2E coverage remains available while the conversion pipeline is simpler and backend-independent.

### Motivation

The repository already used Datadog Code Coverage, so maintaining Codecov in parallel added secrets, configuration, and CI work without providing a necessary capability.

At the same time, several jobs generated useful reports that Datadog never received, and the GPU and Windows upload paths failed silently. This change removes the redundant integration while making the Datadog submission path complete and verifiable.

### Describe how you validated your changes

- `dda inv invoke-unit-tests.run --tests=coverage,go --directory=tasks/unit_tests --no-buffer --verbosity=1`
- `dda inv linter.releasenote-unique-ids`
- `dda inv linter.full-gitlab-ci -t main --pre-push-linters --fail-fast --verbosity=2`
- The [pipeline](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/pipelines/132663591) passed.
- Datadog [confirmed](https://app.datadoghq.com/ci/code-coverage/github.com%2Fdatadog%2Fdatadog-agent/pull-requests/55278) all 11 expected non-E2E reports were ingested.

### Additional Notes

Windows Bazel jobs remain test-only because `rules_go 0.63.0` [does not](bazel-contrib/rules_go#4384 (comment)) reliably support Bazel coverage on Windows. `tests_windows-x64` remains the supported Windows coverage producer.

Co-authored-by: nikita-tkachenko-datadog <nikita.tkachenko@datadoghq.com>
Co-authored-by: ofek.lev <ofek.lev@datadoghq.com>
github-actions Bot pushed a commit to DataDog/datadog-agent that referenced this pull request Aug 31, 2026
### What does this PR do?

This PR ports #52373 onto the current `main` branch and makes Datadog the sole code-coverage backend.

#### Remove the redundant Codecov integration

**Before:** CI uploaded coverage to both Codecov and Datadog. This required a Codecov secret, configuration, helper code, tests, and documentation badges.

**Now:** Codecov configuration, credentials, upload steps, helpers, tests, and badges are removed. Datadog Code Coverage remains the supported backend.

**Impact:** There is one less external integration and secret to maintain, without losing coverage reporting.

#### Separate coverage-cache management from report submission

**Before:** S3 cache management was coupled to the coverage upload step. Reusing that step for additional jobs would also make them pull or push the shared cache, even when they already generated complete reports.

**Now:** CI has separate operations for:

- Managing the S3 coverage cache.
- Uploading a report to Datadog.

Impacted-package jobs run both operations. Flavor and Bazel jobs upload their reports without modifying the shared cache.

**Impact:** Fast test jobs still merge cached coverage from unaffected packages, while additional uploaders cannot overwrite or race on cache entries they do not own.

#### Submit previously unreported Linux flavor coverage

The following jobs already generated `coverage.out`, but only uploaded JUnit results and retained coverage as a CI artifact:

- `tests_flavor_iot_linux-x64`
- `tests_flavor_dogstatsd_linux-x64`
- `tests_flavor_heroku_linux-x64`

They now upload their coverage reports to Datadog.

**Impact:** Coverage from flavor-specific code paths is now included in the aggregate coverage view instead of remaining only in GitLab artifacts.

A shared `.linux_flavor_tests` template also removes duplicated job configuration.

#### Submit Bazel coverage reports

The following jobs generated named coverage artifacts but did not submit them to any backend:

- `bazel:coverage:linux-amd64`
- `bazel:coverage:linux-arm64`
- `bazel:coverage:macos-amd64`
- `bazel:coverage:macos-arm64`

They now pass their `coverage-bazel-*.out` reports to the shared Datadog upload step.

**Impact:** Bazel coverage is visible in Datadog and can be compared with the legacy gotestsum coverage while the repository transitions toward Bazel-based testing.

#### Fix the GPU coverage submission

**Before:** `tests_gpu` attempted to upload coverage, but `gocovmerge` produced a zero-byte profile. Datadog rejected it because a Go coverprofile must begin with a `mode:` header. The upload was non-fatal, so the job remained green even though no report was accepted.

**Now:** If a merged profile is empty, the coverage helper adds the configured Go coverage mode header. Non-empty profiles are left unchanged.

**Impact:** The GPU job now submits a syntactically valid report and is recorded by Datadog instead of failing silently.

#### Fix Windows coverage submission

**Before:** `tests_windows-x64` generated coverage and merged the S3 cache correctly, but its upload attempted to execute `datadog-ci.exe`, which was unavailable in the build image. Direct PowerShell resolution of `datadog-ci` also failed, while the job remained green because uploads are non-fatal.

**Now:** A dedicated Invoke task resolves the platform-specific `datadog-ci` command with `shutil.which` and executes it through Python. This follows the same command-resolution pattern already used successfully by Windows JUnit uploads.

**Impact:** Windows coverage now reaches Datadog and contributes to the same commit-level aggregate as Linux and macOS coverage.

#### Preserve E2E coverage without Codecov coupling

**Before:** E2E profile conversion also read Codecov-specific metadata and uploaded to Codecov before Datadog processed the converted profiles.

**Now:** E2E coverage folders are converted directly into Go coverprofiles for Datadog. Conversion no longer depends on Codecov metadata.

**Impact:** Existing E2E coverage remains available while the conversion pipeline is simpler and backend-independent.

### Motivation

The repository already used Datadog Code Coverage, so maintaining Codecov in parallel added secrets, configuration, and CI work without providing a necessary capability.

At the same time, several jobs generated useful reports that Datadog never received, and the GPU and Windows upload paths failed silently. This change removes the redundant integration while making the Datadog submission path complete and verifiable.

### Describe how you validated your changes

- `dda inv invoke-unit-tests.run --tests=coverage,go --directory=tasks/unit_tests --no-buffer --verbosity=1`
- `dda inv linter.releasenote-unique-ids`
- `dda inv linter.full-gitlab-ci -t main --pre-push-linters --fail-fast --verbosity=2`
- The [pipeline](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/pipelines/132663591) passed.
- Datadog [confirmed](https://app.datadoghq.com/ci/code-coverage/github.com%2Fdatadog%2Fdatadog-agent/pull-requests/55278) all 11 expected non-E2E reports were ingested.

### Additional Notes

Windows Bazel jobs remain test-only because `rules_go 0.63.0` [does not](bazel-contrib/rules_go#4384 (comment)) reliably support Bazel coverage on Windows. `tests_windows-x64` remains the supported Windows coverage producer.

Co-authored-by: nikita-tkachenko-datadog <nikita.tkachenko@datadoghq.com>
Co-authored-by: ofek.lev <ofek.lev@datadoghq.com> 6aeb8ae
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.

bazel coverage panics if flag.CommandLine is reassigned

2 participants