refactor: migrate block handler tests to typed response pattern#915
refactor: migrate block handler tests to typed response pattern#915Ahmedhossamdev wants to merge 3 commits into
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors block handler tests to use a typed generic test helper ( ChangesBlock Handler Test Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
![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 (1)
internal/restapi/block_handler_test.go (1)
212-215: ⚡ Quick winBenchmark unnecessarily measures server creation overhead on each iteration.
callAPIHandlercreates a newhttptest.NewServerfor every benchmark iteration. Extract server setup beforeb.ResetTimer()and reuse it in the loop to measure handler performance rather than server spin-up time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/block_handler_test.go` around lines 212 - 215, The benchmark is measuring server creation each iteration because callAPIHandler spins up an httptest.NewServer inside the loop; refactor the benchmark to create the httptest.Server (and the handler/api/endpoint) once before b.ResetTimer() and then call the handler directly (or call a variant of callAPIHandler that accepts the server URL or *httptest.Server) inside the loop so the loop measures only handler execution; update references to callAPIHandler[BlockEntryResponse] to use the reused server or a new helper that accepts the server to avoid per-iteration NewServer creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/restapi/block_handler_test.go`:
- Around line 55-57: The test in block_handler_test.go is making an
order-dependent assertion on refs.Agencies[0].ID == "25", which can be flaky;
change the test to search refs.Agencies for an entry with ID "25" (e.g., iterate
or use a helper find function), assert that such an agency is found, then assert
the matched agency's Name is not empty. Also ensure production code that builds
refs.Agencies (see functions that populate refs.Agencies in internal/restapi
package) uses a map to deduplicate IDs before converting to a slice so ordering
is not relied upon.
---
Nitpick comments:
In `@internal/restapi/block_handler_test.go`:
- Around line 212-215: The benchmark is measuring server creation each iteration
because callAPIHandler spins up an httptest.NewServer inside the loop; refactor
the benchmark to create the httptest.Server (and the handler/api/endpoint) once
before b.ResetTimer() and then call the handler directly (or call a variant of
callAPIHandler that accepts the server URL or *httptest.Server) inside the loop
so the loop measures only handler execution; update references to
callAPIHandler[BlockEntryResponse] to use the reused server or a new helper that
accepts the server to avoid per-iteration NewServer creation.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 277ad54e-80fc-4c91-8329-8f1b02475228
📒 Files selected for processing (2)
internal/restapi/block_handler_test.gointernal/restapi/response_types.go
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/restapi/block_handler_test.go (1)
156-170: ⚡ Quick winBroaden reference-consistency validation to all stop IDs.
Right now this test validates only one
StopID(Line 164), so reference mismatches in other block stop times can slip through. Consider asserting everyStopIDacross configurations/trips exists inmodel.Data.References.Stops.Proposed refactor
- entry := model.Data.Entry - if len(entry.Configurations) == 0 || len(entry.Configurations[0].Trips) == 0 { + entry := model.Data.Entry + if len(entry.Configurations) == 0 { t.Skip("no trips in block to verify") } - blockStopTimes := entry.Configurations[0].Trips[0].BlockStopTimes - if len(blockStopTimes) == 0 { - t.Skip("no block stop times to verify") - } - stopID := blockStopTimes[0].StopTime.StopID refStopIDs := make(map[string]bool, len(model.Data.References.Stops)) for _, s := range model.Data.References.Stops { refStopIDs[s.ID] = true } - assert.True(t, refStopIDs[stopID], "Stop %s should be in references", stopID) + + foundAnyStopTimes := false + for _, cfg := range entry.Configurations { + for _, trip := range cfg.Trips { + for _, st := range trip.BlockStopTimes { + foundAnyStopTimes = true + assert.True(t, refStopIDs[st.StopTime.StopID], "Stop %s should be in references", st.StopTime.StopID) + } + } + } + if !foundAnyStopTimes { + t.Skip("no block stop times to verify") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/block_handler_test.go` around lines 156 - 170, The test only asserts a single stop ID; instead iterate all configurations -> trips -> BlockStopTimes in the entry (use entry.Configurations, .Trips, .BlockStopTimes) and for each StopTime.StopID verify it exists in the reference map (refStopIDs) built from model.Data.References.Stops; keep the existing early t.Skip checks for empty configurations/trips/blockStopTimes, and replace the single assert on stopID with an assertion inside the loop that each stopID is present (include the stopID in the assertion message for easier debugging).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/restapi/block_handler_test.go`:
- Around line 156-170: The test only asserts a single stop ID; instead iterate
all configurations -> trips -> BlockStopTimes in the entry (use
entry.Configurations, .Trips, .BlockStopTimes) and for each StopTime.StopID
verify it exists in the reference map (refStopIDs) built from
model.Data.References.Stops; keep the existing early t.Skip checks for empty
configurations/trips/blockStopTimes, and replace the single assert on stopID
with an assertion inside the loop that each stopID is present (include the
stopID in the assertion message for easier debugging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eb368c4f-2106-4fde-b188-b9229da0dd4c
📒 Files selected for processing (1)
internal/restapi/block_handler_test.go
- Add BlockEntryResponse alias for EntryResponse[models.BlockEntry] - Replace serveApiAndRetrieveEndpoint + map[string]any casts with callAPIHandler[BlockEntryResponse] - Drop redundant tests: DifferentBlockIDs, AgencyIdExtraction, JSONSerialization, ResponseTime - Merge ResponseValidation into EndToEnd (detailed [0] checks + iterate-all coverage) - Fix duplicate-iteration bug in VerifyBlockStopTimes when len==1 - Trim non-specific assertions (Version/CurrentTime) from error-path tests
Ahmedhossamdev
force-pushed
the
refactor/typed-block-tests
branch
from
3b0d6b2 to
435175d
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
1 participant
block_handler_test.goto the typed response pattern usingcallAPIHandler[BlockEntryResponse]TestBlockHandlerEndToEndTestBlockHandlerVerifyBlockStopTimesSummary by CodeRabbit