fix integration tests#264
Merged
alienvspredator merged 1 commit intomainfrom Mar 25, 2026
Merged
Conversation
push-tags-from-workflow
Bot
added
bug
📝 WalkthroughWalkthroughThis pull request refactors test infrastructure by removing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/grpc/oidcmapping.go (1)
103-104: Consider downgradingErrNotFoundlog level toDebugto reduce noise.If remove is intentionally idempotent, missing-tenant removals are an expected path and
Warncan create alert fatigue.♻️ Optional log-level tweak
- } else { - slogctx.Warn(ctx, "RemoveOIDCMapping is called but the tenant does not exist", "error", err) + } else { + slogctx.Debug(ctx, "RemoveOIDCMapping called for non-existent tenant", "error", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/grpc/oidcmapping.go` around lines 103 - 104, The current branch in RemoveOIDCMapping logs missing-tenant errors with slogctx.Warn which is noisy for idempotent removals; change the logic to detect the not-found condition (e.g., errors.Is(err, ErrNotFound) or comparing with ErrNotFound) and log it at Debug level with slogctx.Debug (including the same context/error), while retaining slogctx.Warn for other unexpected errors so only real failures remain at warn level.integration/session_grpc_test.go (1)
205-210: Same scoping suggestion as ingrpc_test.go.The conditional logging is correct. For consistency with the suggestion in
grpc_test.go, consider usingerr :=to scope the variable to the goroutine and avoid the theoretical data race.Suggested fix
// start go func() { - err = srv.Serve(lis) + err := srv.Serve(lis) if err != nil { slogctx.Error(ctx, "error while starting session server", "error", err) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/session_grpc_test.go` around lines 205 - 210, The goroutine that calls srv.Serve(lis) reuses the outer variable err; change it to declare a new scoped variable inside the goroutine (use err := srv.Serve(lis)) so the error returned by srv.Serve is local to that goroutine and avoids a potential data race when logging with slogctx.Error; update the anonymous goroutine block that currently assigns to err to instead use a short declaration and keep the existing conditional log using the local err variable.integration/grpc_test.go (1)
333-338: Conditional error logging looks good, but consider scopingerrto the goroutine.The
if err != nilguard correctly avoids logging nil on graceful shutdown. However,err = srv.Serve(lis)assigns to the outer function'serrvariable (declared at line 324), creating a theoretical data race since the outer function returns before the goroutine runs.Suggested fix: scope `err` to the goroutine
// start go func() { - err = srv.Serve(lis) + err := srv.Serve(lis) if err != nil { slogctx.Error(ctx, "error while starting server", "error", err) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/grpc_test.go` around lines 333 - 338, The goroutine currently assigns to the outer err variable (err = srv.Serve(lis)), which can create a data race; change the goroutine to declare a local error variable instead (e.g., use err := srv.Serve(lis) or localErr := srv.Serve(lis)) and use that local variable in the conditional and slogctx.Error call so the outer err is not mutated; ensure this change is applied inside the anonymous func that calls srv.Serve(lis) and that slogctx.Error still logs the local error and uses ctx.internal/business/business.go (1)
58-58: Return a non-nil error if either server returns one.Current return logic can still mask an abnormal shutdown if the first received value is
niland the second result is non-nil. Prefer selecting a non-nil error across both goroutine results.♻️ Suggested adjustment
- err := <-errChan - if err != nil { - slogctx.Error(ctx, "Shutting down servers", "error", err) + firstErr := <-errChan + if firstErr != nil { + slogctx.Error(ctx, "Shutting down servers", "error", firstErr) } cancel() // wait for all servers to shutdown wg.Wait() - return err + var secondErr error + select { + case secondErr = <-errChan: + default: + } + if firstErr != nil { + return firstErr + } + return secondErr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/business/business.go` at line 58, The current return statement just returns err which can mask a non-nil error from the other goroutine; after collecting both goroutine results (the two error variables you read from channels, e.g., err and err2 or err1/err2), change the return logic to return the first non-nil error (for example: if err != nil { return err } return err2) so a non-nil error from either server is propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration/infra_test.go`:
- Around line 57-60: The current length check uses istat.Cfg.HTTP.Address which
includes the "unix://" scheme, so it overestimates the socket path length;
update the check to extract the actual filesystem path (e.g., remove the
"unix://" prefix or parse the URL) from istat.Cfg.HTTP.Address and validate that
the resulting path length is <= 108, then call t.Fatal with the same message if
the path is too long; ensure you reference istat.Cfg.HTTP.Address when making
the extraction and keep the original test behavior otherwise.
---
Nitpick comments:
In `@integration/grpc_test.go`:
- Around line 333-338: The goroutine currently assigns to the outer err variable
(err = srv.Serve(lis)), which can create a data race; change the goroutine to
declare a local error variable instead (e.g., use err := srv.Serve(lis) or
localErr := srv.Serve(lis)) and use that local variable in the conditional and
slogctx.Error call so the outer err is not mutated; ensure this change is
applied inside the anonymous func that calls srv.Serve(lis) and that
slogctx.Error still logs the local error and uses ctx.
In `@integration/session_grpc_test.go`:
- Around line 205-210: The goroutine that calls srv.Serve(lis) reuses the outer
variable err; change it to declare a new scoped variable inside the goroutine
(use err := srv.Serve(lis)) so the error returned by srv.Serve is local to that
goroutine and avoids a potential data race when logging with slogctx.Error;
update the anonymous goroutine block that currently assigns to err to instead
use a short declaration and keep the existing conditional log using the local
err variable.
In `@internal/business/business.go`:
- Line 58: The current return statement just returns err which can mask a
non-nil error from the other goroutine; after collecting both goroutine results
(the two error variables you read from channels, e.g., err and err2 or
err1/err2), change the return logic to return the first non-nil error (for
example: if err != nil { return err } return err2) so a non-nil error from
either server is propagated.
In `@internal/grpc/oidcmapping.go`:
- Around line 103-104: The current branch in RemoveOIDCMapping logs
missing-tenant errors with slogctx.Warn which is noisy for idempotent removals;
change the logic to detect the not-found condition (e.g., errors.Is(err,
ErrNotFound) or comparing with ErrNotFound) and log it at Debug level with
slogctx.Debug (including the same context/error), while retaining slogctx.Warn
for other unexpected errors so only real failures remain at warn level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eeea47ca-1602-4c36-992a-5c8b1c0b6222
📒 Files selected for processing (13)
Makefilecmd/session-manager/main.gointegration/api-server-status_test.gointegration/api-server_test.gointegration/grpc_test.gointegration/housekeeper_test.gointegration/infra_test.gointegration/session_grpc_test.gointernal/business/business.gointernal/business/business_test.gointernal/business/server/http_server.gointernal/cmdutils/cmdutils.gointernal/grpc/oidcmapping.go
cb80
approved these changes
Mar 24, 2026
alienvspredator
requested review from
OlegGitH,
apatsap,
pabloszel11,
petersbingham and
protectorT1
OlegGitH
approved these changes
Mar 25, 2026
OlegGitH
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes various errors in the integration tests:
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores