test: add E2E coverage for DUPLICATED real-time trips

[3rabiii]

Description

This PR addresses end-to-end test coverage for the DUPLICATED real-time trips path in the trips-for-route handler. It ensures that the handler correctly resolves base trips for duplicated vehicles using the stripNumericSuffix fallback and successfully populates the schedule and reference data.

Changes Made

• End-to-End Assertion: Added TestTripsForRouteHandler_DuplicatedRealtimeTrip to verify that:
  • The base trip falls outside the handler's -30/+10 minute window, ensuring it is reached only through the DUPLICATED fallback block.
  • The duplicated trip appears in the response list with its preserved combined suffixed tripId.
  • The schedule is successfully populated from the resolved base trip (proving stripNumericSuffix works correctly).
  • ActiveTripID is correctly set from the vehicle's own suffixed ID by BuildVehicleStatus, not the base ID.
  • The resolved base trip correctly appears in references.trips with all associated static data.
• Cleanup: Removed the previous custom fixture zip builder and mock GTFS-RT injection in favor of a cleaner, more direct testing approach using the updated MockAddDuplicatedVehicle.

Closes: #1297

Summary by CodeRabbit

• Tests
  • Added integration coverage for duplicated real-time vehicle trips.
  • Verified that suffixed real-time trips appear correctly, resolve their associated static schedules and stop times, and include the original trip in references.
  • Confirmed duplicated trips report the appropriate in-progress status.
  • Improved synthetic transit fixtures and real-time feed setup for reliable scenario testing.

[coderabbitai]

Warning

Review limit reached

@3rabiii, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b9f083ee-c92d-484f-b1bc-70774999b52a

📥 Commits

Reviewing files that changed from the base of the PR and between 84131b7 and 7b7b0d4.

📒 Files selected for processing (2)

• internal/gtfs/gtfs_manager.go
• internal/restapi/trips_for_route_handler_test.go

📝 Walkthrough

Walkthrough

This change adds end-to-end coverage for DUPLICATED real-time trips in trips-for-route. It adds test vehicle injection, synthetic GTFS fixtures, a fixed test clock, and assertions for base-trip resolution, schedule data, status, and references.

Changes

DUPLICATED real-time trip coverage

Layer / File(s)	Summary
Synthetic fixture and realtime setup internal/gtfs/gtfs_manager.go, internal/restapi/trips_for_route_handler_test.go	The tests add a realtime vehicle injection helper, a fixed clock, aliased GTFS setup, and synthetic base and scheduled trips.
DUPLICATED real-time trip integration test internal/restapi/trips_for_route_handler_test.go	The test injects a DUPLICATED vehicle with a suffixed trip ID. It verifies trip inclusion, base-trip schedule and stop-time resolution, duplicated and in-progress status, and route, headsign, and block references.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• OneBusAway/maglev#1000: Adds related duplicated-trip coverage for tripsForRouteHandler and BuildTripStatus.
• OneBusAway/maglev#1224: Covers duplicated-trip schedule and service-date resolution in related handler tests.
• OneBusAway/maglev#1288: Adds related duplicated-trip reference coverage involving buildTripReferences.

Suggested reviewers: burma-shave, arcoder181105

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The test covers the duplicated trip and suffix fallback, but its references assertions do not prove duplicated-trip reference resolution.	Use a base trip that is inactive at query time so the test exercises the DUPLICATED reference-resolution branch.

✅ 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 describes the added end-to-end coverage for DUPLICATED real-time trips.
Out of Scope Changes check	✅ Passed	The changes remain within scope by adding the requested handler test and a test-only realtime fixture helper.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details

putComment timed out

[3rabiii]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

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 226-228: Update the assertions in the schedule stop-time loop of
the trip-for-route handler test to verify the exact expected combined stop IDs,
rather than merely checking for an underscore. Assert both expected agency/stop
ID combinations in schedule order.

🪄 Autofix

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: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b5b59c93-0de7-4ad6-97ef-f672c7f13a47

📥 Commits

Reviewing files that changed from the base of the PR and between 8bed443 and 5b62599.

📒 Files selected for processing (1)

• internal/restapi/trips_for_route_handler_test.go

[aaronbrethorst]

Code review

Found 1 issue:

1. The references.trips block asserts nothing about the DUPLICATED path — it passes regardless of that behavior. In this fixture tfr-trip is an active scheduled trip, so it is already in fetchedTrips/filteredRouteTrips from the static loop and is emitted into references.trips by buildTripReferences(..., preFetchedTrips, ...) with route, headsign, and block populated. The DUPLICATED branch that would add it (if !filteredRouteTrips[baseTripID] { ... } in trips_for_route_handler.go) is therefore dead in this test — delete that block and these three assertions still pass. The comment claiming "stripNumericSuffix resolved the base trip: references.trips must contain..." misattributes what is being verified (the suffix fallback is genuinely covered, but by the entry.Schedule stop-ID assertions above, not by this block). To actually exercise it, the duplicated vehicle needs a base trip that is not independently active at the query time.

