Fix handling of missing aimed departure time #5865
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5865 +/- ##
=============================================
+ Coverage 68.37% 68.46% +0.08%
- Complexity 16676 16690 +14
=============================================
Files 1914 1914
Lines 72652 72654 +2
Branches 7452 7453 +1
=============================================
+ Hits 49677 49742 +65
+ Misses 20418 20341 -77
- Partials 2557 2571 +14 ☔ View full report in Codecov by Sentry. |
|
@lassetyr In this specific case, does the error need to be reported in the OTP logs? Is this useful for debugging? |
|
I do not see the need for logging this error, so it can fail silently. |
|
It's great that we now have a way to write tests for all these kinds strange edge cases. Well done, @habrahamsson-skanetrafiken |
leonardehrenfried
requested review from
habrahamsson-skanetrafiken and
leonardehrenfried
and removed request for
lassetyr
| call.setExpectedArrivalTime(dateTimeHelper.zonedDateTime(expectedTime)); | ||
| call.setAimedArrivalTime(aimedTime == null ? null : dateTimeHelper.zonedDateTime(aimedTime)); | ||
| call.setExpectedArrivalTime( | ||
| expectedTime == null ? null : dateTimeHelper.zonedDateTime(expectedTime) |
There was a problem hiding this comment.
Can you add a method to the date time helper to extract the repeated null checks?
There was a problem hiding this comment.
I think an even better solution would be to only set the fields if the values are non-null.
There was a problem hiding this comment.
Indeed it sounds simpler to wrap the calls in if( not null)) statements.
| @BeforeEach | ||
| void setUp() { | ||
| env = new RealtimeTestEnvironment(); | ||
| } |
There was a problem hiding this comment.
This is more a matter of style so I will not insist, but I actually preferred @habrahamsson-skanetrafiken's way of instantiating. He can have the final say on it.
There was a problem hiding this comment.
The thing I like about having it in each test is that each test is self-contained and it makes it very easy to parametrize the environment, like if you want to setup some test with a different timezone for example.
There was a problem hiding this comment.
It seems to me that it reduces code duplication while leaving the possibility to customize the environment in each test (since the environment is recreated before each test and is mutable).
I have anyway no strong opinion about this, so I will revert to the previous way of instantiating.
| @@ -428,7 +476,7 @@ public RealtimeTestEnvironment() { | |||
| private record Stop(RegularStop stop, int arrivalTime, int departureTime) {} | |||
|
|
|||
| private Trip createTrip(String id, Route route, List<Stop> stops) { | |||
| var trip = Trip.of(id(id)).withRoute(route).build(); | |||
| var trip = Trip.of(id(id)).withRoute(route).withServiceId(SERVICE_ID).build(); | |||
There was a problem hiding this comment.
👍 I need this in the GTFS tests as well.
vpaturet
force-pushed
the
fix_missing_aimed_departure_time_handling
branch
from
f17eb1a to
3d8a392
Compare
src/ext-test/java/org/opentripplanner/ext/siri/SiriEtBuilder.java
Outdated
Show resolved
Hide resolved
src/ext-test/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSourceTest.java
Show resolved
Hide resolved
src/ext-test/java/org/opentripplanner/ext/siri/SiriEtBuilder.java
Outdated
Show resolved
Hide resolved
Summary
As detailed in #5864 , some SIRI updates fail with a NullPointerException. The root cause is a missing aimed departure date on the first call in the estimated vehicle journey (mandatory field in the Nordic SIRI profile).
This PR:
Note: the null-check on aimed departure date is moved early enough in the process to provide context for the error message, but without changing the outcome.
Note: the SIRI test framework is extended to support:
Issue
Closes #5864
Unit tests
Added unit tests
Documentation
No