fix: added emulator tests for all the modules #243
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRefactors call-profile, call-profiles, and health-check to use a deps struct with injected Firestore client/context and converts handlers to methods. Adds Firebase emulator configs, expands integration tests using emulator-backed Firestore, updates test runner to execute Firebase modules under emulators, and adjusts go.mod to directly require gRPC. Minor .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor AWS as AWS Lambda
participant Main as main()
participant Deps as deps
participant H as handler
participant FS as Firestore
participant HTTP as External HTTP (profiles/health)
AWS->>Main: invoke
Main->>Deps: Initialize ctx, Firestore client
Main->>H: lambda.Start(d.handler)
H->>FS: Read/Write user/profile docs
alt External calls needed
H->>HTTP: Fetch profile/health
HTTP-->>H: Response/Errors
end
H-->>AWS: APIGatewayProxyResponse
sequenceDiagram
autonumber
actor Dev as Test Runner
participant Script as scripts/test.sh
participant Emu as Firebase Emulators
participant Tests as go test (module)
participant FS as Firestore Emulator
Dev->>Script: run tests
Script->>Emu: emulators:exec (per firebase_module)
Emu->>Tests: start go test
Tests->>FS: Read/Write test data
FS-->>Tests: Responses
Tests-->>Emu: exit code + coverage
Emu-->>Script: exit status
Script-->>Dev: aggregate results and coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Context stored in struct ▹ view | |
 |
Silent failure on Firestore client initialization ▹ view | ✅ Fix detected |
 |
