Skip to content

feat: per-test Go code coverage over socket files with blocks, totals and build tables - #208

Open
Gauravsingh096 wants to merge 11 commits into
mainfrom
feat/go-code-coverage
Open

Gauravsingh096 wants to merge 11 commits into
mainfrom
feat/go-code-coverage

Conversation

@Gauravsingh096

@Gauravsingh096 Gauravsingh096 commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

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=atomic and imports the SDK; the SDK reports, for every replayed test, which code ran.

  • G1 fix(keploy): no crash on a test ID without a slash.
  • G2 KEPLOY_COVERAGE_DIR holds the control/data sockets and temp files (the shared keploy-sockets folder in k8s), so the app's own /tmp is untouched.
  • G3 a test with no coverage sends a reason record (not_measured + reason) instead of nothing.
  • G4 runs Keploy's standalone covdata (delivered by the agent) first, then go tool covdata, with a timeout, so the runtime image needs no Go toolchain.
  • G5 module-relative file keys from build info (no go list).
  • G6 parses the covdata profile into blocks, count-0 blocks kept (so the full build table is known).
  • G7 sends the blocks that ran, with columns, next to the lines.
  • G8 sends statement totals, a meta hash and the build's block table per test set.
  • G9 README: 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

How was this tested

  • Unit tests for every commit (go test ./...), including the profile parser, socket protocol and reason records.
  • Not yet run end to end on a cluster; the Go dedup e2e lane (k8s-proxy) needs a go-sdk release with these commits.

🤖 Generated with Claude Code

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>
Copilot AI balanced review requested due to automatic review settings October 29, 2026 12:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 Gauravsingh096 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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. coverageRecord tags (id, executedLinesByFile, executedBlocksByFile, totals{statements,coveredStatements}, metaHash, buildBlocks, reason{code,message}) match enterprise DedupRecord (agent/dedup.go:172-203) field by field. codeBlock [5]int matches CodeBlock [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 enterprise covdata.FileName() (covdata.go:23) and the staging folder.
  • The -covermode=atomic reason is real. On Go 1.27.1, a -covermode=set binary gets WriteCountersDir invoked for program built with -covermode=set back, so the README's "not measured" claim holds as a dump_failed reason (probe, deleted).
  • G1. tempDirPattern replaces the parts[1] index that panicked the control goroutine on an ID without a slash.
  • buildTableSent is read and written only under controlMu.

Gates:

  • CGO_ENABLED=0 go test -count=1 -v ./keploy/: PASS, 27 runs. -race not 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

  1. M2's record-assembly test. A regression there would make every test look 100% covered.
  2. M1's fallback, or a README line saying a staged covdata that cannot run is not retried with go tool covdata.

Comment thread keploy/keploy.go Resolved
Comment thread keploy/keploy.go Resolved
Comment thread README.md Resolved
…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 Gauravsingh096 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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:476 checks cmd.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) and exec format error (the wrong arch) are both start errors, so both now reach go tool covdata when go is on PATH.
  • useGoCovdata is written only on the reportCoverage path, which runs under controlMu (keploy/keploy.go:194), so it has no race with the read in covdataCommand.
  • buildRecord and needsBuildTable are the round-1 code, moved unchanged. reportCoverage still sets buildTableSent only 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: TestCovdataFallsBackToGoWhenKeployCovdataCannotStart FAILS.
    • Exit-error guard removed: TestCovdataNoFallbackWhenKeployCovdataFails FAILS.
    • needsBuildTable reduced to !buildTableSent: TestBuildRecord FAILS.
    • Run blocks taken from codeBlocks(false): TestBuildRecord FAILS.
  • 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 without go on 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.

@Gauravsingh096 Gauravsingh096 assigned Copilot and unassigned Copilot Oct 30, 2026
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.

3 participants