ci: make go-ci test output visible in logs#860
Conversation
The go-ci job redirected all test output to a file, making failures invisible in CI logs. Use tee to stream output to both the log and the artifact file. Add if:always() to artifact upload so test results are downloadable even on failure. Remove redundant second go test run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdates to the PR test GitHub Actions workflow to capture Go test exit codes and produce a single JSON artifact; plus a unit-test change in gin logger recovery to directly assert and re-panic http.ErrAbortHandler behavior. Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component control flow requiring a sequence diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
added
HELIOS-CODEX
…ropagation The test relied on panic propagating back through gin's ServeHTTP, which works on macOS but not Linux. Rewrite to intercept the re-panic with a wrapper middleware, making the test deterministic across platforms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
the
ci:coderabbit-bypass
|
@coderabbitai full review Automated retrigger: CodeRabbit state=FAILURE, age=24.3m (stale after 20m). |
|
I'll perform a full review of the PR changes now. ✅ Actions performedFull review triggered. |
coderabbitai
bot
added
the
pkg:he:service-runtime
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-test-build.yml:
- Around line 59-60: The CI and local baseline generation diverge because
Taskfile.yml still runs two go test invocations and writes both
test-baseline.json and test-baseline.txt, while the workflow job named
go-test-baseline only uploads test-baseline.json from a single uncached run;
make them consistent by either (a) updating the workflow job go-test-baseline to
run the same Taskfile target(s) that produce both test-baseline.json and
test-baseline.txt and upload both artifacts (add path for test-baseline.txt
alongside test-baseline.json), or (b) change Taskfile.yml to mirror CI’s single
uncached go test run and only emit test-baseline.json; pick one approach and
apply it so Taskfile.yml, the go-test-baseline job, and the produced artifacts
(test-baseline.json, test-baseline.txt) match.
- Around line 49-50: The pipeline is redirecting stderr into the JSON artifact;
update the command that runs tests by removing the stderr redirection so only
stdout (the JSON events) are piped to tee. Specifically, change the line
invoking "go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json" to
omit "2>&1" so "go test -json -count=1 ./... | tee target/test-baseline.json"
(leave the surrounding "set +e" behavior unchanged).
In `@pkg/llmproxy/logging/gin_logger_test.go`:
- Around line 12-52: Extract the core recovery behavior into a helper
handleRecoveredPanic(c *gin.Context, recovered interface{}) and update
GinLogrusRecovery to call that helper; then replace the current
TestGinLogrusRecoveryRepanicsErrAbortHandler with a test that directly calls
handleRecoveredPanic using a minimal test gin.Context (or a stubbed context) to
assert that http.ErrAbortHandler is re-panicked, removing the need to run
engine.ServeHTTP and keeping the test under 40 lines. Ensure GinLogrusRecovery
still uses the helper so production behavior is unchanged and the new test only
exercises handleRecoveredPanic for deterministic, framework-independent
verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d35a25e-03f0-465a-8066-cae2688c2c4c
📒 Files selected for processing (2)
.github/workflows/pr-test-build.ymlpkg/llmproxy/logging/gin_logger_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: NEVER create a v2 file - refactor the original instead
NEVER create a new class if an existing one can be made generic
NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Build generic building blocks (provider interface + registry) before application logic
Use chi for HTTP routing (NOT custom routers)
Use zerolog for logging (NOT fmt.Print)
Use viper for configuration (NOT manual env parsing)
Use go-playground/validator for validation (NOT manual if/else validation)
Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Use template strings for messages instead of hardcoded messages and config-driven logic instead of code-driven
Zero new lint suppressions without inline justification
All new code must pass: go fmt, go vet, golint
Maximum function length: 40 lines
No placeholder TODOs in committed code
Files:
pkg/llmproxy/logging/gin_logger_test.go
🪛 GitHub Actions: pr-test-build
pkg/llmproxy/logging/gin_logger_test.go
[error] 44-51: GinLogrusRecovery: expected re-panic of http.ErrAbortHandler, but it did not
| set +e | ||
| go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does go test -jsonguarantee JSON events on stdout while non-JSON diagnostics from thego tool may still be written to stderr?
💡 Result:
Yes.
go test -jsonroutes the test run throughgo tool test2json, which “writes a corresponding stream of JSON events to standard output.” The JSON events include the test binary’s output (its stdout+stderr are merged intoTestEvent.Output). [1], [2]- Separately, the
gocommand’s own diagnostics (for example, failures “building the tests”) are written to thegocommand’s standard error, which is “reserved for printing errors building the tests.” So non-JSON diagnostics from thegotool may still appear on stderr even when stdout is a JSON event stream. [3]
Sources: [1] cmd/test2json docs, [2] Go 1.10 release notes, [3] cmd/go/internal/test docs/source.
Keep stderr out of test-baseline.json.
2>&1 | tee target/test-baseline.json redirects the go command's own diagnostics (e.g., build errors) into the artifact, corrupting it with non-JSON content. go test -json sends JSON events to stdout only; remove 2>&1 so stderr stays in the job log where it should be.
🔧 Proposed fix
- go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json
+ go test -json -count=1 ./... | tee target/test-baseline.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set +e | |
| go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json | |
| set +e | |
| go test -json -count=1 ./... | tee target/test-baseline.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-test-build.yml around lines 49 - 50, The pipeline is
redirecting stderr into the JSON artifact; update the command that runs tests by
removing the stderr redirection so only stdout (the JSON events) are piped to
tee. Specifically, change the line invoking "go test -json -count=1 ./... 2>&1 |
tee target/test-baseline.json" to omit "2>&1" so "go test -json -count=1 ./... |
tee target/test-baseline.json" (leave the surrounding "set +e" behavior
unchanged).
| name: go-test-baseline | ||
| path: | | ||
| target/test-baseline.json | ||
| target/test-baseline.txt | ||
| if-no-files-found: error | ||
| path: target/test-baseline.json |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CI and local baseline generation now diverge.
Taskfile.yml:91-96 still produces both test-baseline.json and test-baseline.txt via two separate go test invocations, while this workflow now uploads only the JSON artifact from a single uncached run. That makes local reproduction materially different from CI when debugging failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-test-build.yml around lines 59 - 60, The CI and local
baseline generation diverge because Taskfile.yml still runs two go test
invocations and writes both test-baseline.json and test-baseline.txt, while the
workflow job named go-test-baseline only uploads test-baseline.json from a
single uncached run; make them consistent by either (a) updating the workflow
job go-test-baseline to run the same Taskfile target(s) that produce both
test-baseline.json and test-baseline.txt and upload both artifacts (add path for
test-baseline.txt alongside test-baseline.json), or (b) change Taskfile.yml to
mirror CI’s single uncached go test run and only emit test-baseline.json; pick
one approach and apply it so Taskfile.yml, the go-test-baseline job, and the
produced artifacts (test-baseline.json, test-baseline.txt) match.
| func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) { | ||
| gin.SetMode(gin.TestMode) | ||
| // Test the recovery handler directly to avoid platform-dependent behavior | ||
| // in gin's ServeHTTP panic propagation (macOS vs Linux differ). | ||
| handler := GinLogrusRecovery() | ||
|
|
||
| gin.SetMode(gin.TestMode) | ||
| engine := gin.New() | ||
| engine.Use(GinLogrusRecovery()) | ||
|
|
||
| var repanicked bool | ||
| var repanic interface{} | ||
|
|
||
| // Wrap the recovery middleware to intercept the re-panic before gin's | ||
| // outer recovery can swallow it. | ||
| engine.Use(func(c *gin.Context) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| repanicked = true | ||
| repanic = r | ||
| } | ||
| }() | ||
| handler(c) | ||
| }) | ||
| engine.GET("/abort", func(c *gin.Context) { | ||
| panic(http.ErrAbortHandler) | ||
| }) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/abort", nil) | ||
| recorder := httptest.NewRecorder() | ||
|
|
||
| defer func() { | ||
| recovered := recover() | ||
| if recovered == nil { | ||
| t.Fatalf("expected panic, got nil") | ||
| } | ||
| err, ok := recovered.(error) | ||
| if !ok { | ||
| t.Fatalf("expected error panic, got %T", recovered) | ||
| } | ||
| if !errors.Is(err, http.ErrAbortHandler) { | ||
| t.Fatalf("expected ErrAbortHandler, got %v", err) | ||
| } | ||
| if err != http.ErrAbortHandler { | ||
| t.Fatalf("expected exact ErrAbortHandler sentinel, got %v", err) | ||
| } | ||
| }() | ||
|
|
||
| engine.ServeHTTP(recorder, req) | ||
|
|
||
| if !repanicked { | ||
| t.Fatalf("expected GinLogrusRecovery to re-panic http.ErrAbortHandler, but it did not") | ||
| } | ||
| err, ok := repanic.(error) | ||
| if !ok { | ||
| t.Fatalf("expected error panic, got %T", repanic) | ||
| } | ||
| if !errors.Is(err, http.ErrAbortHandler) { | ||
| t.Fatalf("expected ErrAbortHandler, got %v", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Gin 1.10.1, if a gin.CustomRecoverycallback re-panics, is that panic guaranteed to be observable by an outer middleware that invokes the returned handler directly instead of registering it withengine.Use?
💡 Result:
Yes — the re-panic will propagate out of the gin.CustomRecovery-returned handler to its direct caller, so an “outer” middleware that wraps and directly invokes that handler can observe (and recover) the panic.
In Gin v1.10.1, the recovery middleware does recover() in a deferred function and then calls your handle(c, err) without any additional recover around that call; so if handle panics, that panic is not caught by Gin’s recovery middleware and unwinds to the caller of the middleware handler. (raw.githubusercontent.com)
If nothing in your stack recovers it, the panic may still be recovered by Go’s net/http server boundary (logging a stack trace and closing/resetting the stream), but that’s outside Gin. (pkg.go.dev)
Citations:
- 1: https://raw.githubusercontent.com/gin-gonic/gin/v1.10.1/recovery.go
- 2: https://pkg.go.dev/net/http
Extract the recovery callback into a testable helper function.
The current test exceeds the 40-line function length limit (41 lines) and remains coupled to Gin's panic propagation semantics, which has introduced a failure in CI. While gin.CustomRecovery callbacks do propagate re-panics to their direct caller, routing through engine.ServeHTTP introduces additional panic recovery boundaries that prevent the outer middleware from observing the panic.
Extract the recovery logic into a standalone handleRecoveredPanic(c *gin.Context, recovered interface{}) helper and test it directly with a minimal test context instead of routing through the full HTTP server pipeline. This isolates the behavior you actually care about, eliminates framework complexity, and satisfies the maximum function length guideline.
🔧 Proposed refactor
diff --git a/pkg/llmproxy/logging/gin_logger.go b/pkg/llmproxy/logging/gin_logger.go
@@
+func handleRecoveredPanic(c *gin.Context, recovered interface{}) {
+ if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) {
+ panic(http.ErrAbortHandler)
+ }
+
+ log.WithFields(log.Fields{
+ "panic": recovered,
+ "stack": string(debug.Stack()),
+ "path": c.Request.URL.Path,
+ }).Error("recovered from panic")
+
+ c.AbortWithStatus(http.StatusInternalServerError)
+}
+
func GinLogrusRecovery() gin.HandlerFunc {
- return gin.CustomRecovery(func(c *gin.Context, recovered interface{}) {
- if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) {
- panic(http.ErrAbortHandler)
- }
-
- log.WithFields(log.Fields{
- "panic": recovered,
- "stack": string(debug.Stack()),
- "path": c.Request.URL.Path,
- }).Error("recovered from panic")
-
- c.AbortWithStatus(http.StatusInternalServerError)
- })
+ return gin.CustomRecovery(handleRecoveredPanic)
}
diff --git a/pkg/llmproxy/logging/gin_logger_test.go b/pkg/llmproxy/logging/gin_logger_test.go
@@
func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) {
- // Test the recovery handler directly to avoid platform-dependent behavior
- // in gin's ServeHTTP panic propagation (macOS vs Linux differ).
- handler := GinLogrusRecovery()
-
gin.SetMode(gin.TestMode)
- engine := gin.New()
-
- var repanicked bool
- var repanic interface{}
-
- // Wrap the recovery middleware to intercept the re-panic before gin's
- // outer recovery can swallow it.
- engine.Use(func(c *gin.Context) {
- defer func() {
- if r := recover(); r != nil {
- repanicked = true
- repanic = r
- }
- }()
- handler(c)
- })
- engine.GET("/abort", func(c *gin.Context) {
- panic(http.ErrAbortHandler)
- })
-
- req := httptest.NewRequest(http.MethodGet, "/abort", nil)
- recorder := httptest.NewRecorder()
-
- engine.ServeHTTP(recorder, req)
-
- if !repanicked {
- t.Fatalf("expected GinLogrusRecovery to re-panic http.ErrAbortHandler, but it did not")
- }
- err, ok := repanic.(error)
- if !ok {
- t.Fatalf("expected error panic, got %T", repanic)
- }
- if !errors.Is(err, http.ErrAbortHandler) {
- t.Fatalf("expected ErrAbortHandler, got %v", err)
- }
+ c, _ := gin.CreateTestContext(httptest.NewRecorder())
+ c.Request = httptest.NewRequest(http.MethodGet, "/abort", nil)
+
+ defer func() {
+ r := recover()
+ err, ok := r.(error)
+ if !ok || !errors.Is(err, http.ErrAbortHandler) {
+ t.Fatalf("expected ErrAbortHandler, got %T (%v)", r, r)
+ }
+ }()
+
+ handleRecoveredPanic(c, http.ErrAbortHandler)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) { | |
| gin.SetMode(gin.TestMode) | |
| // Test the recovery handler directly to avoid platform-dependent behavior | |
| // in gin's ServeHTTP panic propagation (macOS vs Linux differ). | |
| handler := GinLogrusRecovery() | |
| gin.SetMode(gin.TestMode) | |
| engine := gin.New() | |
| engine.Use(GinLogrusRecovery()) | |
| var repanicked bool | |
| var repanic interface{} | |
| // Wrap the recovery middleware to intercept the re-panic before gin's | |
| // outer recovery can swallow it. | |
| engine.Use(func(c *gin.Context) { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| repanicked = true | |
| repanic = r | |
| } | |
| }() | |
| handler(c) | |
| }) | |
| engine.GET("/abort", func(c *gin.Context) { | |
| panic(http.ErrAbortHandler) | |
| }) | |
| req := httptest.NewRequest(http.MethodGet, "/abort", nil) | |
| recorder := httptest.NewRecorder() | |
| defer func() { | |
| recovered := recover() | |
| if recovered == nil { | |
| t.Fatalf("expected panic, got nil") | |
| } | |
| err, ok := recovered.(error) | |
| if !ok { | |
| t.Fatalf("expected error panic, got %T", recovered) | |
| } | |
| if !errors.Is(err, http.ErrAbortHandler) { | |
| t.Fatalf("expected ErrAbortHandler, got %v", err) | |
| } | |
| if err != http.ErrAbortHandler { | |
| t.Fatalf("expected exact ErrAbortHandler sentinel, got %v", err) | |
| } | |
| }() | |
| engine.ServeHTTP(recorder, req) | |
| if !repanicked { | |
| t.Fatalf("expected GinLogrusRecovery to re-panic http.ErrAbortHandler, but it did not") | |
| } | |
| err, ok := repanic.(error) | |
| if !ok { | |
| t.Fatalf("expected error panic, got %T", repanic) | |
| } | |
| if !errors.Is(err, http.ErrAbortHandler) { | |
| t.Fatalf("expected ErrAbortHandler, got %v", err) | |
| } | |
| func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) { | |
| gin.SetMode(gin.TestMode) | |
| c, _ := gin.CreateTestContext(httptest.NewRecorder()) | |
| c.Request = httptest.NewRequest(http.MethodGet, "/abort", nil) | |
| defer func() { | |
| r := recover() | |
| err, ok := r.(error) | |
| if !ok || !errors.Is(err, http.ErrAbortHandler) { | |
| t.Fatalf("expected ErrAbortHandler, got %T (%v)", r, r) | |
| } | |
| }() | |
| handleRecoveredPanic(c, http.ErrAbortHandler) | |
| } |
🧰 Tools
🪛 GitHub Actions: pr-test-build
[error] 44-51: GinLogrusRecovery: expected re-panic of http.ErrAbortHandler, but it did not
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/llmproxy/logging/gin_logger_test.go` around lines 12 - 52, Extract the
core recovery behavior into a helper handleRecoveredPanic(c *gin.Context,
recovered interface{}) and update GinLogrusRecovery to call that helper; then
replace the current TestGinLogrusRecoveryRepanicsErrAbortHandler with a test
that directly calls handleRecoveredPanic using a minimal test gin.Context (or a
stubbed context) to assert that http.ErrAbortHandler is re-panicked, removing
the need to run engine.ServeHTTP and keeping the test under 40 lines. Ensure
GinLogrusRecovery still uses the helper so production behavior is unchanged and
the new test only exercises handleRecoveredPanic for deterministic,
framework-independent verification.
Extract ginLogrusRecoveryFunc so tests can verify re-panic behavior without depending on gin.CustomRecovery's internal panic propagation, which differs between macOS and Linux. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
> target/test-baseline.json, making failures completely invisible in CI logsteeto stream output to both the CI log and the artifact fileif: always()to artifact upload so test results are downloadable even on failurego testinvocation (was running the full suite twice)-count=1to disable test caching in CI for deterministic resultsTest plan
Generated with Claude Code
Summary by CodeRabbit