Parallelize Firebase emulator tests ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| health-check/main.go | ✅ |
| scripts/test.sh | ✅ |
| call-profiles/main.go | ✅ |
| call-profile/main.go | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
verify/main_test.go (2)
21-28: Don’t exit the process from tests; enforce emulator usage and close the client.
- log.Fatalf ends the whole test run; prefer t.Fatalf.
- Ensure FIRESTORE_EMULATOR_HOST is set so tests never hit real Firestore.
- Close the client via t.Cleanup to avoid leaks.
-func newFirestoreMockClient(ctx context.Context) *firestore.Client { - client, err := firestore.NewClient(ctx, "test") +func newFirestoreMockClient(t *testing.T, ctx context.Context) *firestore.Client { + t.Helper() + if os.Getenv("FIRESTORE_EMULATOR_HOST") == "" { + t.Fatalf("FIRESTORE_EMULATOR_HOST must be set (e.g., 127.0.0.1:8090)") + } + client, err := firestore.NewClient(ctx, "test") if err != nil { - log.Fatalf("firebase.NewClient err: %v", err) + t.Fatalf("firestore.NewClient err: %v", err) } - - return client + t.Cleanup(func() { _ = client.Close() }) + return client }Update call sites accordingly, e.g.:
- client := newFirestoreMockClient(ctx) + client := newFirestoreMockClient(t, ctx)
156-160: Use t.Setenv for test env and rely on t.Cleanup for client close.Replace manual Setenv/Unsetenv and avoid forgetting cleanup.
- os.Setenv("environment", "test") - defer os.Unsetenv("environment") + t.Setenv("environment", "test")(With the helper change above, client close is automatic.)
call-profile/main.go (2)
42-51: Guard against empty profileURL to avoid index-out-of-range panic.If profileURL is "", userUrl[len(userUrl)-1] panics. Treat empty as missing and block early.
- if str, ok := dsnap.Data()["profileURL"].(string); ok { - userUrl = str - } else { - utils.LogProfileSkipped(d.client, d.ctx, "Profile URL not available", userId, sessionId) - utils.SetProfileStatusBlocked(d.client, d.ctx, userId, "Profile URL not available", sessionId, discordId) - return events.APIGatewayProxyResponse{ - Body: "Profile Skipped No Profile URL", - StatusCode: 200, - }, nil - } + if str, ok := dsnap.Data()["profileURL"].(string); ok && str != "" { + userUrl = str + } else { + utils.LogProfileSkipped(d.client, d.ctx, "Profile URL not available", userId, sessionId) + utils.SetProfileStatusBlocked(d.client, d.ctx, userId, "Profile URL not available", sessionId, discordId) + return events.APIGatewayProxyResponse{ + Body: "Profile Skipped No Profile URL", + StatusCode: 200, + }, nil + } @@ - if userUrl[len(userUrl)-1] != '/' { + if userUrl[len(userUrl)-1] != '/' { userUrl = userUrl + "/" }Also applies to: 82-84
86-94: Close health-check response body to avoid leaking connections.The response from c.Get is never closed, leaking TCP connections in the Lambda container.
- _, serviceErr := c.Get(userUrl + "health") - if serviceErr != nil { - isServiceRunning = false - } else { - isServiceRunning = true - } + resp, serviceErr := c.Get(userUrl + "health") + if serviceErr != nil { + isServiceRunning = false + } else { + isServiceRunning = true + resp.Body.Close() + }health-check/main_test.go (1)
41-52: Fix captured loop variables in parallel subtests (flaky tests).You’re closing over the range variable while calling t.Parallel(). Rebind per-iteration to avoid data races/flakiness.
Apply this pattern and repeat for the other loops below:
-for _, test := range tests { - t.Run(test.name, func(t *testing.T) { +for _, test := range tests { + tc := test + t.Run(tc.name, func(t *testing.T) { t.Parallel() - url := test.userUrl + url := tc.userUrl ... }) }Also applies to: 57-68, 71-95, 98-110, 113-125, 128-159, 161-175, 177-202, 204-222, 224-254, 257-294, 370-384
call-profile/main_test.go (1)
22-30: Fix captured loop variables in parallel subtests (flaky tests).Rebind the range variable before t.Run. Example (apply to each block listed):
-for _, test := range GetDataFromBodyTests { - t.Run(test.Name, func(t *testing.T) { +for _, test := range GetDataFromBodyTests { + tc := test + t.Run(tc.Name, func(t *testing.T) { t.Parallel() - userId, sessionId := utils.GetDataFromBody([]byte(test.Body)) - assert.Equal(t, test.ExpectedUserId, userId, test.Description) - assert.Equal(t, test.ExpectedSessionId, sessionId, test.Description) + userId, sessionId := utils.GetDataFromBody([]byte(tc.Body)) + assert.Equal(t, tc.ExpectedUserId, userId, tc.Description) + assert.Equal(t, tc.ExpectedSessionId, sessionId, tc.Description) }) }Also applies to: 33-54, 57-70, 73-86, 130-142, 156-170, 182-191, 203-217, 245-254, 269-283
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)call-profile/firebase.json(1 hunks)call-profile/main.go(6 hunks)call-profile/main_test.go(2 hunks)call-profiles/firebase.json(1 hunks)call-profiles/main.go(5 hunks)call-profiles/main_test.go(2 hunks)go.mod(1 hunks)health-check/firebase.json(1 hunks)health-check/main.go(4 hunks)health-check/main_test.go(3 hunks)scripts/test.sh(2 hunks)verify/main_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
call-profiles/main.go (1)
layer/utils/firestore.go (1)
InitializeFirestoreClient(25-38)
health-check/main.go (1)
layer/utils/firestore.go (1)
InitializeFirestoreClient(25-38)
health-check/main_test.go (2)
call-profile/main_test.go (2)
TestHandlerIntegration(285-498)TestHandlerWithRealFirestore(500-563)call-profiles/main_test.go (2)
TestHandlerIntegration(307-419)TestHandlerWithRealFirestore(421-435)
verify/main_test.go (2)
call-profile/main_test.go (2)
TestURLFormatting(56-70)TestHandlerEdgeCases(565-624)health-check/main_test.go (1)
TestURLFormatting(56-68)
call-profile/main_test.go (2)
call-profiles/main_test.go (1)
TestHandlerIntegration(307-419)verify/main_test.go (1)
TestHandlerEdgeCases(432-518)
call-profiles/main_test.go (1)
health-check/main_test.go (2)
TestHandlerIntegration(401-537)TestHandlerWithRealFirestore(577-591)
call-profile/main.go (3)
layer/utils/logging.go (2)
LogProfileSkipped(29-44)LogHealth(46-61)layer/utils/firestore.go (3)
SetProfileStatusBlocked(77-113)Getdata(115-194)InitializeFirestoreClient(25-38)layer/utils/validation.go (1)
DiffToRes(32-47)
🪛 Shellcheck (0.11.0)
scripts/test.sh
[warning] 29-29: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run_tests
🔇 Additional comments (5)
verify/main_test.go (1)
277-280: Repeat: pass testing.T to helper and drop manual client lifecycle.Apply the new helper signature here as well to ensure cleanup:
- client := newFirestoreMockClient(ctx) + client := newFirestoreMockClient(t, ctx)Also applies to: 367-370, 436-439
go.mod (1)
17-17: gRPC dependency consistency verified. go.sum shows only google.golang.org/grpc v1.72.2 and go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.61.0; no additional grpc versions are pulled transitively.health-check/firebase.json (1)
1-11: Emulator config looks good; confirm port alignment across modules/CI.Port 8090 matches tests; ensure all emulator invocations and FIRESTORE_EMULATOR_HOST use 127.0.0.1:8090 in CI to avoid flaky connections. Optionally pin a test projectId for isolation.
health-check/main.go (1)
31-36: Good guard to skip empty URLs.Prevents slice-bounds panic and wasted calls. LGTM.
scripts/test.sh (1)
38-56: Emulator port configuration matches kill command
Allfirebase.jsonfiles configure the Firestore emulator on port 8090, so the existingkillcommand is correct.
4 participants
Date: 29/09/2025
Developer Name: @lakshayman
Issue Ticket Number
#241
Description
Added firestore emulator tests in all the tests modules
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Add emulator-backed tests for all modules by refactoring handlers to accept a Firestore client via a deps struct, introducing comprehensive integration tests and test scaffolding that exercise health, profile calls, and verification flows against a Firestore emulator.
Why are these changes being made?
To ensure end-to-end validation of all modules with Firebase Firestore emulator support, improving test coverage and reliability across integration scenarios. This approach centralizes dependencies (Firestore client and context) for easier testing and better isolation of emulator-based test flows.