Skip to content
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

Introduce module tests for GTFS-RT #5871

Conversation

leonardehrenfried
Copy link
Member

Summary

As discussed in the realtime working group meeting, this introduces module tests for the GTFS-RT realtime code. They are ports of the existing tests but have been converted to use @habrahamsson-skanetrafiken's test environment code. This results in faster test execution and a less confusing debugging experience.

To make this possible, I extracted the bulk of the test environment into a base class which is subclassed by both the Siri and GTFS-RT test to add a few specific methods.

Unit tests

Well... yes.

Documentation

Javadoc for each test case.

@leonardehrenfried leonardehrenfried added Real-Time Update The issue/PR is related to RealTime updates Skip Changelog labels May 27, 2024
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner May 27, 2024 09:00
@leonardehrenfried leonardehrenfried force-pushed the clean-up-timetable-tests branch from b4d5495 to 6abcd5f Compare May 27, 2024 09:05
Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.54%. Comparing base (f535524) to head (410284c).

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5871      +/-   ##
=============================================
+ Coverage      68.51%   68.54%   +0.02%     
- Complexity     16725    16732       +7     
=============================================
  Files           1918     1918              
  Lines          72734    72734              
  Branches        7456     7456              
=============================================
+ Hits           49834    49854      +20     
+ Misses         20329    20310      -19     
+ Partials        2571     2570       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leonardehrenfried leonardehrenfried changed the title Introduce module test for GTFS-RT Introduce module testd for GTFS-RT May 27, 2024
@leonardehrenfried leonardehrenfried changed the title Introduce module testd for GTFS-RT Introduce module tests for GTFS-RT May 27, 2024
@habrahamsson-skanetrafiken
Copy link
Contributor

@leonardehrenfried I think the idea looks good! But in general I would prefer composition in favor of inheritance. Specifically if we ever want to test gtfs and siri together it would get clunky with an abstract base class.

@leonardehrenfried
Copy link
Member Author

@habrahamsson-skanetrafiken I've changed the inheritance to composition. It's not super-elegant either but less bad than some other options. My intention is for the split between Siri and GTFS test environment to become unnecessary once I've extracted a common timetable snapshot manager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we would want to merge the SiriEnv, the GtfsEnv and the RealtimeTestData into one class, then we could put two separate snapshot sources in the merged environment but make sure only one was initialized. And then throw a runtime exception if you try to call the gtfs methods on an environment where the siri snapshot source was initialized. I think this would work?

But maybe it's nice with the separation that we get now. I don't really have a strong opinion. So you can leave it as it is if you want.

I like the structure that the moduletests add!

Otherwise just some minor comments.

private TransitModel transitModel;

private final GtfsRealtimeFuzzyTripMatcher TRIP_MATCHER_NOOP = null;

private final boolean fullDataset = false;
private byte[] cancellation;
private final boolean FULL_DATASET = false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be called NOT_FULL_DATASET to make it clearer that we are setting the param to false in the call sites?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "not full" mean? Partial ? I guess the underlying problem is that the input param i a boolean and not an enum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thomas is correct. I wanted to turn this boolean into an enum for many years. Do you want me to do that in this PR or another one?

@leonardehrenfried leonardehrenfried force-pushed the clean-up-timetable-tests branch from ccb3903 to 22aa4b1 Compare June 2, 2024 20:17
@leonardehrenfried leonardehrenfried merged commit 73c418a into opentripplanner:dev-2.x Jun 4, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the clean-up-timetable-tests branch June 14, 2024 06:59
@t2gran t2gran added this to the 2.6 (next release) milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Real-Time Update The issue/PR is related to RealTime updates Skip Changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants