[dead-code] chore: remove dead functions — 1 function removed#44330
Conversation
Remove renderMarkdownMetricsTrend which was unreachable (no non-test callers) and its exclusive test TestRenderMarkdownMetricsTrend_IncludesTurnsWithoutTokens. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pelikhan
deleted the
dead-code/remove-renderMarkdownMetricsTrend-76dae1895bd7acd6
branch
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44330 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. (PR removes 28 lines including 24 lines of test deletions from audit_cross_run_test.go, which is a dead-code cleanup, not new test coverage.) |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Removes a dead stdout wrapper function for rendering cross-run Markdown metrics and deletes the associated unit test that called it.
Changes:
- Removed
renderMarkdownMetricsTrend(stdout wrapper) in favor of usingrenderMarkdownMetricsTrendToWriterdirectly. - Removed
TestRenderMarkdownMetricsTrend_IncludesTurnsWithoutTokens.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/audit_cross_run_render.go | Removes an unused stdout wrapper around renderMarkdownMetricsTrendToWriter. |
| pkg/cli/audit_cross_run_test.go | Deletes the only test that covered Markdown metrics “turns without tokens” output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
|
|
||
| func TestRenderMarkdownMetricsTrend_IncludesTurnsWithoutTokens(t *testing.T) { | ||
| oldStdout := os.Stdout | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err, "should create stdout pipe") | ||
| os.Stdout = w | ||
|
|
||
| renderMarkdownMetricsTrend(MetricsTrendData{ | ||
| TotalTurns: 9, | ||
| AvgTurns: 4.5, | ||
| MaxTurns: 6, | ||
| }) | ||
|
|
||
| w.Close() | ||
| os.Stdout = oldStdout | ||
|
|
||
| var buf bytes.Buffer | ||
| _, err = buf.ReadFrom(r) | ||
| require.NoError(t, err, "should read captured stdout") | ||
| output := buf.String() | ||
|
|
||
| assert.Contains(t, output, "## Metrics Trends", "Should render metrics section when turn metrics exist") | ||
| assert.Contains(t, output, "| Turns | 9 | 4.5 | — | 6 | — |", "Should render turns row without tokens") | ||
| } | ||
|
|
||
| func TestRenderPrettyMetricsTrend_IncludesDurationWithoutTokens(t *testing.T) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Dead Code Removal
The change is clean and correct. renderMarkdownMetricsTrend was a trivial os.Stdout-bound wrapper with no remaining callers; removing it alongside its dedicated test is the right approach. The underlying renderMarkdownMetricsTrendToWriter remains intact and tested.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 17.8 AIC · ⌖ 5.76 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a clean, risk-free dead-code removal.
📋 Key Themes & Highlights
Positive Highlights
- ✅
renderMarkdownMetricsTrendhad zero callers — confirmed dead; removal is correct - ✅ The
ToWritervariant (renderMarkdownMetricsTrendToWriter) remains, is the better-designed function (injectedio.Writervs hard-codedos.Stdout), and is still exercised by higher-level tests - ✅ Removing the test that relied on stdout pipe-hijacking is a quality improvement — the underlying behaviour is covered by
TestRenderCrossRunReportMarkdownvia theToWriterpath - ✅ All verification checks pass (build, vet, fmt)
No actionable issues found.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 17.9 AIC · ⌖ 6.56 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Dead code removal is correct; one test coverage gap needs attention
The renderMarkdownMetricsTrend wrapper has zero callers — removal is justified. The generated check runs confirm go build and go vet pass. One non-blocking issue flagged inline.
Review summary
What's good: The removal is clean — no stale imports, no lingering references, and the ...ToWriter variant retains its integration-level call path via renderCrossRunReportMarkdown.
Issue: TestRenderMarkdownMetricsTrend_IncludesTurnsWithoutTokens was the sole test exercising the TotalTurns > 0 && TotalTokens == 0 code path in renderMarkdownMetricsTrendToWriter. Deleting the wrapper does not justify deleting this branch coverage. The fix is a one-to-one test migration to call renderMarkdownMetricsTrendToWriter with an io.Writer instead of relying on the now-removed os.Stdout-swapping wrapper.
🔎 Code quality review by PR Code Quality Reviewer · 78.7 AIC · ⌖ 6.45 AIC · ⊞ 5.4K
Comment /review to run again
| assert.Contains(t, output, "| Turns | 9 | 4.5 | — | 6 | — |", "Should render turns row without tokens") | ||
| } | ||
|
|
||
| func TestRenderPrettyMetricsTrend_IncludesDurationWithoutTokens(t *testing.T) { |
There was a problem hiding this comment.
Test coverage gap: deleting this test removes the only coverage for the TotalTurns > 0 && TotalTokens == 0 branch in renderMarkdownMetricsTrendToWriter.
💡 Suggested fix
The test tested the now-dead wrapper (renderMarkdownMetricsTrend), but the interesting assertion — that the Turns row renders correctly when no token data exists — tests the underlying renderMarkdownMetricsTrendToWriter logic, which is still live. The wrapper removal does not justify deleting the coverage.
The remaining integration test TestRenderCrossRunReportMarkdown_IncludesNewSections always supplies both TotalTokens and TotalTurns, so it never exercises lines 79–82 of the render file:
if mt.TotalTurns > 0 {
fmt.Fprintf(w, "| Turns | %d | %.1f | — | %d | — |\\n",
mt.TotalTurns, mt.AvgTurns, mt.MaxTurns)
}Instead of deleting, migrate the test to call renderMarkdownMetricsTrendToWriter directly:
func TestRenderMarkdownMetricsTrendToWriter_TurnsOnlyNoTokens(t *testing.T) {
var buf bytes.Buffer
renderMarkdownMetricsTrendToWriter(&buf, MetricsTrendData{
TotalTurns: 9,
AvgTurns: 4.5,
MaxTurns: 6,
})
output := buf.String()
assert.Contains(t, output, "## Metrics Trends", "Should render metrics section when turn metrics exist")
assert.Contains(t, output, "| Turns | 9 | 4.5 | — | 6 | — |", "Should render turns row without tokens")
}This also eliminates the flaky os.Stdout swap antipattern from the deleted test.
|
🎉 This pull request is included in a new release. Release: |
Summary
Removes one dead unexported function and its corresponding test.
Changes
pkg/cli/audit_cross_run_render.gorenderMarkdownMetricsTrend(mt MetricsTrendData)— an unexported wrapper that forwarded torenderMarkdownMetricsTrendToWriter(os.Stdout, mt). The function had no callers and was dead code. The underlyingrenderMarkdownMetricsTrendToWriteris unchanged.pkg/cli/audit_cross_run_test.goTestRenderMarkdownMetricsTrend_IncludesTurnsWithoutTokens— the sole test for the removed function. The test captured stdout viaos.Pipe(), exercisedrenderMarkdownMetricsTrend, and asserted markdown table output. Removed as it no longer has a subject.Impact
os.Stdoutredirection (fragile pattern).Checklist