maglev/internal/restapi/trips_for_route_handler_test.go

Lines 238 to 252 in 00c17c0

	// stripNumericSuffix resolved the base trip: references.trips must contain the
	// scheduled trip populated with route, headsign, and block data from static GTFS.
	var baseTripRef *models.Trip
	for i := range model.Data.References.Trips {
	if model.Data.References.Trips[i].ID == baseTripID {
	baseTripRef = &model.Data.References.Trips[i]
	break
	}
	}
	require.NotNil(t, baseTripRef, "references.trips should contain the resolved base trip")
	assert.Equal(t, combinedRouteID, baseTripRef.RouteID)
	assert.Equal(t, tripsForRouteHeadsign, baseTripRef.TripHeadsign)
	assert.Equal(t, utils.FormCombinedID(tripsForRouteAgencyID, "tfr-block"), baseTripRef.BlockID)
	}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[aaronbrethorst]

The schedule assertions in here are the good part — they genuinely cover the stripNumericSuffix fallback and confirm the suffixed trip keeps its own entry ID while carrying the base trip's stop times. The RT feed setup is correct too; InitGTFSManager does a synchronous first fetch, so there's no race to worry about.

One assertion doesn't do what its comment claims.

The references.trips block is tautological for the DUPLICATED path. In this fixture tfr-trip is an active scheduled trip, so it reaches references.trips through the ordinary static path — fetchedTrips → filteredRouteTrips → buildTripReferences(..., preFetchedTrips, ...) — fully populated. The handler's DUPLICATED-side resolution (if !filteredRouteTrips[baseTripID] { ... }, trips_for_route_handler.go:472-479) never executes, and deleting it entirely would leave this test green. The comment says the block verifies that stripNumericSuffix resolved the base trip, but that's covered by the schedule assertions instead.

Since base-trip reference resolution is one of the things #1297 specifically asked for, it's worth making real: make the base trip inactive at query time, or query at a moment when only the duplicated run is live, so the fallback branch is the only way the reference can appear.

The branch also conflicts with main, and more substantially than the conflict markers suggest: createTestApiWithTripsForRouteFixture no longer exists — it was replaced by a generic createTestApiWithGTFSFixture(t, clock, zipName, files map[string]string) plus per-scenario file maps. So your buildTripsForRouteFixtureZip extraction is largely redundant with what main now does more generally, and createTestApiWithTripsForRouteAndRealtime will want rebuilding on top of the new builder. Sorry — that landed while this was in the queue.

Two smaller notes for the rework: the URL only passes includeSchedule=true, so the DUPLICATED → ("DUPLICATED", "in_progress") status mapping is never exercised, which seems worth having in a PR with this title; and there's no assertion on list length, so a spurious extra entry would slip through.

Re-review whenever it's rebased.

[aaronbrethorst]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

[burma-shave]

merge in main and fix conflicts

[aaronbrethorst]

Approving on the merits — this looks good, but it can't merge as-is. See below.

All four points from my last review are addressed, and the important one is genuinely fixed. I checked it rather than assuming: at the 11:30 clock the base trip falls outside the handler's -30/+10 minute window, so it can only reach references.trips through the DUPLICATED fallback block — delete that block and the test fails. That's the difference from last time, when the assertion held no matter what the handler did.

The rest holds up too. dupEntry.Schedule would come back with empty stop IDs if stripNumericSuffix were removed, so the fallback is really exercised. ActiveTripID is set from the vehicle's own suffixed ID by BuildVehicleStatus, not the base ID, so that assertion has teeth. The DUPLICATED/in_progress pair matches GetVehicleStatusAndPhase, and require.Len(..., 2) guards against extra entries.

Two things stand between this and merging, neither of them the code:

1. The branch conflicts with main — it's one commit ahead and thirty-seven behind, and both files it touches have changed since. It needs another rebase. Sorry for the treadmill; that file is busy right now.
2. burma-shave has a change request still open on this PR. My approval doesn't clear that one, so please square it with them before merging.

While you're rebasing: the PR description still describes buildTripsForRouteFixtureZip and a mock GTFS-RT feed, neither of which exists in the current diff. Worth a refresh so the next reader isn't misled.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[burma-shave]

needs merge conflicts resolved again

[burma-shave]

@3rabiii checking in on this older PR. It is still blocked on merge conflicts from the last review. Could you please merge main, resolve the conflicts, and let us know when it is ready for re-review?