Skip to content

refactor(telemetry): reduce PrintTraceSummary positional params and add output assertions #906

Description

@lizhengfeng101

Description

PrintTraceSummary in internal/telemetry/events.go currently takes 9 positional parameters (after #870 merged the sessionID param). This makes calls hard to read and error-prone as the signature grows. Additionally, all existing tests for this function are smoke tests that only verify "no panic" — they cannot assert the actual printed output because stdout.Writer() captures os.Stdout at init time and there is no swap/redirect mechanism for tests.

This issue covers two related improvements:

1. Refactor the function signature

Current signature (after #870):

func PrintTraceSummary(filesReviewed, commentsGenerated int64,
    inputTokens, outputTokens, totalTokens int64,
    cacheReadTokens, cacheWriteTokens int64,
    duration time.Duration, sessionID string)

Option A (preferred): Introduce a TraceSummary struct and accept it as a single parameter:

type TraceSummary struct {
    FilesReviewed    int64
    CommentsGenerated int64
    InputTokens      int64
    OutputTokens     int64
    TotalTokens      int64
    CacheReadTokens  int64
    CacheWriteTokens int64
    Duration         time.Duration
    SessionID        string
}

func PrintTraceSummary(s TraceSummary) { ... }

Option B: Extract session printing into a separate PrintSessionID(id string) helper and keep the remaining 8-param signature (less ideal long-term but simpler diff).

2. Add a stdout.Swap test helper for output assertions

The internal/stdout package currently exposes Writer() and Quiet(). A new Swap(w io.Writer) func() (or a test-only CaptureOutput(t *testing.T) *bytes.Buffer) would allow tests to redirect output and assert content:

// Swap replaces the stdout writer and returns a restore function.
func Swap(replacement io.Writer) func() {
    mu.Lock()
    old := w
    w = replacement
    mu.Unlock()
    return func() {
        mu.Lock()
        w = old
        mu.Unlock()
    }
}

Then tests can assert actual output:

func TestPrintTraceSummary_WithSessionID(t *testing.T) {
    var buf bytes.Buffer
    defer stdout.Swap(&buf)()
    PrintTraceSummary(TraceSummary{..., SessionID: "abc-123"})
    assert.Contains(t, buf.String(), "[ocr] Session: abc-123")
}

Scope

  • File(s): internal/telemetry/events.go, internal/telemetry/events_test.go, internal/stdout/stdout.go
  • Function/area: PrintTraceSummary, stdout package
  • Call site: cmd/opencodereview/shared.go (update to use struct)

Acceptance Criteria

  • PrintTraceSummary accepts a struct (or the session line is extracted to a helper)
  • internal/stdout exposes a Swap (or test-capture) mechanism
  • PrintTraceSummary tests assert actual output strings, not just "no panic"
  • All existing tests updated for the new signature
  • Tests pass (make test)
  • Code check passes (make check)
  • Coverage maintained ≥90% (make coverage)

Context

Follow-up from #870 (which closed #866). The PR author flagged both items as out-of-scope improvements. See the PR description "Design notes" section for rationale.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions