From 4a684125df523820870a144b2fa8b55ef99df588 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Mon, 25 May 2026 14:48:33 -0700 Subject: [PATCH 1/2] fix: guard nil OBA SDK responses to prevent segfault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The OBA REST server can return a literal JSON `null` body with HTTP 200. The go-sdk decodes that into a (nil, nil) response — a nil pointer with a nil error — so checks that dereferenced `resp.Data...` after only an `err != nil` check panicked with SIGSEGV (reported on `oba-validator -json example_configs/kcm.json`). Add a nil-response guard to every in-check SDK call (vehicle, endpoint, alert, trip-update checks). Severity follows the evidence-based model, matching each call's existing empty/error sibling: - per-stop/per-vehicle cross-refs Warn (unconfirmed) - vehicles-for-agency Fails (feed proves vehicles exist, API returned none) - the basic-endpoints smoke test Fails (null core endpoint = server fault) Also add a `withReason` helper so a null-body Fail no longer renders a dangling colon ("current-time failed: "). --- validator/check_agencies.go | 2 +- validator/check_alerts.go | 6 ++ validator/check_alerts_test.go | 28 +++++++ validator/check_endpoints.go | 26 +++---- validator/check_endpoints_test.go | 71 ++++++++++++++++++ validator/check_tripupdates.go | 6 ++ validator/check_tripupdates_test.go | 28 +++++++ validator/check_vehicles.go | 13 ++++ validator/check_vehicles_test.go | 109 ++++++++++++++++++++++++++++ validator/util.go | 11 +++ 10 files changed, 286 insertions(+), 14 deletions(-) diff --git a/validator/check_agencies.go b/validator/check_agencies.go index cebd2ca..3b9944f 100644 --- a/validator/check_agencies.go +++ b/validator/check_agencies.go @@ -12,7 +12,7 @@ func (agencyUnionCheck) Name() string { return "agency-union" } func (agencyUnionCheck) Run(ctx context.Context, vc *ValidationContext) []Result { const name = "agency-union" if vc.AgenciesErr != nil || vc.Agencies == nil { - return []Result{{Check: name, Status: Fail, Message: "agencies-with-coverage unavailable: " + redact(vc.AgenciesErr, vc.Config.APIKey)}} + return []Result{{Check: name, Status: Fail, Message: withReason("agencies-with-coverage unavailable", vc.AgenciesErr, vc.Config.APIKey)}} } apiSet := map[string]bool{} diff --git a/validator/check_alerts.go b/validator/check_alerts.go index 50c7fb9..beee225 100644 --- a/validator/check_alerts.go +++ b/validator/check_alerts.go @@ -54,6 +54,12 @@ func (serviceAlertCheck) Run(ctx context.Context, vc *ValidationContext, src *So Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, key))}) continue } + if ad == nil { + out = append(out, Result{Check: name, Source: src.Label, Status: Warn, + Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop), + Details: map[string]any{"stopId": obaStop}}) + continue + } anySituation := false matched := false for _, adp := range ad.Data.Entry.ArrivalsAndDepartures { diff --git a/validator/check_alerts_test.go b/validator/check_alerts_test.go index bb0010b..6f74d2f 100644 --- a/validator/check_alerts_test.go +++ b/validator/check_alerts_test.go @@ -37,6 +37,34 @@ func TestServiceAlertFoundInSituationIDs(t *testing.T) { } } +// A `null` arrivals response (nil SDK response, nil error) must not be mistaken +// for "stop has no situations" and Fail — it is an unconfirmed query, so Warn. +func TestServiceAlertNullArrivalsResponseWarns(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`null`)) + return + } + t.Errorf("unexpected path %s", r.URL.Path) + }) + src := &SourceContext{ + Label: "ds0", + Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, + PrepErrors: map[string]error{}, + Static: staticForVehicle(), + ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ + ID: "ALERT1", + InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, + }}}, + } + vc := &ValidationContext{Config: cfgForTest("test"), Client: client} + results := serviceAlertCheck{}.Run(context.Background(), vc, src) + if len(results) == 0 || results[0].Status != Warn { + t.Errorf("null arrivals response: want Warn, got %+v", results) + } +} + func TestServiceAlertNoSamplableWarns(t *testing.T) { src := &SourceContext{ Label: "ds0", diff --git a/validator/check_endpoints.go b/validator/check_endpoints.go index 1541804..f322165 100644 --- a/validator/check_endpoints.go +++ b/validator/check_endpoints.go @@ -31,8 +31,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 1. current-time ct, err := vc.Client.CurrentTime.Get(ctx) - if err != nil { - add("current-time", Fail, "current-time failed: "+redact(err, key), nil) + if err != nil || ct == nil { + add("current-time", Fail, withReason("current-time failed", err, key), nil) skipRest("current-time failed") return out } @@ -48,7 +48,7 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 2. agencies-with-coverage (pre-fetched into the context) if vc.Agencies == nil || vc.AgenciesErr != nil { - add("agencies-with-coverage", Fail, "agencies-with-coverage failed: "+redact(vc.AgenciesErr, key), nil) + add("agencies-with-coverage", Fail, withReason("agencies-with-coverage failed", vc.AgenciesErr, key), nil) pop() skipRest("agencies-with-coverage failed") return out @@ -65,8 +65,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 3. routes-for-agency routes, err := vc.Client.RoutesForAgency.List(ctx, agencyID) - if err != nil || len(routes.Data.List) == 0 { - add("routes-for-agency", Fail, "routes-for-agency empty/failed: "+redact(err, key), map[string]any{"agencyId": agencyID}) + if err != nil || routes == nil || len(routes.Data.List) == 0 { + add("routes-for-agency", Fail, withReason("routes-for-agency empty/failed", err, key), map[string]any{"agencyId": agencyID}) pop() skipRest("routes-for-agency failed") return out @@ -77,8 +77,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 4. stops-for-route sfr, err := vc.Client.StopsForRoute.List(ctx, routeID, onebusaway.StopsForRouteListParams{}) - if err != nil || len(sfr.Data.Entry.StopIDs) == 0 { - add("stops-for-route", Fail, "stops-for-route empty/failed: "+redact(err, key), map[string]any{"routeId": routeID}) + if err != nil || sfr == nil || len(sfr.Data.Entry.StopIDs) == 0 { + add("stops-for-route", Fail, withReason("stops-for-route empty/failed", err, key), map[string]any{"routeId": routeID}) pop() skipRest("stops-for-route failed") return out @@ -89,8 +89,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 5. stop st, err := vc.Client.Stop.Get(ctx, stopID) - if err != nil || st.Data.Entry.ID != stopID { - add("stop", Fail, "stop lookup failed/mismatch: "+redact(err, key), map[string]any{"stopId": stopID}) + if err != nil || st == nil || st.Data.Entry.ID != stopID { + add("stop", Fail, withReason("stop lookup failed/mismatch", err, key), map[string]any{"stopId": stopID}) pop() skipRest("stop failed") return out @@ -104,8 +104,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { Lat: onebusaway.Float(lat), Lon: onebusaway.Float(lon), }) - if err != nil || loc.Data.OutOfRange || len(loc.Data.List) == 0 { - add("stops-for-location", Fail, "stops-for-location empty/out-of-range/failed: "+redact(err, key), nil) + if err != nil || loc == nil || loc.Data.OutOfRange || len(loc.Data.List) == 0 { + add("stops-for-location", Fail, withReason("stops-for-location empty/out-of-range/failed", err, key), nil) pop() skipRest("stops-for-location failed") return out @@ -115,8 +115,8 @@ func (endpointsCheck) Run(ctx context.Context, vc *ValidationContext) []Result { // 7. arrivals-and-departures-for-stop ad, err := vc.Client.ArrivalAndDeparture.List(ctx, stopID, onebusaway.ArrivalAndDepartureListParams{}) - if err != nil { - add("arrivals-and-departures-for-stop", Fail, "arrivals failed: "+redact(err, key), map[string]any{"stopId": stopID}) + if err != nil || ad == nil { + add("arrivals-and-departures-for-stop", Fail, withReason("arrivals failed", err, key), map[string]any{"stopId": stopID}) return out } n := len(ad.Data.Entry.ArrivalsAndDepartures) diff --git a/validator/check_endpoints_test.go b/validator/check_endpoints_test.go index 2a5e95b..60dc2d4 100644 --- a/validator/check_endpoints_test.go +++ b/validator/check_endpoints_test.go @@ -66,6 +66,77 @@ func TestEndpointsCheckHappyPath(t *testing.T) { } } +// validEndpointBody returns the happy-path JSON for whichever endpoint the path +// names, so a test can null out one step while the rest of the chain succeeds. +func validEndpointBody(p string) string { + switch { + case strings.Contains(p, "current-time"): + return `{"data":{"entry":{"time":1716000000000,"readableTime":"now"}}}` + case strings.Contains(p, "agencies-with-coverage"): + return `{"data":{"list":[{"agencyId":"1"}],"references":{"agencies":[{"id":"1","name":"Metro"}]}}}` + case strings.Contains(p, "routes-for-agency"): + return `{"data":{"list":[{"id":"1_R1","agencyId":"1"}]}}` + case strings.Contains(p, "stops-for-route"): + return `{"data":{"entry":{"routeId":"1_R1","stopIds":["1_S1"]}}}` + case strings.Contains(p, "stops-for-location"): + return `{"data":{"outOfRange":false,"list":[{"id":"1_S1"}]}}` + case strings.Contains(p, "arrivals-and-departures-for-stop"): + return `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_S1","tripId":"1_T1"}]}}}` + case strings.Contains(p, "/stop/"): + return `{"data":{"entry":{"id":"1_S1","lat":47.6,"lon":-122.3,"name":"Stop"}}}` + } + return "" +} + +// OBA can return a literal `null` body (HTTP 200) for a working-looking +// endpoint; the SDK decodes that into a nil response with a nil error. A core +// endpoint returning null is a server fault, so this smoke check must Fail +// rather than panic — for every SDK call in the chain. Each call dereferences +// the response differently (list length, scalar field, skew math), so all are +// covered, not just a representative one. +func TestEndpointsNullResponseFailsPerStep(t *testing.T) { + steps := []struct{ step, pathSubstr string }{ + {"current-time", "current-time"}, + {"routes-for-agency", "routes-for-agency"}, + {"stops-for-route", "stops-for-route"}, + {"stop", "/stop/"}, + {"stops-for-location", "stops-for-location"}, + {"arrivals-and-departures-for-stop", "arrivals-and-departures-for-stop"}, + } + for _, tc := range steps { + t.Run(tc.step, func(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.Contains(r.URL.Path, tc.pathSubstr) { + w.Write([]byte(`null`)) + return + } + w.Write([]byte(validEndpointBody(r.URL.Path))) + }) + ag, err := client.AgenciesWithCoverage.List(context.Background()) + if err != nil { + t.Fatal(err) + } + vc := &ValidationContext{Config: cfgForTest("test"), Client: client, Agencies: ag} + results := endpointsCheck{}.Run(context.Background(), vc) + var got Result + for _, r := range results { + if r.Check == "basic-endpoints/"+tc.step { + got = r + } + } + if got.Status != Fail { + t.Errorf("null %s response: want Fail got %v (%q)", tc.step, got.Status, got.Message) + } + // withReason must not leave a dangling colon when the cause is a null + // body (nil error) rather than a transport error. + if strings.HasSuffix(strings.TrimRight(got.Message, " "), ":") { + t.Errorf("null %s message has a dangling colon: %q", tc.step, got.Message) + } + }) + } +} + func TestEndpointsCheckCurrentTimeFailureSkipsRest(t *testing.T) { client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) diff --git a/validator/check_tripupdates.go b/validator/check_tripupdates.go index 59d452e..5e0123c 100644 --- a/validator/check_tripupdates.go +++ b/validator/check_tripupdates.go @@ -64,6 +64,12 @@ func (tripUpdateSamplingCheck) Run(ctx context.Context, vc *ValidationContext, s Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, key))}) continue } + if ad == nil { + out = append(out, Result{Check: name, Source: src.Label, Status: Warn, + Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop), + Details: map[string]any{"stopId": obaStop}}) + continue + } found := false for _, adp := range ad.Data.Entry.ArrivalsAndDepartures { if IDMatch(adp.TripID, tr.ID.ID, agency) { diff --git a/validator/check_tripupdates_test.go b/validator/check_tripupdates_test.go index 1ae0b87..2ac018a 100644 --- a/validator/check_tripupdates_test.go +++ b/validator/check_tripupdates_test.go @@ -103,3 +103,31 @@ func TestTripUpdateSampling404StopWarns(t *testing.T) { t.Errorf("404 on stop should Warn (not Fail), got %+v", results) } } + +// A `null` arrivals response (nil SDK response, nil error) must not be mistaken +// for "predicted trip absent" and Fail — it is an unconfirmed query, so Warn. +func TestTripUpdateNullArrivalsResponseWarns(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`null`)) + return + } + t.Errorf("unexpected path %s", r.URL.Path) + }) + src := &SourceContext{ + Label: "ds0", + Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, + PrepErrors: map[string]error{}, + Static: staticForVehicle(), + TripUpdates: >fs.Realtime{Trips: []gtfs.Trip{{ + ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, + StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, + }}}, + } + vc := &ValidationContext{Config: cfgForTest("test"), Client: client} + results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, src) + if len(results) == 0 || results[0].Status != Warn { + t.Errorf("null arrivals response: want Warn, got %+v", results) + } +} diff --git a/validator/check_vehicles.go b/validator/check_vehicles.go index 14f28d9..0f92be3 100644 --- a/validator/check_vehicles.go +++ b/validator/check_vehicles.go @@ -59,6 +59,11 @@ func (vehicleSamplingCheck) Run(ctx context.Context, vc *ValidationContext, src case err != nil: out = append(out, Result{Check: name + "/vehicles-for-agency", Source: src.Label, Status: Fail, Message: "vehicles-for-agency failed: " + redact(err, key), Details: map[string]any{"agencyId": agency}}) + case vfa == nil: + // Same evidence as the empty-list branch below — the feed proves the + // agency has vehicles, the API returned none — so Fail, not Warn. + out = append(out, Result{Check: name + "/vehicles-for-agency", Source: src.Label, Status: Fail, + Message: fmt.Sprintf("vehicles-for-agency %q returned a null response while feed has vehicles", agency), Details: map[string]any{"agencyId": agency}}) case len(vfa.Data.List) == 0: out = append(out, Result{Check: name + "/vehicles-for-agency", Source: src.Label, Status: Fail, Message: fmt.Sprintf("vehicles-for-agency %q empty while feed has vehicles", agency)}) @@ -88,6 +93,9 @@ func (vehicleSamplingCheck) Run(ctx context.Context, vc *ValidationContext, src case err != nil: out = append(out, Result{Check: name + "/trip-for-vehicle", Source: src.Label, Status: Warn, Message: "trip-for-vehicle returned no current trip: " + redact(err, key), Details: map[string]any{"vehicleId": obaVeh}}) + case tfv == nil: + out = append(out, Result{Check: name + "/trip-for-vehicle", Source: src.Label, Status: Warn, + Message: fmt.Sprintf("trip-for-vehicle returned a null response for vehicle %q", rawVeh), Details: map[string]any{"vehicleId": obaVeh}}) case IDMatch(tfv.Data.Entry.TripID, rawTrip, agency): out = append(out, Result{Check: name + "/trip-for-vehicle", Source: src.Label, Status: Pass, Message: fmt.Sprintf("vehicle %q on expected trip %q", rawVeh, rawTrip)}) @@ -118,6 +126,11 @@ func (vehicleSamplingCheck) Run(ctx context.Context, vc *ValidationContext, src Message: "trips-for-location failed: " + redact(err, key)}) continue } + if tfl == nil { + out = append(out, Result{Check: name + "/trips-for-location", Source: src.Label, Status: Warn, + Message: fmt.Sprintf("trips-for-location returned a null response near vehicle %q", rawVeh)}) + continue + } found := false for _, item := range tfl.Data.List { if IDMatch(item.TripID, rawTrip, agency) { diff --git a/validator/check_vehicles_test.go b/validator/check_vehicles_test.go index bd16a08..05c9164 100644 --- a/validator/check_vehicles_test.go +++ b/validator/check_vehicles_test.go @@ -104,6 +104,115 @@ func TestVehicleSamplingEmptyTripForVehicleWarns(t *testing.T) { } } +// vehicleSrcForTest builds a SourceContext with one sampled vehicle on trip T1. +func vehicleSrcForTest() *SourceContext { + return &SourceContext{ + Label: "ds0", + Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, + PrepErrors: map[string]error{}, + Static: staticForVehicle(), + VehiclePositions: >fs.Realtime{Vehicles: []gtfs.Vehicle{{ + ID: >fs.VehicleID{ID: "V1"}, + Trip: >fs.Trip{ID: gtfs.TripID{ID: "T1", RouteID: "R1"}}, + Position: >fs.Position{Latitude: f32(47.6), Longitude: f32(-122.3)}, + }}}, + } +} + +// The OBA server returns a literal `null` body (HTTP 200) for some queries; the +// SDK decodes that into a nil response with a nil error. The check must not +// dereference it. Each of the three OBA calls is covered below. + +func TestVehicleSamplingNullTripsForLocationResponse(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(r.URL.Path, "vehicles-for-agency"): + w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) + case strings.Contains(r.URL.Path, "trip-for-vehicle"): + w.Write([]byte(`{"data":{"entry":{"tripId":"1_T1"}}}`)) + case strings.Contains(r.URL.Path, "trips-for-location"): + w.Write([]byte(`null`)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + }) + vc := &ValidationContext{Config: cfgForTest("test"), Client: client} + results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) + if !hasWarn(results, "trips-for-location") { + t.Errorf("null trips-for-location response should Warn, got %v", results) + } + assertNoFail(t, results) +} + +// vehicles-for-agency is the agency-wide roster: the feed proves the agency has +// vehicles, so a null (like an empty) response is the API missing data the feed +// proves exists — a Fail, matching the sibling empty-list branch. +func TestVehicleSamplingNullVehiclesForAgencyFails(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(r.URL.Path, "vehicles-for-agency"): + w.Write([]byte(`null`)) + case strings.Contains(r.URL.Path, "trip-for-vehicle"): + w.Write([]byte(`{"data":{"entry":{"tripId":"1_T1"}}}`)) + case strings.Contains(r.URL.Path, "trips-for-location"): + w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + }) + vc := &ValidationContext{Config: cfgForTest("test"), Client: client} + results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) + if !hasStatus(results, "vehicles-for-agency", Fail) { + t.Errorf("null vehicles-for-agency response should Fail, got %v", results) + } +} + +func TestVehicleSamplingNullTripForVehicleResponse(t *testing.T) { + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.Contains(r.URL.Path, "vehicles-for-agency"): + w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) + case strings.Contains(r.URL.Path, "trip-for-vehicle"): + w.Write([]byte(`null`)) + case strings.Contains(r.URL.Path, "trips-for-location"): + w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + }) + vc := &ValidationContext{Config: cfgForTest("test"), Client: client} + results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) + if !hasWarn(results, "trip-for-vehicle") { + t.Errorf("null trip-for-vehicle response should Warn, got %v", results) + } + assertNoFail(t, results) +} + +func hasWarn(results []Result, checkSubstr string) bool { + return hasStatus(results, checkSubstr, Warn) +} + +func hasStatus(results []Result, checkSubstr string, status Status) bool { + for _, r := range results { + if strings.Contains(r.Check, checkSubstr) && r.Status == status { + return true + } + } + return false +} + +func assertNoFail(t *testing.T, results []Result) { + t.Helper() + for _, r := range results { + if r.Status == Fail { + t.Errorf("unexpected Fail: %s %s", r.Check, r.Message) + } + } +} + func TestVehicleSamplingUnresolvableAgencyWarns(t *testing.T) { src := &SourceContext{ Label: "ds0", diff --git a/validator/util.go b/validator/util.go index 244520e..435217e 100644 --- a/validator/util.go +++ b/validator/util.go @@ -17,6 +17,17 @@ func redact(err error, apiKey string) string { return s } +// withReason appends a redacted error to msg as ": ", or returns msg +// unchanged when err is nil — e.g. when a step failed because the server +// returned an empty list or a null body rather than a transport error, so the +// message reads cleanly instead of trailing a bare colon. +func withReason(msg string, err error, apiKey string) string { + if err == nil { + return msg + } + return msg + ": " + redact(err, apiKey) +} + // sampleByID deterministically selects up to n items: it sorts by keyFn(item) // and returns the first n, so repeated runs sample the same entities. func sampleByID[T any](items []T, n int, keyFn func(T) string) []T { From d0e0348df7ede7e5be7a6ab3e4a303023d3fa923 Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Mon, 25 May 2026 15:00:57 -0700 Subject: [PATCH 2/2] refactor: DRY up checks and tests to cut duplication SonarQube flagged 24.5% duplication on new code. Remove the duplicated blocks: - Production: extract `queryArrivals`, shared by the alert and trip-update cross-reference checks, replacing the identical error/null-response guard each had inline. - Tests: add shared helpers (`arrivalsClient`, `endpointsClient`, `vehicleClient`, `baseSrc`, `alertSrcForStop`, `tripUpdateSrcForStop`, `assertFirstStatus`), collapse the three vehicle null-response tests into one table test, and route the happy-path/empty/404 tests through the same builders instead of repeating httptest handlers and SourceContext literals. Behavior unchanged; full suite green, `dupl -threshold 100` reports zero clone groups in the package. --- validator/check_alerts.go | 32 +++-- validator/check_alerts_test.go | 156 ++++------------------- validator/check_endpoints_test.go | 72 +++++------ validator/check_tripupdates.go | 14 +-- validator/check_tripupdates_test.go | 105 +++------------- validator/check_vehicles_test.go | 189 +++++++--------------------- validator/helpers_test.go | 94 ++++++++++++++ 7 files changed, 230 insertions(+), 432 deletions(-) create mode 100644 validator/helpers_test.go diff --git a/validator/check_alerts.go b/validator/check_alerts.go index beee225..03f7f57 100644 --- a/validator/check_alerts.go +++ b/validator/check_alerts.go @@ -48,16 +48,9 @@ func (serviceAlertCheck) Run(ctx context.Context, vc *ValidationContext, src *So var out []Result for _, s := range sample { obaStop := PrefixedID(agency, s.rawStop) - ad, err := vc.Client.ArrivalAndDeparture.List(ctx, obaStop, onebusaway.ArrivalAndDepartureListParams{}) - if err != nil { - out = append(out, Result{Check: name, Source: src.Label, Status: Warn, - Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, key))}) - continue - } - if ad == nil { - out = append(out, Result{Check: name, Source: src.Label, Status: Warn, - Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop), - Details: map[string]any{"stopId": obaStop}}) + ad, bad := queryArrivals(ctx, vc, name, src.Label, obaStop) + if bad != nil { + out = append(out, *bad) continue } anySituation := false @@ -92,3 +85,22 @@ func (serviceAlertCheck) Run(ctx context.Context, vc *ValidationContext, src *So } return out } + +// queryArrivals fetches arrivals-and-departures for a stop. It returns a non-nil +// *Result — a Warn the caller should record before skipping the stop — when the +// call errors or the server returns a null body, so the two cross-reference +// checks (alerts, trip-updates) share identical "couldn't read this stop" +// handling. A null body must never be read as "stop confirmed empty". +func queryArrivals(ctx context.Context, vc *ValidationContext, check, label, obaStop string) (*onebusaway.ArrivalAndDepartureListResponse, *Result) { + ad, err := vc.Client.ArrivalAndDeparture.List(ctx, obaStop, onebusaway.ArrivalAndDepartureListParams{}) + switch { + case err != nil: + return nil, &Result{Check: check, Source: label, Status: Warn, + Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, vc.Config.APIKey))} + case ad == nil: + return nil, &Result{Check: check, Source: label, Status: Warn, + Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop), + Details: map[string]any{"stopId": obaStop}} + } + return ad, nil +} diff --git a/validator/check_alerts_test.go b/validator/check_alerts_test.go index 6f74d2f..061ba7f 100644 --- a/validator/check_alerts_test.go +++ b/validator/check_alerts_test.go @@ -3,7 +3,6 @@ package validator import ( "context" "net/http" - "strings" "testing" gtfs "github.com/OneBusAway/go-gtfs" @@ -12,57 +11,19 @@ import ( ) func TestServiceAlertFoundInSituationIDs(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_ALERT1"]}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_ALERT1"]}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Pass { - t.Errorf("want Pass, got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Pass, "alert in situationIds") } // A `null` arrivals response (nil SDK response, nil error) must not be mistaken // for "stop has no situations" and Fail — it is an unconfirmed query, so Warn. func TestServiceAlertNullArrivalsResponseWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`null`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + client := arrivalsClient(t, `null`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Warn { - t.Errorf("null arrivals response: want Warn, got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Warn, "null arrivals response") } func TestServiceAlertNoSamplableWarns(t *testing.T) { @@ -75,109 +36,36 @@ func TestServiceAlertNoSamplableWarns(t *testing.T) { } vc := &ValidationContext{Config: cfgForTest("test")} results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if results[0].Status != Warn { - t.Errorf("agency-only alert not stop-referenceable: want Warn got %v", results[0].Status) - } + assertFirstStatus(t, results, Warn, "agency-only alert not stop-referenceable") } func TestServiceAlertNoSituationsFails(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - // Arrivals present but NO situations at all, though the feed says this stop is affected. - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + // Arrivals present but NO situations at all, though the feed says this stop is affected. + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Fail { - t.Errorf("affected stop with no situations should Fail, got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Fail, "affected stop with no situations") } func TestServiceAlertSituationsButNoMatchWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - // Situations exist but none match the feed alert id. - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_DIFFERENT"]}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + // Situations exist but none match the feed alert id. + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","situationIds":["1_DIFFERENT"]}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Warn { - t.Errorf("situations present but no match should Warn, got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Warn, "situations present but no match") } func TestServiceAlertFoundInGlobalReferences(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - // situationIds empty on the arrival, but the alert IS in references.situations - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]},"references":{"situations":[{"id":"1_ALERT1"}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + // situationIds empty on the arrival, but the alert IS in references.situations. + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1"}]},"references":{"situations":[{"id":"1_ALERT1"}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Pass { - t.Errorf("alert in global references.situations should Pass, got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Pass, "alert in global references.situations") } func TestServiceAlert404StopWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - ServiceAlerts: >fs.Realtime{Alerts: []gtfs.Alert{{ - ID: "ALERT1", - InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, - }}}, - } + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) }) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := serviceAlertCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Warn { - t.Errorf("404 on stop should Warn (not Fail), got %+v", results) - } + results := serviceAlertCheck{}.Run(context.Background(), vc, alertSrcForStop()) + assertFirstStatus(t, results, Warn, "404 on stop should Warn not Fail") } diff --git a/validator/check_endpoints_test.go b/validator/check_endpoints_test.go index 60dc2d4..0556be8 100644 --- a/validator/check_endpoints_test.go +++ b/validator/check_endpoints_test.go @@ -27,34 +27,7 @@ func cfgForTest(apiKey string) config.Config { } func TestEndpointsCheckHappyPath(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - p := r.URL.Path - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(p, "current-time"): - w.Write([]byte(`{"data":{"entry":{"time":1716000000000,"readableTime":"now"}}}`)) - case strings.Contains(p, "agencies-with-coverage"): - w.Write([]byte(`{"data":{"list":[{"agencyId":"1"}],"references":{"agencies":[{"id":"1","name":"Metro"}]}}}`)) - case strings.Contains(p, "routes-for-agency"): - w.Write([]byte(`{"data":{"list":[{"id":"1_R1","agencyId":"1"}]}}`)) - case strings.Contains(p, "stops-for-route"): - w.Write([]byte(`{"data":{"entry":{"routeId":"1_R1","stopIds":["1_S1"]}}}`)) - case strings.Contains(p, "stops-for-location"): - w.Write([]byte(`{"data":{"outOfRange":false,"list":[{"id":"1_S1"}]}}`)) - case strings.Contains(p, "arrivals-and-departures-for-stop"): - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_S1","tripId":"1_T1","vehicleId":"1_V1","routeId":"1_R1"}]}}}`)) - case strings.Contains(p, "/stop/"): - w.Write([]byte(`{"data":{"entry":{"id":"1_S1","lat":47.6,"lon":-122.3,"name":"Stop"}}}`)) - default: - t.Errorf("unexpected path %s", p) - } - }) - ag, err := client.AgenciesWithCoverage.List(context.Background()) - if err != nil { - t.Fatal(err) - } - vc := &ValidationContext{Config: cfgForTest("test"), Client: client, Agencies: ag} - + vc := endpointsVC(t, endpointsClient(t, "")) results := endpointsCheck{}.Run(context.Background(), vc) for _, r := range results { if r.Status == Fail || r.Status == Skip { @@ -88,6 +61,35 @@ func validEndpointBody(p string) string { return "" } +// endpointsClient answers every endpoint with its happy-path body, except the +// step whose path contains nullStep (when non-empty), which returns `null`. +func endpointsClient(t *testing.T, nullStep string) *onebusaway.Client { + t.Helper() + return newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if nullStep != "" && strings.Contains(r.URL.Path, nullStep) { + w.Write([]byte(`null`)) + return + } + if body := validEndpointBody(r.URL.Path); body != "" { + w.Write([]byte(body)) + return + } + t.Errorf("unexpected path %s", r.URL.Path) + }) +} + +// endpointsVC pre-fetches agencies-with-coverage (as Run expects) and returns a +// context wired to client. +func endpointsVC(t *testing.T, client *onebusaway.Client) *ValidationContext { + t.Helper() + ag, err := client.AgenciesWithCoverage.List(context.Background()) + if err != nil { + t.Fatal(err) + } + return &ValidationContext{Config: cfgForTest("test"), Client: client, Agencies: ag} +} + // OBA can return a literal `null` body (HTTP 200) for a working-looking // endpoint; the SDK decodes that into a nil response with a nil error. A core // endpoint returning null is a server fault, so this smoke check must Fail @@ -105,19 +107,7 @@ func TestEndpointsNullResponseFailsPerStep(t *testing.T) { } for _, tc := range steps { t.Run(tc.step, func(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - if strings.Contains(r.URL.Path, tc.pathSubstr) { - w.Write([]byte(`null`)) - return - } - w.Write([]byte(validEndpointBody(r.URL.Path))) - }) - ag, err := client.AgenciesWithCoverage.List(context.Background()) - if err != nil { - t.Fatal(err) - } - vc := &ValidationContext{Config: cfgForTest("test"), Client: client, Agencies: ag} + vc := endpointsVC(t, endpointsClient(t, tc.pathSubstr)) results := endpointsCheck{}.Run(context.Background(), vc) var got Result for _, r := range results { diff --git a/validator/check_tripupdates.go b/validator/check_tripupdates.go index 5e0123c..df9a68d 100644 --- a/validator/check_tripupdates.go +++ b/validator/check_tripupdates.go @@ -5,7 +5,6 @@ import ( "fmt" gtfs "github.com/OneBusAway/go-gtfs" - onebusaway "github.com/OneBusAway/go-sdk" ) type tripUpdateSamplingCheck struct{} @@ -58,16 +57,9 @@ func (tripUpdateSamplingCheck) Run(ctx context.Context, vc *ValidationContext, s } obaStop := PrefixedID(agency, rawStop) - ad, err := vc.Client.ArrivalAndDeparture.List(ctx, obaStop, onebusaway.ArrivalAndDepartureListParams{}) - if err != nil { - out = append(out, Result{Check: name, Source: src.Label, Status: Warn, - Message: fmt.Sprintf("could not query stop %q (agency prefix may be wrong): %s", obaStop, redact(err, key))}) - continue - } - if ad == nil { - out = append(out, Result{Check: name, Source: src.Label, Status: Warn, - Message: fmt.Sprintf("arrivals query for stop %q returned a null response", obaStop), - Details: map[string]any{"stopId": obaStop}}) + ad, bad := queryArrivals(ctx, vc, name, src.Label, obaStop) + if bad != nil { + out = append(out, *bad) continue } found := false diff --git a/validator/check_tripupdates_test.go b/validator/check_tripupdates_test.go index 2ac018a..2c76914 100644 --- a/validator/check_tripupdates_test.go +++ b/validator/check_tripupdates_test.go @@ -3,72 +3,28 @@ package validator import ( "context" "net/http" - "strings" "testing" gtfs "github.com/OneBusAway/go-gtfs" - - "github.com/onebusaway/oba-validator/config" ) func TestTripUpdateSamplingFound(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","vehicleId":"1_V1","routeId":"1_R1"}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), // trip T1 -> agency KCM - TripUpdates: >fs.Realtime{Trips: []gtfs.Trip{{ - ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, - StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, - }}}, - } + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_T1","vehicleId":"1_V1","routeId":"1_R1"}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Pass { - t.Errorf("want Pass, got %+v", results) - } + results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, tripUpdateSrcForStop()) + assertFirstStatus(t, results, Pass, "predicted trip present at stop") } func TestTripUpdateSamplingAbsentFails(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - // Arrivals exist, but for a DIFFERENT trip than the feed predicts. - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_OTHER"}]}}}`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - TripUpdates: >fs.Realtime{Trips: []gtfs.Trip{{ - ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, - StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, - }}}, - } + // Arrivals exist, but for a DIFFERENT trip than the feed predicts. + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[{"stopId":"1_ST1","tripId":"1_OTHER"}]}}}`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Fail { - t.Errorf("predicted trip absent from arrivals should Fail, got %+v", results) - } + results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, tripUpdateSrcForStop()) + assertFirstStatus(t, results, Fail, "predicted trip absent from arrivals") } func TestTripUpdateSamplingNilStaticNoPanic(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"data":{"entry":{"arrivalsAndDepartures":[]}}}`)) - }) + client := arrivalsClient(t, `{"data":{"entry":{"arrivalsAndDepartures":[]}}}`) src := &SourceContext{ Label: "ds0", PrepErrors: map[string]error{}, @@ -84,50 +40,17 @@ func TestTripUpdateSamplingNilStaticNoPanic(t *testing.T) { } func TestTripUpdateSampling404StopWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNotFound) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - TripUpdates: >fs.Realtime{Trips: []gtfs.Trip{{ - ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, - StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, - }}}, - } + client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) }) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Warn { - t.Errorf("404 on stop should Warn (not Fail), got %+v", results) - } + results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, tripUpdateSrcForStop()) + assertFirstStatus(t, results, Warn, "404 on stop should Warn not Fail") } // A `null` arrivals response (nil SDK response, nil error) must not be mistaken // for "predicted trip absent" and Fail — it is an unconfirmed query, so Warn. func TestTripUpdateNullArrivalsResponseWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`null`)) - return - } - t.Errorf("unexpected path %s", r.URL.Path) - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - TripUpdates: >fs.Realtime{Trips: []gtfs.Trip{{ - ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, - StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, - }}}, - } + client := arrivalsClient(t, `null`) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, src) - if len(results) == 0 || results[0].Status != Warn { - t.Errorf("null arrivals response: want Warn, got %+v", results) - } + results := tripUpdateSamplingCheck{}.Run(context.Background(), vc, tripUpdateSrcForStop()) + assertFirstStatus(t, results, Warn, "null arrivals response") } diff --git a/validator/check_vehicles_test.go b/validator/check_vehicles_test.go index 05c9164..441dc2b 100644 --- a/validator/check_vehicles_test.go +++ b/validator/check_vehicles_test.go @@ -2,7 +2,6 @@ package validator import ( "context" - "net/http" "strings" "testing" @@ -29,166 +28,66 @@ func staticForVehicle() *feeds.ParsedStatic { return p } +// vehicleSrcForTest builds a SourceContext with one sampled vehicle on trip T1. +func vehicleSrcForTest() *SourceContext { + s := baseSrc() + s.VehiclePositions = >fs.Realtime{Vehicles: []gtfs.Vehicle{{ + ID: >fs.VehicleID{ID: "V1"}, + Trip: >fs.Trip{ID: gtfs.TripID{ID: "T1", RouteID: "R1"}}, + Position: >fs.Position{Latitude: f32(47.6), Longitude: f32(-122.3)}, + }}} + return s +} + func TestVehicleSamplingHappyPath(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - p := r.URL.Path - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(p, "vehicles-for-agency"): - w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) - case strings.Contains(p, "trip-for-vehicle"): - w.Write([]byte(`{"data":{"entry":{"tripId":"1_T1"}}}`)) - case strings.Contains(p, "trips-for-location"): - w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) - default: - t.Errorf("unexpected path %s", p) - } - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - VehiclePositions: >fs.Realtime{Vehicles: []gtfs.Vehicle{{ - ID: >fs.VehicleID{ID: "V1"}, - Trip: >fs.Trip{ID: gtfs.TripID{ID: "T1", RouteID: "R1"}}, - Position: >fs.Position{Latitude: f32(47.6), Longitude: f32(-122.3)}, - }}}, - } + client := vehicleClient(t, vehicleValidBodies()) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := vehicleSamplingCheck{}.Run(context.Background(), vc, src) - for _, r := range results { - if r.Status == Fail { - t.Errorf("%s Fail: %s", r.Check, r.Message) - } - } + results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) + assertNoFail(t, results) if len(results) == 0 { t.Fatal("expected sub-results") } } func TestVehicleSamplingEmptyTripForVehicleWarns(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(r.URL.Path, "vehicles-for-agency"): - w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) - case strings.Contains(r.URL.Path, "trip-for-vehicle"): - w.Write([]byte(`{"data":{"entry":{"tripId":""}}}`)) // no current trip - case strings.Contains(r.URL.Path, "trips-for-location"): - w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) - default: - t.Errorf("unexpected path %s", r.URL.Path) - } - }) - src := &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - VehiclePositions: >fs.Realtime{Vehicles: []gtfs.Vehicle{{ - ID: >fs.VehicleID{ID: "V1"}, - Trip: >fs.Trip{ID: gtfs.TripID{ID: "T1", RouteID: "R1"}}, - Position: >fs.Position{Latitude: f32(47.6), Longitude: f32(-122.3)}, - }}}, - } - vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := vehicleSamplingCheck{}.Run(context.Background(), vc, src) - for _, r := range results { - if strings.Contains(r.Check, "trip-for-vehicle") && r.Status != Warn { - t.Errorf("empty current trip should Warn, got %v: %s", r.Status, r.Message) - } - if r.Status == Fail { - t.Errorf("unexpected Fail: %s %s", r.Check, r.Message) - } - } -} - -// vehicleSrcForTest builds a SourceContext with one sampled vehicle on trip T1. -func vehicleSrcForTest() *SourceContext { - return &SourceContext{ - Label: "ds0", - Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, - PrepErrors: map[string]error{}, - Static: staticForVehicle(), - VehiclePositions: >fs.Realtime{Vehicles: []gtfs.Vehicle{{ - ID: >fs.VehicleID{ID: "V1"}, - Trip: >fs.Trip{ID: gtfs.TripID{ID: "T1", RouteID: "R1"}}, - Position: >fs.Position{Latitude: f32(47.6), Longitude: f32(-122.3)}, - }}}, - } -} - -// The OBA server returns a literal `null` body (HTTP 200) for some queries; the -// SDK decodes that into a nil response with a nil error. The check must not -// dereference it. Each of the three OBA calls is covered below. - -func TestVehicleSamplingNullTripsForLocationResponse(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(r.URL.Path, "vehicles-for-agency"): - w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) - case strings.Contains(r.URL.Path, "trip-for-vehicle"): - w.Write([]byte(`{"data":{"entry":{"tripId":"1_T1"}}}`)) - case strings.Contains(r.URL.Path, "trips-for-location"): - w.Write([]byte(`null`)) - default: - t.Errorf("unexpected path %s", r.URL.Path) - } - }) + bodies := vehicleValidBodies() + bodies["trip-for-vehicle"] = `{"data":{"entry":{"tripId":""}}}` // no current trip + client := vehicleClient(t, bodies) vc := &ValidationContext{Config: cfgForTest("test"), Client: client} results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) - if !hasWarn(results, "trips-for-location") { - t.Errorf("null trips-for-location response should Warn, got %v", results) + if !hasWarn(results, "trip-for-vehicle") { + t.Errorf("empty current trip should Warn, got %+v", results) } assertNoFail(t, results) } -// vehicles-for-agency is the agency-wide roster: the feed proves the agency has -// vehicles, so a null (like an empty) response is the API missing data the feed -// proves exists — a Fail, matching the sibling empty-list branch. -func TestVehicleSamplingNullVehiclesForAgencyFails(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(r.URL.Path, "vehicles-for-agency"): - w.Write([]byte(`null`)) - case strings.Contains(r.URL.Path, "trip-for-vehicle"): - w.Write([]byte(`{"data":{"entry":{"tripId":"1_T1"}}}`)) - case strings.Contains(r.URL.Path, "trips-for-location"): - w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) - default: - t.Errorf("unexpected path %s", r.URL.Path) - } - }) - vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) - if !hasStatus(results, "vehicles-for-agency", Fail) { - t.Errorf("null vehicles-for-agency response should Fail, got %v", results) +// The OBA server returns a literal `null` body (HTTP 200) for some queries; the +// SDK decodes that into a nil response with a nil error. The check must not +// dereference it — each of the three OBA calls is covered here. +func TestVehicleSamplingNullResponses(t *testing.T) { + cases := []struct { + nullPath, wantCheck string + want Status + }{ + // vehicles-for-agency is the agency-wide roster: the feed proves the + // agency has vehicles, so a null (like an empty) response is the API + // missing data the feed proves exists — Fail, matching the empty branch. + {"vehicles-for-agency", "vehicles-for-agency", Fail}, + // Per-vehicle cross-refs are unconfirmed on a null response — Warn. + {"trip-for-vehicle", "trip-for-vehicle", Warn}, + {"trips-for-location", "trips-for-location", Warn}, } -} - -func TestVehicleSamplingNullTripForVehicleResponse(t *testing.T) { - client := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch { - case strings.Contains(r.URL.Path, "vehicles-for-agency"): - w.Write([]byte(`{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`)) - case strings.Contains(r.URL.Path, "trip-for-vehicle"): - w.Write([]byte(`null`)) - case strings.Contains(r.URL.Path, "trips-for-location"): - w.Write([]byte(`{"data":{"list":[{"tripId":"1_T1"}]}}`)) - default: - t.Errorf("unexpected path %s", r.URL.Path) - } - }) - vc := &ValidationContext{Config: cfgForTest("test"), Client: client} - results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) - if !hasWarn(results, "trip-for-vehicle") { - t.Errorf("null trip-for-vehicle response should Warn, got %v", results) + for _, tc := range cases { + t.Run(tc.nullPath, func(t *testing.T) { + bodies := vehicleValidBodies() + bodies[tc.nullPath] = `null` + vc := &ValidationContext{Config: cfgForTest("test"), Client: vehicleClient(t, bodies)} + results := vehicleSamplingCheck{}.Run(context.Background(), vc, vehicleSrcForTest()) + if !hasStatus(results, tc.wantCheck, tc.want) { + t.Errorf("null %s: want %v on %s, got %+v", tc.nullPath, tc.want, tc.wantCheck, results) + } + }) } - assertNoFail(t, results) } func hasWarn(results []Result, checkSubstr string) bool { diff --git a/validator/helpers_test.go b/validator/helpers_test.go new file mode 100644 index 0000000..4d160dc --- /dev/null +++ b/validator/helpers_test.go @@ -0,0 +1,94 @@ +package validator + +import ( + "net/http" + "strings" + "testing" + + gtfs "github.com/OneBusAway/go-gtfs" + onebusaway "github.com/OneBusAway/go-sdk" + + "github.com/onebusaway/oba-validator/config" +) + +// arrivalsClient returns a client that answers the arrivals-and-departures +// endpoint with body and rejects any other path. The cross-reference checks +// (alerts, trip-updates) only ever call that one endpoint. +func arrivalsClient(t *testing.T, body string) *onebusaway.Client { + t.Helper() + return newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "arrivals-and-departures-for-stop") { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(body)) + return + } + t.Errorf("unexpected path %s", r.URL.Path) + }) +} + +// baseSrc returns a source for agency KCM (mapped to OBA id "1") with static +// GTFS but no realtime feeds; callers attach the one feed they exercise. +func baseSrc() *SourceContext { + return &SourceContext{ + Label: "ds0", + Config: config.DataSource{AgencyMapping: map[string]string{"KCM": "1"}}, + PrepErrors: map[string]error{}, + Static: staticForVehicle(), + } +} + +// alertSrcForStop builds a source whose single alert affects stop ST1. +func alertSrcForStop() *SourceContext { + s := baseSrc() + s.ServiceAlerts = >fs.Realtime{Alerts: []gtfs.Alert{{ + ID: "ALERT1", + InformedEntities: []gtfs.AlertInformedEntity{{StopID: strp("ST1")}}, + }}} + return s +} + +// tripUpdateSrcForStop builds a source predicting trip T1 at stop ST1. +func tripUpdateSrcForStop() *SourceContext { + s := baseSrc() + s.TripUpdates = >fs.Realtime{Trips: []gtfs.Trip{{ + ID: gtfs.TripID{ID: "T1", RouteID: "R1"}, + StopTimeUpdates: []gtfs.StopTimeUpdate{{StopID: strp("ST1"), Arrival: >fs.StopTimeEvent{}}}, + }}} + return s +} + +// assertFirstStatus checks the first result's status, the common shape for the +// single-sample cross-reference tests. +func assertFirstStatus(t *testing.T, results []Result, want Status, desc string) { + t.Helper() + if len(results) == 0 || results[0].Status != want { + t.Errorf("%s: want %v, got %+v", desc, want, results) + } +} + +// vehicleValidBodies returns happy-path JSON keyed by endpoint path fragment for +// the vehicle-sampling check's three OBA calls. Callers override one entry to +// simulate an empty/null/mismatched response. +func vehicleValidBodies() map[string]string { + return map[string]string{ + "vehicles-for-agency": `{"data":{"list":[{"vehicleId":"1_V1","tripId":"1_T1"}]}}`, + "trip-for-vehicle": `{"data":{"entry":{"tripId":"1_T1"}}}`, + "trips-for-location": `{"data":{"list":[{"tripId":"1_T1"}]}}`, + } +} + +// vehicleClient returns a client that answers each vehicle-sampling endpoint +// with the matching body from bodies, rejecting any unmapped path. +func vehicleClient(t *testing.T, bodies map[string]string) *onebusaway.Client { + t.Helper() + return newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + for path, body := range bodies { + if strings.Contains(r.URL.Path, path) { + w.Write([]byte(body)) + return + } + } + t.Errorf("unexpected path %s", r.URL.Path) + }) +}