test: add E2E coverage for status sub-object fields - #1300
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test suite adds a scheduled vehicle-position fixture for ChangesTrips-for-route status coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR adds focused end-to-end assertions for status fields without changing production behavior, so no actionable merge-blocking risk remains beyond normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Code reviewFound 1 issue:
maglev/internal/restapi/trips_for_route_handler_test.go Lines 228 to 234 in 9805ac4 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Most of this test is load-bearing. Injecting a real GTFS-RT vehicle with a GPS fix and asserting on status.position.lat/lon, the -1 occupancyCount sentinel, and non-null-but-empty situationIds/vehicleFeatures would all catch real regressions — and the [] vs null checks in particular guard something easy to break and hard to notice. I confirmed none of it contradicts openapi.yml's TripStatus schema.
The one field the PR headlines first, though, asserts nothing.
assert.GreaterOrEqual(t, entry.Status.BlockTripSequence, 0) can never fail. calculateBlockTripSequence returns 0 on any failure, and BuildTripStatus only assigns the field under if blockTripSequence > 0 (trips_helper.go:62-65) — so it's non-negative by construction, whatever happens. The fixture is fully deterministic here (a single trip in tfr-block, and GetBlockTripSequence is zero-based), so the real assertion is assert.Equal(t, 0, ...). One-line change.
I know GreaterOrEqual(seq, 0) appears elsewhere in the suite, but those run over RABA data where the value genuinely isn't pinned. Over a controlled fixture — and in a PR whose stated goal is strict assertions — it's worth being exact.
The branch also conflicts with main, and more deeply than it looks: createTestApiWithTripsForRouteFixture, the helper this refactors, was removed in c905b6c8 and replaced by a generic createTestApiWithGTFSFixture(t, clock, zipName, files) plus per-scenario file maps. So the buildTripsForRouteFixtureZip extraction is now redundant, and createTestApiWithScheduledRealtimePosition needs rebuilding on top of the new builder. That landed while this sat in the queue — not your doing. TestTripsForRouteHandler_StatusFields itself should transplant cleanly once the fixture dependency is sorted.
One last thing: the helper doc and test comment both emphasize "a SCHEDULED vehicle", but the feed never sets ScheduleRelationship (it relies on the proto default) and the test never asserts Status/Phase. Either assert it or soften the comment.
Happy to re-review once it's rebased.
- Change blockTripSequence check to expect exactly 0 instead of >= 0 - Check that the vehicle status is SCHEDULED and the phase is in_progress - Use the new createTestApiWithGTFSFixture setup after rebasing from main
9805ac4 to
f727460
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Approving on the merits — this looks good, but it can't merge as-is. See below.
All three points from my last review are addressed: GreaterOrEqual(seq, 0) is now a real equality assertion, the redundant fixture-zip extraction is gone in favor of createTestApiWithGTFSFixture, and the "SCHEDULED vehicle" claim is asserted rather than narrated.
I checked the parts that could have made this test quietly meaningless and they're sound. InitGTFSManager does a synchronous first RT fetch before MarkReady(), so there's no race. The fixture has no shapes.txt, which means BuildTripStatus skips shape projection and status.Position stays the raw GPS fix — so the float64(float32(...)) expectation is correct rather than accidentally correct. And the situationIds/vehicleFeatures assertions are load-bearing, since decoding null into []string yields nil and require.NotNil distinguishes [] from null.
Two things blocking the merge, neither of them the code:
- The branch conflicts with
main— that file gained ~198 lines after your 08-11 rebase, touching the import block and the fixture region you also edit. Mechanical to resolve, but it needs doing. - burma-shave has a change request still open here. My approval doesn't dismiss it, so please square that before merging.
Two notes for the rebase:
createTestApiWithScheduledRealtimePositiontakes aclock.Clockparameter but hardcodestripsForRouteTestClockfor the feed header and vehicle timestamps. A future caller passing a different clock more than 15 minutes away would silently tripStaleDetectorand getdefault/scheduledinstead of a clear failure. Deriving the timestamp fromc.Now()removes the trap.- The
occupancyCountassertion's message says "when the feed omits occupancy data," butNewTripStatus()sets -1 unconditionally and nothing on this path overwrites it — the assertion pins a constructor default, not feed-driven behavior.
One coordination note: #1298 adds a second real-time fixture builder to this same file using a different injection mechanism (SetRealTimeVehiclesForTest rather than an httptest feed). If both land we end up with two overlapping patterns — worth settling which one wins before the second merges.
- Resolve merge conflicts in trips_for_route_handler_test.go. - Use dynamic timestamp (c.Now()) for injected vehicles to avoid StaleDetector traps. - Fix occupancyCount assertion message to reflect the constructor default. - Unify real-time mock pattern using MockAddVehicleWithOptions instead of httptest.
|
burma-shave
left a comment
burma-shave
left a comment
There was a problem hiding this comment.
conflicts need to be resolved again
|
@3rabiii checking in on this older PR. It is still blocked on merge conflicts from the last review. Could you please merge |
3 participants
Description
This PR addresses Issue #1299 by adding strict end-to-end test coverage for the
statussub-object fields in thetrips-for-routeAPI response.Changes Made
TestTripsForRouteHandler_StatusFieldsto verify:blockTripSequenceis a non-negative integer.position.latandposition.lonare accurately populated (handling the GTFS-RTfloat32to Gofloat64conversion).occupancyCountcorrectly returns the-1sentinel constructor default value.situationIdsandvehicleFeaturesare strictly non-nil empty slices ([]), differentiating them fromnull.SCHEDULEDvehicle tracking without race conditions.Summary by CodeRabbit
Summary by CodeRabbit