Skip to content

refactor: introduce TripsForRouteResponse type and update related tests#914

Open
Ahmedhossamdev wants to merge 3 commits into
mainfrom
refactor/typed-trips-for-route-tests
Open

refactor: introduce TripsForRouteResponse type and update related tests#914
Ahmedhossamdev wants to merge 3 commits into
mainfrom
refactor/typed-trips-for-route-tests

Conversation

@Ahmedhossamdev
Copy link
Copy Markdown
Member

@Ahmedhossamdev Ahmedhossamdev commented May 5, 2026

  • Added TripsForRouteResponse with type TripsForRouteListEntry.
  • Removed verifyTripEntry and verifyRef rhey only checked for the Json keys existed, which is now guaranteed by the typed structs.
  • Update the old tests with the new response type.

Summary by CodeRabbit

  • Tests

    • Improved test coverage for the trips-for-route API with stronger validation of response metadata, limits, per-entry fields, schedule inclusion/exclusion, and malformed-ID handling.
  • Refactor

    • Introduced a typed API response for the trips-for-route endpoint to increase type-safety and make returned data more predictable.

Review Change Stack

@Ahmedhossamdev Ahmedhossamdev requested a review from fletcherw May 5, 2026 09:08
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b9f38e59-6413-4517-b134-f68d15a48ead

📥 Commits

Reviewing files that changed from the base of the PR and between baf9dae and aed7914.

📒 Files selected for processing (2)
  • internal/restapi/response_types.go
  • internal/restapi/trips_for_route_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/restapi/response_types.go

📝 Walkthrough

Walkthrough

Added a new exported generic response type alias TripsForRouteResponse and refactored trips-for-route handler tests to use the typed callAPIHandler[TripsForRouteResponse] with structured assertions instead of untyped map decoding.

Changes

Typed Response Introduction & Test Refactoring

Layer / File(s) Summary
Data Shape
internal/restapi/response_types.go
Added type TripsForRouteResponse ListResponse[models.TripsForRouteListEntry].
Fixture and test helper
internal/restapi/trips_for_route_handler_test.go
Adds an in-memory GTFS fixture and createTestApiWithTripsForRouteFixture wired to a test clock to support typed handler assertions.
Test Refactoring
internal/restapi/trips_for_route_handler_test.go
Replaced untyped response parsing with callAPIHandler[TripsForRouteResponse] in TestTripsForRouteHandler_DifferentRoutes, TestTripsForRouteHandler_ScheduleInclusion, and TestTripsForRouteHandlerWithMalformedID. Tests now assert response metadata and typed entry fields (trip id, service date, situation ids, schedule presence/contents, and optional status fields).
Formatting adjustments
internal/restapi/trips_for_route_handler_test.go
Minor whitespace/blank-line changes around collectStopIDsFromSchedule test invocations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • OneBusAway/maglev#897: Introduces similar typed response aliases and refactors handler tests to use typed callers.
  • OneBusAway/maglev#913: Adds a typed response alias and migrates a handler test to callAPIHandler[...] with typed decoding.
  • OneBusAway/maglev#912: Related migration pattern converting handler tests to the typed API caller and adding response aliases.

Suggested reviewers

  • fletcherw
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main changes: introducing a new TripsForRouteResponse type and updating related tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/typed-trips-for-route-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Performance Smoke Test Results

Status: PASSED

Metric Value
p(95) latency 1.8 ms
Error rate 0.00%
Total requests 329
Req/sec 10.8

Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%.

Full results uploaded as workflow artifact: k6-smoke-summary.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/trips_for_route_handler_test.go`:
- Around line 55-58: The test currently returns after asserting resp.StatusCode
for non-OK cases without checking the response body code; update the non-OK
branch to also assert that the parsed response model's Code equals
tt.expectStatus (i.e., add assert.Equal(t, tt.expectStatus, model.Code) before
the return) so the transport status and body status cannot diverge — locate this
in the test where resp, tt.expectStatus and model (model.Code) are available and
add the assertion immediately prior to returning.
🪄 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: baa16094-57d2-4b4a-9a03-c322042b4655

📥 Commits

Reviewing files that changed from the base of the PR and between a7e624c and 13a1179.

📒 Files selected for processing (2)
  • internal/restapi/response_types.go
  • internal/restapi/trips_for_route_handler_test.go

Comment thread internal/restapi/trips_for_route_handler_test.go
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Performance Smoke Test Results

Status: PASSED

Metric Value
p(95) latency 1.8 ms
Error rate 0.00%
Total requests 335
Req/sec 11.0

Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%.

Full results uploaded as workflow artifact: k6-smoke-summary.

Comment thread internal/restapi/trips_for_route_handler_test.go
Comment thread internal/restapi/trips_for_route_handler_test.go
@Ahmedhossamdev Ahmedhossamdev force-pushed the refactor/typed-trips-for-route-tests branch from baf9dae to aed7914 Compare May 13, 2026 17:30
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

Performance Smoke Test Results

Status: PASSED

Metric Value
p(95) latency 2.2 ms
Error rate 0.00%
Total requests 336
Req/sec 11.1

Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%.

Full results uploaded as workflow artifact: k6-smoke-summary.

@Ahmedhossamdev Ahmedhossamdev requested a review from fletcherw May 16, 2026 23:35
Copy link
Copy Markdown
Collaborator

@burma-shave burma-shave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider removing dead types in the models package

Now that restapi.TripsForRouteResponse (the new alias) is in place, two types in internal/models/trips_for_route.go appear to be unused and can be cleaned up:

Type Can remove? Reason
models.TripsForRouteResponse ✅ Yes Shadowed by the new restapi.TripsForRouteResponse alias; no non-test callers remain
models.TripsForRouteData ✅ Yes Only referenced by the dead struct above
models.TripsForRouteListEntry ❌ No Still the element type for ListResponse[models.TripsForRouteListEntry] throughout the handler and tests

There's also a subtle naming collision: models.TripsForRouteResponse (old struct) and restapi.TripsForRouteResponse (new alias) coexist in different packages, which compiles fine but can cause confusion. Removing the models one would resolve that ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants