fix: trips-for-route logging, test coverage, and performance follow-ups

[ARCoder181105]

Description

This PR addresses follow-up items from #679 to improve logging, increase test coverage, optimize queries, and align with the OpenAPI spec for the trips-for-route endpoint.

Changes

Logging

• Added warning logs for non-ErrNoRows errors when resolving base trip references.
• Added debug and warning logs for DUPLICATED trip fallback resolution.
• Logged failures in GetRouteIDsForStops batch queries.
• Logged errors when fetching shape points in buildScheduleForTrip.
• Updated stale comment to correctly reflect default occupancy values (-1).

Test Coverage

• Added test for DUPLICATED trips resolution.
• Added test for past-midnight service time handling.
• Covered edge case where a trip spans the current time in TestSelectBestTripInBlock.
• Made integration test deterministic with a fixed timestamp and tightened expected bounds.

Performance

• Removed redundant GetTripsInBlock query.
• Reused GetTripsInBlockWithTimeBounds for both validation and selection.

Spec Compliance

• Removed omitempty from LastKnownLocation to match OpenAPI expectations.

Testing

• All unit and integration tests pass (make test).

fixes #748

Made a extra changes in internal/restapi/arrivals_and_departures_for_stop_handler_test.go this just to fix the CI failure, made a separate PR #754

[aaronbrethorst]

Hey Aditya, solid follow-up work here — the redundant query removal is a nice optimization, the clock-based time handling is a real improvement for test determinism, and the omitempty removal on LastKnownLocation is correct per the OpenAPI spec (nullable: true means always present). A few things to address before merging.

Critical Issues (0 found)

(none)

Important Issues (2 found)

1. Extra GetTrip call on stripped ID is a wasted query

internal/restapi/trips_for_route_handler.go:375-381

After stripping the suffix and setting baseTripID = stripped (line 373), the code makes a GetTrip call purely for logging:

baseTripID = stripped

if _, err2 := api.GtfsManager.GtfsDB.Queries.GetTrip(ctx, baseTripID); err2 != nil {
    // ... log warnings ...
}

The result is discarded — baseTripID stays as stripped regardless. The downstream code (buildScheduleForTrip, BuildTripStatus) will discover and handle a missing trip on its own. This is a redundant DB query on every DUPLICATED vehicle with a suffix.

Remove this GetTrip call. The existing downstream error handling is sufficient, and the debug log on line 371 already records the fallback.

2. Commented-out code should be removed

internal/restapi/trips_for_route_handler_test.go:400-401

// Temporarily override the API clock
// api.Clock = mockClock{Time: pastMidnightTime}

Dead commented-out code shouldn't be committed. Since the test uses the time query parameter instead, just remove these two lines.

Suggestions (1 found)

1. time.Sleep(500ms) is a flaky test pattern

internal/restapi/trips_for_route_handler_test.go:355

time.Sleep(500 * time.Millisecond)

This waits for the real-time poller to complete its first fetch, but the duration is arbitrary — it could be too short on slow CI or too long on fast machines. I know other tests in the codebase already use this pattern, so it's not unique to this PR, but worth noting. Non-blocking for now.

Strengths

• The redundant GetTripsInBlock query removal is correct — GetActiveTripInBlockAtTime + fallback to GetTripsInBlockWithTimeBounds covers all cases, saving a query per block per service day
• The clock-based time handling (api.Clock.Now() when no time param) is a meaningful improvement — makes tests deterministic without mocking
• Tests are now pinned to a specific time and route (25_151 at noon Pacific on a Tuesday) that reliably has service, eliminating the flaky minExpected: 0 range
• The selectBestTripInBlock test for the "trip spans now" case is a good edge case to cover
• SetRealTimeVehiclesForTest follows the existing per-feed pattern correctly, using "_test" feed ID
• Comment corrections (occupancy defaults to -1, not 0) are accurate

Recommended Action

1. Remove the redundant GetTrip call at line 375-381
2. Remove the commented-out code at line 400-401
3. Consider the time.Sleep suggestion for a future cleanup