mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-20 14:14:30 +00:00
66d71b23eb
`--audience human --format json` ran completely silent: newQuietHandle replaced stdout with io.Discard whenever the format was machine-readable, without ever looking at the audience, so `--audience human` was ignored and the user watched a blank terminal until the document appeared at the end. `--format text` streamed progress but is not stable to parse, leaving no way to have both live progress and machine-readable output. Progress is now redirected to stderr instead of discarded when a human asked to watch a machine-readable run. This is safe because every result document (json, sarif, text) is encoded straight to os.Stdout and never travels through stdout.Writer(), so stdout remains a single parseable document while stderr carries the live [ocr] lines. Discarding was never necessary to protect stdout; the two streams were already separate. The three cases are now explicit: audience=agent discards progress regardless of format because the caller asked for none, a machine-readable format with a human audience redirects to stderr, and everything else leaves progress on stdout. Progress lines keep their existing text form. Emitting them as structured NDJSON events, which the report also asks for, would mean defining an event schema and reworking every call site; it is left for separate work. Tests cover where progress lands for each format/audience pair, that stdout stays empty and parseable while stderr receives the lines, and an end-to-end review asserting stdout unmarshals as JSON with no [ocr] line while stderr shows progress. Reverting the fix fails exactly the human-audience assertions and leaves the agent ones passing. The flag help and the CLI reference in all four locales are updated, along with the tip that implied `--format json` means a quiet terminal. Fixes #928
80 lines
2.6 KiB
Go
80 lines
2.6 KiB
Go
// SPDX-License-Identifier: Apache-2.0
|
|
// Copyright 2026 alibaba/open-code-review Contributors
|
|
|
|
package main
|
|
|
|
import (
|
|
"encoding/json"
|
|
"strings"
|
|
"testing"
|
|
)
|
|
|
|
// End-to-end coverage of #928: `--audience human --format json` used to run
|
|
// completely silent because the json format discarded stdout outright, ignoring
|
|
// the audience. Progress now goes to stderr so the human sees the run live
|
|
// while stdout stays a single parseable document.
|
|
|
|
// runReviewWithAudience drives a full review through runReview and returns
|
|
// (stdout, stderr, error) so both halves of the contract can be asserted at
|
|
// once: what a pipe consumer receives, and what the terminal shows.
|
|
func runReviewWithAudience(t *testing.T, repoDir, format, audience string) (string, string, error) {
|
|
t.Helper()
|
|
var err error
|
|
var out string
|
|
errOut := captureStderr(t, func() {
|
|
out = captureStdout(t, func() {
|
|
err = runReview([]string{
|
|
"--repo", repoDir, "--from", "HEAD~1", "--to", "HEAD",
|
|
"--format", format, "--audience", audience,
|
|
})
|
|
})
|
|
})
|
|
return out, errOut, err
|
|
}
|
|
|
|
func TestReviewE2E_JSONHumanStreamsProgressToStderr(t *testing.T) {
|
|
repoDir := retryTestRepo(t)
|
|
startFakeLLM(t, newFakeLLM())
|
|
|
|
out, errOut, err := runReviewWithAudience(t, repoDir, "json", "human")
|
|
if err != nil {
|
|
t.Fatalf("review must succeed: %v\nstderr: %s", err, errOut)
|
|
}
|
|
|
|
// The regression itself: the run must not be silent.
|
|
if !strings.Contains(errOut, "[ocr]") {
|
|
t.Errorf("--audience human must stream [ocr] progress to stderr, got:\n%s", errOut)
|
|
}
|
|
|
|
// stdout must remain exactly one JSON document. Unmarshal is the real
|
|
// assertion a consumer like jq cares about; a single leaked progress line
|
|
// would break it.
|
|
var doc map[string]any
|
|
if err := json.Unmarshal([]byte(out), &doc); err != nil {
|
|
t.Fatalf("stdout must be parseable JSON, got error %v for:\n%s", err, out)
|
|
}
|
|
if strings.Contains(out, "[ocr]") {
|
|
t.Errorf("no progress line may reach stdout:\n%s", out)
|
|
}
|
|
}
|
|
|
|
// TestReviewE2E_JSONAgentStaysSilent is the counterpart: audience=agent asked
|
|
// for no progress, so the stderr redirect must not resurrect it.
|
|
func TestReviewE2E_JSONAgentStaysSilent(t *testing.T) {
|
|
repoDir := retryTestRepo(t)
|
|
startFakeLLM(t, newFakeLLM())
|
|
|
|
out, errOut, err := runReviewWithAudience(t, repoDir, "json", "agent")
|
|
if err != nil {
|
|
t.Fatalf("review must succeed: %v\nstderr: %s", err, errOut)
|
|
}
|
|
|
|
if strings.Contains(errOut, "[ocr] ▶") {
|
|
t.Errorf("audience=agent must not emit tool progress, got:\n%s", errOut)
|
|
}
|
|
|
|
var doc map[string]any
|
|
if err := json.Unmarshal([]byte(out), &doc); err != nil {
|
|
t.Fatalf("stdout must be parseable JSON, got error %v for:\n%s", err, out)
|
|
}
|
|
}
|