Repository navigation
feat: per-test Go code coverage over socket files with blocks, totals and build tables - #208
Gauravsingh096 wants to merge 11 commits into
Conversation
Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
…a timeout (G4) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
… (G6) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
…ble per test set (G8) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
…sons (G9) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
Gauravsingh096
left a comment
There was a problem hiding this comment.
Principal review: go-sdk#208 at 95a2292, approve with follow-ups
Reviewed as one set with keploy#4984, enterprise#3136, api-server#2420, enterprise-ui#2027 and k8s-proxy#1163, each repo at its own PR head. I read the whole diff and keploy/keploy.go in full. I also checked the record shape against the enterprise agent receiver (pkg/service/agent/dedup.go) and its consumers: the enterprise CLI and k8s-proxy dedup_agent_coverage.go. Not run live: no keploy CLI, no cluster.
Verified fine:
- Wire contract with the agent.
coverageRecordtags (id,executedLinesByFile,executedBlocksByFile,totals{statements,coveredStatements},metaHash,buildBlocks,reason{code,message}) match enterpriseDedupRecord(agent/dedup.go:172-203) field by field.codeBlock [5]intmatchesCodeBlock [5]int. - Old agent + this SDK. Main's receiver drops a record with no lines (main agent/dedup.go:582). Reason records are therefore ignored exactly as "nothing sent" was before. An empty-coverage test is never grouped as a duplicate (enterprise deduplication/dedup.go:458-465).
- Staged covdata name.
keploy-covdata-<GOARCH>in the socket folder matches enterprisecovdata.FileName()(covdata.go:23) and the staging folder. - The
-covermode=atomicreason is real. On Go 1.27.1, a-covermode=setbinary getsWriteCountersDir invoked for program built with -covermode=setback, so the README's "not measured" claim holds as adump_failedreason (probe, deleted). - G1.
tempDirPatternreplaces theparts[1]index that panicked the control goroutine on an ID without a slash. buildTableSentis read and written only undercontrolMu.
Gates:
CGO_ENABLED=0 go test -count=1 -v ./keploy/: PASS, 27 runs.-racenot run: the review host has no C compiler.- The new tests do not compile on the merge base, because they call new symbols. Instead I reverted lines at the head; see M2.
Earlier reviews
| Reviewer | Point | Status |
|---|---|---|
| copilot-pull-request-reviewer | No review (quota) | n/a |
Minor
M1. When Keploy's staged keploy-covdata is present but cannot run, the SDK gives up instead of falling back to go tool covdata. (inline)
M2. The record the SDK actually sends is not under test: two wrong versions of it pass the whole suite. (inline)
Nits
N1. README uses "the agent" for both Keploy's agent and the SDK. (inline)
Claims ledger
| Claim | Status | Evidence |
|---|---|---|
| G1 no crash on an ID without a slash | Verified (C) | tempDirPattern uses strings.Cut; TestTempDirPattern |
G2 KEPLOY_COVERAGE_DIR holds the sockets and temp files; unset keeps old behaviour |
Verified (C) | keploy.go:53-55 (socketDir), MkdirTemp/CreateTemp with "" -> OS temp |
| G3 a test with no coverage sends a reason record | Verified (C) | END mismatch, no START, dump, covdata paths all call sendReason |
G4 Keploy's covdata first, then go tool covdata, with a timeout |
Partly | Correct when the file is absent; no fallback when it is present but fails (M1) |
G5 module-relative keys from build info, no go list |
Verified (C) | fileKey + mainModulePath |
| G6/G7/G8 blocks, count-0 kept, totals, meta hash, build table once per set | Verified (C), untested end to end | M2 |
"Wire format is additive: old receivers keep reading lines" |
Partly | Additive fields, yes. The keys of executedLinesByFile changed from absolute paths to module-relative ones. Within one run that is harmless, but say it in the body. |
README: "If it is not there, go tool covdata is used" |
Verified | "if it does not run" is not covered (M1) |
Cross-repo / linked PRs
- Contract: matches the enterprise agent and the k8s-proxy meta reader at their PR heads (see above).
- Merge order: release this first. enterprise#3136 (E4) stages the covdata this SDK looks for, and k8s-proxy#1163's e2e lane needs a go-sdk tag. Any order is safe for data, since old agents ignore the new fields and reason records.
What I'd want before merge
- M2's record-assembly test. A regression there would make every test look 100% covered.
- M1's fallback, or a README line saying a staged covdata that cannot run is not retried with
go tool covdata.
…a cannot start (review M1) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
…d table) through buildRecord and needsBuildTable, and say the SDK in the README (review M2, N1) Signed-off-by: Gaurav Singh <singhgaurav07122004@gmail.com>
Gauravsingh096
left a comment
There was a problem hiding this comment.
Principal review (round 2): go-sdk#208 at 88f70a9, approve
Round 2 covers the two commits pushed since round 1 (95a2292..88f70a9). I read the full diff of 86dcce4 and 88f70a9 and the author's thread replies, and re-checked each round-1 point by running it. The six PRs (go-sdk#208, keploy#4984, enterprise#3136, api-server#2420, enterprise-ui#2027, k8s-proxy#1163) were reviewed together again. Not run live.
Verified fine:
- The fallback only fires when the staged tool could not start:
keploy/keploy.go:476checkscmd.Path == covdataPath()and that the error is not an*exec.ExitError. A tool that ran and exited non-zero is not retried.permission denied(noexec) andexec format error(the wrong arch) are both start errors, so both now reachgo tool covdatawhengois onPATH. useGoCovdatais written only on thereportCoveragepath, which runs undercontrolMu(keploy/keploy.go:194), so it has no race with the read incovdataCommand.buildRecordandneedsBuildTableare the round-1 code, moved unchanged.reportCoveragestill setsbuildTableSentonly after a successful send (keploy/keploy.go:281).
Gates:
CGO_ENABLED=0 go test ./keploy/ -count=2: ok.- Mutations at the head, each run against its test:
- Fallback block disabled:
TestCovdataFallsBackToGoWhenKeployCovdataCannotStartFAILS. - Exit-error guard removed:
TestCovdataNoFallbackWhenKeployCovdataFailsFAILS. needsBuildTablereduced to!buildTableSent:TestBuildRecordFAILS.- Run blocks taken from
codeBlocks(false):TestBuildRecordFAILS.
- Fallback block disabled:
- Every mutation was reverted. I left no files in the repo.
Earlier reviews
| Reviewer | Point | Status |
|---|---|---|
| @Gauravsingh096 (round 1) | M1: no fallback when the staged covdata cannot start | ✅ Fixed (L), 86dcce4 |
| @Gauravsingh096 (round 1) | M2: record assembly untested | ✅ Fixed (L), 88f70a9 |
| @Gauravsingh096 (round 1) | N1: README says "the agent" for the SDK | ✅ Fixed (C), 88f70a9 |
| Copilot | No review (quota) | n/a |
No new findings.
Cross-repo / linked PRs
- The new fallback partly covers enterprise#3136's open M1 (the arch mismatch). It covers an image that has
go. It does not cover an amd64-only image withoutgoon an arm64 host, because there the file the SDK looks for is absent, not unable to start. That follow-up stays on enterprise#3136. - Merge order is unchanged: tag go-sdk first, because k8s-proxy#1163's K17 lane and the e2e fixture pin to it.
Residual risk: the fallback has not been run on a real noexec /tmp mount. The tests stand in for that case with a non-executable file, which fails the same way.
Describe the changes that are made
Per-test Go code coverage for Keploy's dynamic dedup and the new per-test code-coverage store. The app is built by the user with
go build -cover -covermode=atomicand imports the SDK; the SDK reports, for every replayed test, which code ran.fix(keploy): no crash on a test ID without a slash.KEPLOY_COVERAGE_DIRholds the control/data sockets and temp files (the sharedkeploy-socketsfolder in k8s), so the app's own/tmpis untouched.not_measured+ reason) instead of nothing.covdata(delivered by the agent) first, thengo tool covdata, with a timeout, so the runtime image needs no Go toolchain.go list).KEPLOY_COVERAGE_DIR, Keploy's covdata, the not-measured reasons.Wire format is additive: old receivers keep reading
lines; Java and older SDKs are unaffected.Links & References
Part of keploy/k8s-proxy#1133 — https://github.com/keploy/k8s-proxy/issues/1133
Related PRs
feat/go-code-coverage(PR follows; it pins this set)How was this tested
go test ./...), including the profile parser, socket protocol and reason records.🤖 Generated with Claude Code