opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.18k stars 1.02k forks source link

Add nicer assertions to SiriTimetableSnapshotSourceTest #5877

Closed habrahamsson-skanetrafiken closed 4 months ago

habrahamsson-skanetrafiken commented 4 months ago

Summary

Inspired by the raptor module tests I wanted to make the assertions easier to work with in SiriTimetableSnapshotSourceTest.

The previous assertions were pretty verbose since you had to assert each call by itself. Now you can write something along the lines:

  assertEquals(
      "UPDATED | A1 0:00:15 0:00:15 | B1 0:00:25 0:00:25",
      env.getRealtimeTimetable(env.trip1)
  );

instead of this:

    var tripTimes = env.getTripTimesForTrip(env.trip1);
    assertEquals(RealTimeState.UPDATED, tripTimes.getRealTimeState());
    assertEquals(11, tripTimes.getArrivalTime(0));
    assertEquals(15, tripTimes.getDepartureTime(0));
    assertEquals(20, tripTimes.getDepartureTime(1));
    assertEquals(25, tripTimes.getArrivalTime(1));

There are a lot of open PRs touching this piece of code so there might be some conflicts. But since this makes it easier to write these tests I thought it might be worth creating this PR anyway.

Documentation

Added a little bit of documentation

vpaturet commented 4 months ago

I have just realized that expected and actual are inverted in assertFailure(), maybe you can apply the fix in this PR?

leonardehrenfried commented 4 months ago

I think this is a nice addition but in the GTFS case we are going to need a realtime state for each each stop. Something like: scheduled, updated, cancelled/skipped, no data. In Siri there is also "prediction inaccurate" and "recorded".

Would you want to add something like that or should that be GTFS-specific?

habrahamsson-skanetrafiken commented 4 months ago

I think this is a nice addition but in the GTFS case we are going to need a realtime state for each each stop. Something like: scheduled, updated, cancelled/skipped, no data. In Siri there is also "prediction inaccurate" and "recorded".

Would you want to add something like that or should that be GTFS-specific?

Sure, we should probably add the stuff that is generally interesting for most tests. And if there are some specific attributes that are only set in rare cases those can be manually asserted.

If we want to add realtime state for each stop, perhaps it's best to use abbreviations to not make it to verbose? Something like:

UPDATED | A1 [C] 0:00:15 0:00:15 | B1 [ND] 0:00:25 0:00:25 | C1 [U] 0:00:25 0:00:25"

C: Canceled U: Updated ND: No data PI: Prediction inaccurate R: Recorded

Or something along those lines?

leonardehrenfried commented 4 months ago

Looks great.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 73.07692% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 68.49%. Comparing base (17717d4) to head (397818f). Report is 12 commits behind head on dev-2.x.

Files Patch % Lines
...ransit/model/timetable/TripTimesStringBuilder.java 73.07% 4 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev-2.x #5877 +/- ## ============================================= - Coverage 68.49% 68.49% -0.01% - Complexity 16702 16711 +9 ============================================= Files 1915 1917 +2 Lines 72652 72686 +34 Branches 7447 7455 +8 ============================================= + Hits 49761 49783 +22 - Misses 20326 20337 +11 - Partials 2565 2566 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.