Repository navigation
feat(coverage): detect -cover from Go build info before the symbol scan - #4984
Gauravsingh096 wants to merge 3 commits into
Conversation
…an (O1) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request. Check if the Files changed in this pull request are included in default exclusions.
🚀 Keploy Performance Test ResultsMulti-Run Validation: Tests run 3 times, pipeline fails only if 2+ runs show regression.
Thresholds: P50 < 5ms, P90 < 15ms, P99 < 70ms, RPS >= 100 (±1% tolerance), Error Rate < 1% ✅ Result: PASSED - Only 0 out of 3 runs failed (threshold: 2) P50, P90, and P99 percentiles naturally filter out outliers |
🚀 Keploy Performance Test ResultsMulti-Run Validation: Tests run 3 times, pipeline fails only if 2+ runs show regression.
Thresholds: P50 < 5ms, P90 < 15ms, P99 < 70ms, RPS >= 100 (±1% tolerance), Error Rate < 1% ✅ Result: PASSED - Only 0 out of 3 runs failed (threshold: 2) P50, P90, and P99 percentiles naturally filter out outliers |
Gauravsingh096
left a comment
Gauravsingh096
left a comment
There was a problem hiding this comment.
Principal review: #4984 at 5645641, approve with follow-ups
Reviewed as one set with go-sdk#208, enterprise#3136, api-server#2420, enterprise-ui#2027 and k8s-proxy#1163, each repo at its own PR head. I read the diff, utils.go in full and its only caller (golang.go:41). I ran checkForCoverFlag on real binaries. Not run live.
Verified fine:
-
The fix works. I built three binaries with Go 1.27.1 and called
checkForCoverFlagon each, at the merge base and at this head:-cover -ldflags='-s -w':falseon the base,trueat the head.-cover:trueon both.- plain build:
falseon both.
go version -mshowsbuild -cover=trueon both cover binaries. -
Build info first, symbol scan second. A binary with no build info, or no
-cover=true, still falls through to the old symbol scan, so nothing that was detected before is lost. -
The known limit is real and correctly stated. A plain binary that imports
runtime/coveragegivestruefrom the symbol fallback (probe).
Gates:
go test -count=1 -v ./pkg/platform/coverage/golang/: PASS.- The new test does not compile on the merge base (
undefined: builtWithCover). - With the call at utils.go:35 disabled at the head, the test still passes (M1).
Earlier reviews
| Reviewer | Point | Status |
|---|---|---|
| copilot-pull-request-reviewer | "unable to review any files" | n/a |
Minor
M1. The test passes with the fix removed, and the PR body describes a test that does not exist. (inline)
Claims ledger
| Claim | Status | Evidence |
|---|---|---|
-cover=true in build info means built with -cover |
Verified (L) | go version -m; cmd/go load/pkg.go:2483 appends it |
A -s -w cover binary was missed before |
Verified (L) | base false, head true |
| Works on Mach-O too | Unverified (not run) | debug/buildinfo is format-agnostic, so the true path returns before elf.Open |
| Scope: native only, the only caller is golang.go:41 | Verified (C) | grep |
"new utils_test.go builds a -cover -ldflags='-s -w' binary and a plain one" |
Refuted | The test only builds []debug.BuildSetting literals |
| Known limit: a plain binary importing go-sdk is detected as covered | Verified (L) | probe |
Cross-repo / linked PRs
Independent of the others: this is the OSS aggregate-coverage path, not the per-test path. It can merge in any order.
🚀 Keploy Performance Test ResultsMulti-Run Validation: Tests run 3 times, pipeline fails only if 2+ runs show regression.
Thresholds: P50 < 5ms, P90 < 15ms, P99 < 70ms, RPS >= 100 (±1% tolerance), Error Rate < 1% ✅ Result: PASSED - Only 0 out of 3 runs failed (threshold: 2) P50, P90, and P99 percentiles naturally filter out outliers |
Describe the changes that are made
checkForCoverFlag(pkg/platform/coverage/golang/utils.go) now reads the binary's Go build info first:-cover=truein its build settings means the app was built with-cover. Only when build info says nothing does it fall back to the existing symbol scan.-coverbinary built with-ldflags='-s -w'has-cover=truein build info but nointernal/coveragesymbols, so the old check said "not built with -cover". Works on Mach-O too.golang.go:41); OSS aggregate coverage, not the per-test path.Links & References
Issue: Part of keploy/k8s-proxy#1133 — https://github.com/keploy/k8s-proxy/issues/1133
🔗 Related PRs
🐞 Related Issues
📄 Related Documents
What type of PR is this? (check all applicable)
Added e2e test pipeline?
Added comments for hard-to-understand areas?
Added to documentation?
Are there any sample code or steps to test the changes?
go test ./pkg/platform/coverage/golang/— the newutils_test.gochecksbuiltWithCoveron build-settings lists only (it builds no binary). The fix itself was checked by runningcheckForCoverFlagon real Go 1.27.1 binaries:-cover -ldflags='-s -w'givesfalseon the merge base andtruehere;-covergivestrueon both; a plain build givesfalseon both.Known limit (not changed here): a plain binary that imports go-sdk links
runtime/coverage, so the symbol fallback can still say "covered" for it.Self Review done?
Any relevant screenshots, recordings or logs?
Additional checklist:
🤖 Generated with Claude Code