-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Transfer cache speed test #6362
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6362 +/- ##
=============================================
- Coverage 69.80% 69.73% -0.07%
- Complexity 17944 18025 +81
=============================================
Files 2046 2057 +11
Lines 76678 76970 +292
Branches 7830 7846 +16
=============================================
+ Hits 53526 53677 +151
- Misses 20411 20546 +135
- Partials 2741 2747 +6 ☔ View full report in Codecov by Sentry. |
application/src/test/java/org/opentripplanner/transit/speed_test/SpeedTest.java
Outdated
Show resolved
Hide resolved
application/src/test/java/org/opentripplanner/transit/speed_test/SpeedTest.java
Outdated
Show resolved
Hide resolved
64b10c2
to
041d1bb
Compare
0363fc8
to
ebbee33
Compare
ebbee33
to
25ffb73
Compare
I have extracted the transfer cache test into a separate file that is run separately. |
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.
This looks good to me. Just a minor comment
*/ | ||
class SetupHelper { | ||
|
||
static LoadModel loadGraph(File baseDir, URI path) { |
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.
path
should be @Nullable
here
Summary
It turns out we have a pretty big blind spot in our speed tests: transfer cache computation is not measured at all. This is added in this PR.
You can see a preview of this data here: https://tinyurl.com/224xmjfe
Issue
Relates to #6357