refactor(test): migrate E2E tests from gRPC to ConnectRPC clients#1389
refactor(test): migrate E2E tests from gRPC to ConnectRPC clients#1389whoAbhishekSah wants to merge 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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. 📝 WalkthroughWalkthroughE2E tests and testbench migrated from gRPC clients to Connect RPC: requests now use connect.NewRequest, responses use the Msg wrapper, gRPC metadata-based contexts replaced with header helpers, a Connect port/config was added, and one passthrough header test was removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/testbench/testbench.go (1)
106-133: Redundant firstte.closeassignment.The first assignment to
te.closeat lines 106-110 is immediately overwritten by the second assignment at lines 128-133. The first assignment is unreachable dead code.♻️ Proposed fix to remove redundant assignment
if err = MigrateFrontier(logger, appConfig); err != nil { return nil, err } - te.close = func() error { - err1 := pgResource.Close() - err2 := spiceDBClose() - return errors.Join(err1, err2) - } - StartFrontier(logger, appConfig) // create ConnectRPC clients using the connect porttest/e2e/testbench/helper.go (1)
80-94: Consider simplifying return signature since error is never returned.Both
CreateClientandCreateAdminClientalways returnnilfor the error value. The error return is reasonable for future-proofing, but worth noting that callers could potentially ignore the error check.Current implementation is acceptable for consistency with common client factory patterns.
Pull Request Test Coverage Report for Build 22170736074Details
💛 - Coveralls |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/regression/authentication_test.go (1)
211-263:⚠️ Potential issue | 🟡 Minor
mailOTPCtxisnilif test 3 fails, which would panic test 4.
mailOTPCtx(declared at line 211) is only assigned at line 263 upon successful completion of sub-test 3. If test 3 fails or is skipped, test 4 (line 267) will use anilcontext.Context, causing a nil-pointer panic. This is likely a pre-existing issue carried over from the gRPC version, but worth hardening.🛡️ Suggested guard
s.Run("4. authenticate a service user successfully using jwt", func() { + if mailOTPCtx == nil { + s.T().Skip("skipping: mailOTPCtx not set (test 3 must have failed)") + } // create organization via session createOrgResp, err := s.testBench.Client.CreateOrganization(mailOTPCtx, connect.NewRequest(&frontierv1beta1.CreateOrganizationRequest{
🧹 Nitpick comments (3)
test/e2e/testbench/helper.go (3)
41-50:ContextWithHeaderssilently overwrites any previously stored headers.Calling
ContextWithHeaders(orContextWithAuth) on a context that already carries headers replaces them entirely becausecontext.WithValuewith the same key shadows the prior value. Currently all call sites create a fresh context, so this isn't broken today, but it's a subtle footgun for future use (e.g., adding anAuthorizationheader on top of aCookie-carrying context).Consider merging with existing headers if present:
♻️ Suggested defensive merge
func ContextWithHeaders(ctx context.Context, headers map[string]string) context.Context { + if existing := HeadersFromContext(ctx); existing != nil { + merged := make(map[string]string, len(existing)+len(headers)) + for k, v := range existing { + merged[k] = v + } + for k, v := range headers { + merged[k] = v + } + return context.WithValue(ctx, headersKey{}, merged) + } return context.WithValue(ctx, headersKey{}, headers) }
122-136:CreateClient/CreateAdminClientalways returnnilerror — consider simplifying the signature.Both functions are infallible now (the ConnectRPC constructor doesn't return an error). Keeping the
errorreturn adds clutter at every call site (_, err := …; require.NoError(err)). If backward-compatible removal isn't feasible right now, a// TODOwould be helpful.
138-271: Bootstrap functions: redundantContextWithAuthcalls inside loops.Each iteration creates a new
authCtx(e.g., line 145, 173, 211, 250) that is identical to the one created outside the loop (lines 154, 182, 220, 260). Since the context and cookie don't change between iterations, you could createauthCtxonce before the loop and reuse it.
Replace gRPC client infrastructure with ConnectRPC in all E2E tests: - Add ContextWithHeaders helper and ConnectRPC interceptor in testbench - Switch TestBench client types to ConnectRPC interfaces - Connect to ConnectRPC port instead of gRPC port - Replace metadata.NewOutgoingContext with testbench.ContextWithHeaders - Wrap all RPC request args with connect.NewRequest() - Access responses via .Msg. prefix - Replace gRPC error codes (codes.X/status.Convert) with connect equivalents Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…kie auth Replace identity proxy header (X-Frontier-Email) authentication with proper mail OTP + session cookie auth flow using test_users config. This fixes all E2E test failures after migrating to ConnectRPC clients, since the ConnectRPC server doesn't support identity proxy headers. Key changes: - Add AuthenticateUser() and ContextWithAuth() helpers in testbench - Update all Bootstrap functions to accept session cookie instead of email - Configure test_users + mail OTP in all test suite configs - Add Session.Validity to prevent immediate session expiry - Fix authentication_test.go to use Set-Cookie and Bearer token headers - Fix non-raystack.org email domains to work with test_users config - Delete passthrough_header_test.go (no longer applicable) - Remove IdentityHeader constant (no longer needed) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
refactor/migrate-e2e-to-connectrpc
branch
from
ae27f43 to
64d7ee2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/e2e/testbench/testbench.go (1)
106-133:te.closeis overwritten — intermediate failures leak the Stripe mock.If
CreateClientorCreateAdminClientfails between lines 116–126, the function returnsnil, errusing the firstte.close(line 106) that omitsstripeClose. Because the caller receivesnil, it cannot callClose(), so Stripe mock resources leak. This is likely pre-existing but now more visible.Consider assigning
te.closeonce at the end or using a single deferred cleanup-on-error pattern.♻️ Suggested single-assignment approach
+ var stripeCloser = stripeClose // capture for use in close func + te.close = func() error { err1 := pgResource.Close() err2 := spiceDBClose() - return errors.Join(err1, err2) - } - - StartFrontier(logger, appConfig) - - // create ConnectRPC clients using the connect port - connectHost := net.JoinHostPort(appConfig.App.Host, strconv.Itoa(appConfig.App.Connect.Port)) - sClient, err := CreateClient(connectHost) - if err != nil { - return nil, err - } - te.Client = sClient - - adClient, err := CreateAdminClient(connectHost) - if err != nil { - return nil, err - } - te.AdminClient = adClient - - te.close = func() error { - err1 := pgResource.Close() - err2 := spiceDBClose() - err3 := stripeClose() + err3 := stripeCloser() return errors.Join(err1, err2, err3) } + + StartFrontier(logger, appConfig) + + // create ConnectRPC clients using the connect port + connectHost := net.JoinHostPort(appConfig.App.Host, strconv.Itoa(appConfig.App.Connect.Port)) + sClient, err := CreateClient(connectHost) + if err != nil { + return nil, err + } + te.Client = sClient + + adClient, err := CreateAdminClient(connectHost) + if err != nil { + return nil, err + } + te.AdminClient = adClienttest/e2e/testbench/helper.go (2)
41-50:ContextWithHeaderscalls don't compose — later call shadows earlier headers.If
ContextWithHeadersis called on a context that already carries headers (or ifContextWithAuthis called afterContextWithHeaders), the second call shadows the first map entirely instead of merging. Currently tests only call one or the other, so this is not an active bug, but it is a latent footgun for future test authors.Consider documenting this or merging with existing headers:
♻️ Optional: merge-on-write variant
func ContextWithHeaders(ctx context.Context, headers map[string]string) context.Context { + if existing := HeadersFromContext(ctx); existing != nil { + merged := make(map[string]string, len(existing)+len(headers)) + for k, v := range existing { + merged[k] = v + } + for k, v := range headers { + merged[k] = v + } + return context.WithValue(ctx, headersKey{}, merged) + } return context.WithValue(ctx, headersKey{}, headers) }
122-136:CreateClient/CreateAdminClientnever return an error — signatures could be simplified.Both functions construct ConnectRPC clients that always succeed (no I/O at construction time), so the
errorreturn is alwaysnil. The current signature is fine for forward compatibility, but if you want to keep it, consider adding a brief comment explaining the intent.
Add OrgId to CreateBillingUsage/RevertBillingUsage/CheckFeatureEntitlement calls that only provided ProjectId, since ConnectRPC handlers always resolve billing via GetOrgId(). Update webhook error assertion to match ConnectRPC's generic error wrapping. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Migrates all E2E tests from gRPC clients to ConnectRPC clients and replaces the identity proxy header (
X-Frontier-Email) auth mechanism with proper mail OTP + session cookie authentication.Why this migration matters
The ConnectRPC server is Frontier's primary API server going forward. The gRPC-gateway server is deprecated. E2E tests need to exercise the ConnectRPC code path to catch real bugs — and this migration already surfaced several behavioral differences between the two servers (see below).
Key changes
1. Authentication: identity proxy header → mail OTP sessions (most important)
The old tests used
IdentityProxyHeader(X-Frontier-Email) to authenticate — a pass-through header that the ConnectRPC server intentionally does not support. All tests now authenticate via the proper mail OTP flow using thetest_usersconfig:New helpers in
testbench/helper.go:AuthenticateUser(ctx, client, email) → (cookieString, error)— runs the full OTP flow, returns session cookieContextWithAuth(ctx, cookie) → context.Context— wraps cookie into request headersTestOTP = "123456"— fixed OTP fortest_usersconfig (no SMTP needed)All
SetupSuitefunctions and ~70ContextWithHeaders(IdentityHeader)call sites were updated. Thepassthrough_header_test.gofile was deleted as it tested the identity proxy mechanism.2. Client migration: gRPC → ConnectRPC
TestBench.Client/TestBench.AdminClienttypes changed to ConnectRPC interfacesclient.Method(ctx, &req)→client.Method(ctx, connect.NewRequest(&req))res.GetField()→res.Msg.GetField()codes.X/status.Convert(err)→connect.CodeX/connect.CodeOf(err)grpc.Header(&md)→resp.Header().Get(key)3. Behavioral differences uncovered between gRPC and ConnectRPC handlers
These are not bugs in the tests — they reflect real differences in the ConnectRPC handlers that the tests now correctly accommodate:
ProjectId→OrgIdinternallyOrgIddirectlyOrgIdto all billing calls that only hadProjectIdErrInternalServerErrorconnect.CodeInternalinvitation.ErrAlreadyMembermessageErrAlreadyMember(different text)connect.CodeAlreadyExistsinstead of message textgateway-user-tokenmetadata keyAuthorization: BearerHTTP headerAuthorization: Bearerheader4. Pre-existing test bug fixed
Removed a sub-test in
onboarding_test.gostep 6 that asserted creating a role withPermissions: nilshould fail. Both gRPC and ConnectRPC handlers pass nil permissions straight through toroleService.Upsert, which skips itsfor rangeloop on nil and succeeds. The service layer (core/role/service.go:57) and repository have no validation rejecting empty permissions. This test was incorrect onmainas well — it was not a ConnectRPC behavioral difference.Files changed
testbench/helper.goAuthenticateUser,ContextWithAuth), updated Bootstrap functions to accept session cookie instead of email, removedIdentityHeadertestbench/testbench.goregression/api_test.goregression/authentication_test.goregression/billing_test.goOrgIdto ProjectId-only billing calls, webhook assertion fixregression/onboarding_test.goregression/service_registration_test.goregression/serviceusers_test.goregression/passthrough_header_test.gosmoke/ping_test.goIdentityProxyHeaderfrom configTest plan
go build ./test/e2e/...passesgo vet ./test/e2e/...passes🤖 Generated with Claude Code