-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prevent exception raised when serviceId is missing from calendar file #1646
Conversation
❌ Invalid acceptance test. |
9c29f43
to
35bb737
Compare
❌ Invalid acceptance test. |
❌ Invalid acceptance test. |
@davidgamez I did an initial round of checks on all 141 datasets to see if the feed itself was completely expired or not aka no services currently running. I did this check because that would indicate the issue is primarily with our stale data on the Mobility Database (or the producer hasn't updated the feed.) 70 out of 141 feeds were fully expired, which leaves us with 71 that have this warning and are actually active (5% of the overall Mobility Database). Next steps:
|
Based on internal discussion, next steps are:
|
c55390b
to
b449bc0
Compare
❌ Invalid acceptance test. |
@davidgamez I'm a bit surprised by the results of the acceptance tests! I'd still expect around 71 feeds to return this warning because their calendar_dates.date fields are all expired. Here's a few examples: ca-alberta-lethbridge-transit-gtfs-765: https://storage.googleapis.com/storage/v1/b/mdb-latest/o/ca-alberta-lethbridge-transit-gtfs-765.zip?alt=media ca-ontario-go-transit-gtfs-727: https://storage.googleapis.com/storage/v1/b/mdb-latest/o/ca-ontario-go-transit-gtfs-727.zip?alt=media Full original list of expired feeds is here. I didn't check them all to see if the producer actually made changes that would've impacted the re-run of the acceptance tests. |
… file and all dates are expired
d6f3f41
to
3605f4d
Compare
❌ Invalid acceptance test. |
❌ Invalid acceptance test. |
@davidgamez I took a quick look at the acceptance test results and this logic looks good to me. When we run these analytics before each new release, this can be my list to clean up/check for replacements for feeds in the Mobility Database. From a QA perspective, so long as we can run https://gtfs.pro/files/uran/improved-gtfs-dft-gtfs.zip successfully, this change can be passed. |
❌ Invalid acceptance test. |
} | ||
if (serviceDates.last().isBefore(dateForValidation.getDate())) { | ||
if (calendarTable.byServiceId(serviceId).isPresent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking here. In the case you know the calendarTable is empty (isCalendarTableEmpty == true), why bother doing a lookup on it?
if (calendarTable.byServiceId(serviceId).isPresent()) { | |
if (!isCalendarTableEmpty && calendarTable.byServiceId(serviceId).isPresent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. However, reducing the logic in the if condition helps readability. In either case, empty or not, the byService(serviceId) method will behave the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
❌ Invalid acceptance test. |
❌ Invalid acceptance test. |
Summary:
This PR fixes the issue raised here.
How to reproduce the bug:
Expected behavior:
All calendar expired notices are reported.
The root cause of the issue is the improper manipulation of the Optional.get method.
The fix:
What is new:
Acceptance Tests Results:
Acceptance tests fail due to the number of expired feeds that have not been reported. All cases are foreign service ID key violations or feeds with only calendar_dates.txt service IDs.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything