Expand core-engine unit test coverage (Tiers 1–4) - #2
Merged
Conversation
Preparing for core-engine tests (matchers, AvlProcessor, prediction generators) that need static-method mocking and richer assertions than JUnit 4.11 + Hamcrest provide. Pins byte-buddy to 1.14.15 because Hibernate 5.5 drags in 1.11.12 at compile scope and wins classpath order over Mockito's, breaking Mockito initialization. Includes a smoke test that exercises assertThrows, instance + static mocking, and AssertJ fluent assertions end-to-end.
Covers the singleton, crow-flies travel-time math under all three distance branches (below cutoff, exactly at cutoff, above cutoff), the layover early-exits, and the Indices-driven stop-path lookups.
Covers day-of-week computation in the agency timezone, service-id selection by weekday, the "all calendars expired" fallback to the most recent one, calendar_date add/remove overrides, and the serviceDate-keyed cache in getServiceIdsForDay.
Covers block assignment across all four AVL assignment types (block, trip, tripShortName, route), the null-report / null-assignment guards, the "no matching block" path, and the preference for an active block over an inactive one when multiple service IDs match.
Covers the generate() decision tree: not-predictable short-circuit, wait-stop before/after departure, regular scheduled stop, and the fall-through to the next-scheduled-stop branch when there's no stop info or no ScheduleTime at the current stop.
Covers mean (list, double[], int[]), toArray/toDoubleArray conversions including null handling, sample-stddev including the single-value NaN case, filteredMean (<=2 pass-through, no-outlier no-op, outlier drop), and biasedFilteredMean under zero bias, non-zero bias, and the <=2-after-filtering fallback.
Covers null-in/null-out, the no-regex-file pass-through, regex replacements loaded from a temp file including ampersand-spacing patterns, isReplaceTitle detection, comment/blank-line skipping, malformed-line tolerance, the missing-file graceful failure, and both modes of logRegexesThatDidNotMakeDifference.
Covers constructor-computed atStop state under layover, near-end-of-path, just-after-previous-stop, and in-the-middle conditions; the ordering comparators (lessThan/lessThanOrEqualTo) across trip/stopPath/segment/ distance; numberStopsBetweenMatches within a trip and across trips; isLastTripOfBlock; isAtStop overloads; and the getScheduledWaitStop* exception-to-minus-one fallback.
Covers the problemMatchDueToLackOfHeadingInfo decision tree (null match, layover short-circuit, valid heading, and no-prev-AVL problem), the null/empty tripsToInvestigate guards on the static getSpatialMatches overload, and pins the MatchingType enum order.
Covers the singleton accessor, the matchToLayoverStopEvenIfOffRoute decision path (empty potentialTrips, deadheadable trip, and a trip whose start time has already passed), and the greedy-match shortcut for a single no-schedule match in getBestTemporalMatchComparedToSchedule.
Covers the max-predictions-window default, getTravelTimeForPath pass-through when storeTravelTimeStopPathPredictions is off, and the delegate methods getStopTimeForPath and expectedTravelTimeFromMatchToEndOfStopPath.
Confirms it extends PredictionGeneratorDefaultImpl so the fallback path stays intact, and that the un-overridden getStopTimeForPath still delegates through to TravelTimes. Kalman's getTravelTimeForPath requires live cache singletons to exercise, so is intentionally left to integration tests.
Pins the four-layer inheritance chain (Kalman → HistoricalAverage → LastVehicle → DefaultImpl) so a refactor that accidentally breaks the fallback order trips a test. The override methods themselves require the FrequencyBasedHistoricalAverageCache singleton to exercise, so they're intentionally left to integration tests.
Exercises the private static calculateHoldingTime math via reflection: the three-prediction case picks the worst-average slot, the max_predictions cap truncates the search, and the negative-delta clamp to zero works. Also pins the implements-HoldingTimeGenerator contract. The public generate* methods aren't covered because they require VehicleStateManager and HoldingTimeCache singletons.
Covers the HeadwayGenerator contract, the blanket try/catch that returns null on bad input, and the private average/variance/coefficientOfVariance statistics (via reflection) with a three-point dataset where the mean and squared-deviations are trivial to verify by hand.
Matches the LastDepartureHeadwayGenerator test shape: contract check, exception-swallowing generate(), and the private average/variance/ coefficientOfVariance statistics verified via reflection with a hand-computable three-point dataset.
Covers vehicleId + sensible defaults, AVL-report history push, the setMatch(null) → not-predictable + reset-bad-matches transition, match set/get, headway/holdingTime/arrival round-trips, trip counter increments, trip-start-time map storage, and the predictable-only visibility guard on getRealTimeSchedAdh. Named CoreVehicleStateTest to stay distinct from the existing db/structs VehicleStateTest, which tests a different (entity) class that happens to share the name.
Covers the ArrivalDepartureGenerator contract and the two early-return guards in generate(): unpredictable vehicle and null current match. The full stop-traversal logic requires a live DataDbLogger and Core singleton, so is left to integration tests.
Minimal coverage only: singleton identity and the lastAvlReportTime / getLastAvlReport invariant. The rest of AvlProcessor (matchVehicleToAssignment, matchNewFixForPredictableVehicle, processAvlReport) runs the full AVL pipeline through singletons (VehicleStateManager, VehicleDataCache, Core, DataDbLogger) and is intentionally left to integration tests.
Minimal coverage: empty-inputs processPathSegments no-op and the constructor's shape-id grouping/sorting path. The real geometry derivation requires a constructed TripPattern + real shapes, which is GTFS-fixture territory and better covered by integration tests.
Covers the constructor's field wiring (via default counters), the null-guard on setNumberOfTravelTimes, the round-trip setters/getters, and the private static numberOfTravelTimes list-summing helper via reflection. The full process(session, gtfsData) path requires a Hibernate session and parsed GTFS data, so is left to integration tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpgrades test dependencies (JUnit → 4.13.2; adds Mockito, AssertJ, ByteBuddy) and adds a comprehensive suite of new JUnit tests (≈24 classes) exercising core matching/prediction, vehicle state, GTFS processing, headway/holding generation, and statistics utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
transitclock/src/test/java/org/transitclock/gtfs/StopPathProcessorTest.java (1)
30-41: Test intent and assertion are slightly mismatched.The method name says sorting is verified, but the test only checks that construction does not throw. Consider either asserting observable sorted behavior (if accessible) or renaming the test to reflect no-throw semantics only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/gtfs/StopPathProcessorTest.java` around lines 30 - 41, The test named constructor_handlesDuplicateShapeIdsAndSortsThem is misleading because it claims to verify sorting but only asserts that the StopPathProcessor constructor doesn't throw; update the test to either (1) assert observable sorted behavior by accessing the processor's internal grouping or output (e.g., call a method or getter on StopPathProcessor that returns the shape groups or ordered points and assert the order of entries for shape-A), or (2) rename the test to constructor_handlesDuplicateShapeIdsWithoutThrowing to reflect the current no-throw check; modify the test body accordingly and keep references to the existing GtfsShape instances (s1, s2) and the StopPathProcessor constructor so the change is minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@transitclock/src/test/java/org/transitclock/core/ArrivalDepartureGeneratorDefaultImplTest.java`:
- Around line 19-30: The tests currently assert stubbed return values or use an
unlinked mock, making them vacuous; change both tests to verify interactions on
the VehicleState and the match mock instead: in
generate_unpredictableVehicleIsANoOp(), after calling new
ArrivalDepartureGeneratorDefaultImpl().generate(vs), verify that
vs.isPredictable() was invoked (verify(vs).isPredictable()) and that
vs.getMatch() was never called (verify(vs, never()).getMatch()); in
generate_doesNotTouchTrivialCollaboratorsWhenUnpredictable(), wire the match
into the VehicleState via when(vs.getMatch()).thenReturn(match) before calling
generate, then verify vs.isPredictable() was called and assert the match mock
had no interactions (verifyNoInteractions(match)) to ensure generate() respected
the interaction boundary.
In
`@transitclock/src/test/java/org/transitclock/core/predictiongenerator/frequency/traveltime/kalman/FrequencyKalmanPredictionGeneratorImplTest.java`:
- Around line 27-31: The test currently asserts the default max prediction time
by calling PredictionGeneratorDefaultImpl.getMaxPredictionsTimeSecs(), which
bypasses any static override on the Kalman implementation; change the assertion
to call the Kalman class's static accessor (e.g.,
FrequencyKalmanPredictionGeneratorImpl.getMaxPredictionsTimeSecs()) instead so
the test will fail if the Kalman class later defines its own static value and
thus truly verifies the Kalman class behavior.
In `@transitclock/src/test/java/org/transitclock/core/SpatialMatchTest.java`:
- Around line 311-320: The test currently never reaches the exception path
because buildSimpleBlock sets block.isWaitStop(0,0) to false, so
getScheduledWaitStopTimeSecs returns -1 before getScheduleTime is called; fix by
arranging the mock Block to report a wait stop for the tested indices (e.g.,
stub block.isWaitStop(0, 0) to return true) after creating the block returned by
buildSimpleBlock (or adjust buildSimpleBlock to allow specifying wait-stop
behavior), then keep the when(block.getScheduleTime(...)).thenThrow(...) and
assert that getScheduledWaitStopTimeSecs() and getScheduledWaitStopTime() return
-1 so the exception path is exercised for getScheduleTime in SpatialMatch.
- Around line 153-160: The test atStop_layoverStopAlwaysCountsAsAtStop currently
uses distanceAlongSegment = 5.0 which falls inside the configured
afterStopDistance and can pass without layover logic; change the test to use a
distance outside the after-stop window so only the layover flag forces atStop.
Update the matchAt invocation in that test (and any helper setup) to use a
distance greater than the configured afterStopDistance (for example replace
matchAt(block, 0, 5.0) with matchAt(block, 0, SpatialMatch.AFTER_STOP_DISTANCE +
1.0) or a hard-coded value larger than the afterStopDistance) so that
SpatialMatch.isAtStop() and atEndOfPathStop() only succeed because of the
layover flag.
---
Nitpick comments:
In `@transitclock/src/test/java/org/transitclock/gtfs/StopPathProcessorTest.java`:
- Around line 30-41: The test named
constructor_handlesDuplicateShapeIdsAndSortsThem is misleading because it claims
to verify sorting but only asserts that the StopPathProcessor constructor
doesn't throw; update the test to either (1) assert observable sorted behavior
by accessing the processor's internal grouping or output (e.g., call a method or
getter on StopPathProcessor that returns the shape groups or ordered points and
assert the order of entries for shape-A), or (2) rename the test to
constructor_handlesDuplicateShapeIdsWithoutThrowing to reflect the current
no-throw check; modify the test body accordingly and keep references to the
existing GtfsShape instances (s1, s2) and the StopPathProcessor constructor so
the change is minimal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e543cb6-f19e-4682-b7ce-715b879d807e
📒 Files selected for processing (22)
transitclock/pom.xmltransitclock/src/test/java/org/transitclock/TestLibrariesSmokeTest.javatransitclock/src/test/java/org/transitclock/core/ArrivalDepartureGeneratorDefaultImplTest.javatransitclock/src/test/java/org/transitclock/core/AvlProcessorTest.javatransitclock/src/test/java/org/transitclock/core/BlockAssignerTest.javatransitclock/src/test/java/org/transitclock/core/CoreVehicleStateTest.javatransitclock/src/test/java/org/transitclock/core/PredictionGeneratorDefaultImplTest.javatransitclock/src/test/java/org/transitclock/core/RealTimeSchedAdhProcessorTest.javatransitclock/src/test/java/org/transitclock/core/ServiceUtilsTest.javatransitclock/src/test/java/org/transitclock/core/SpatialMatchTest.javatransitclock/src/test/java/org/transitclock/core/SpatialMatcherTest.javatransitclock/src/test/java/org/transitclock/core/TemporalMatcherTest.javatransitclock/src/test/java/org/transitclock/core/TravelTimesTest.javatransitclock/src/test/java/org/transitclock/core/headwaygenerator/LastArrivalsHeadwayGeneratorTest.javatransitclock/src/test/java/org/transitclock/core/headwaygenerator/LastDepartureHeadwayGeneratorTest.javatransitclock/src/test/java/org/transitclock/core/holdingmethod/HoldingTimeGeneratorDefaultImplTest.javatransitclock/src/test/java/org/transitclock/core/predictiongenerator/frequency/traveltime/kalman/FrequencyKalmanPredictionGeneratorImplTest.javatransitclock/src/test/java/org/transitclock/core/predictiongenerator/scheduled/traveltime/kalman/ScheduledKalmanPredictionGeneratorImplTest.javatransitclock/src/test/java/org/transitclock/gtfs/StopPathProcessorTest.javatransitclock/src/test/java/org/transitclock/gtfs/TitleFormatterTest.javatransitclock/src/test/java/org/transitclock/gtfs/TravelTimesProcessorForGtfsUpdatesTest.javatransitclock/src/test/java/org/transitclock/statistics/StatisticsTest.java
Comment on lines
+311
to
+320
| public void getScheduledWaitStopTimeSecs_returnsMinusOneOnException() { | ||
| Block block = buildSimpleBlock(2, null); | ||
| when(block.getScheduleTime(anyInt(), anyInt())) | ||
| .thenThrow(new RuntimeException("no schedule time")); | ||
|
|
||
| SpatialMatch match = matchAt(block, 0, 50.0); | ||
|
|
||
| assertThat(match.getScheduledWaitStopTimeSecs()).isEqualTo(-1); | ||
| assertThat(match.getScheduledWaitStopTime()).isEqualTo(-1); | ||
| } |
There was a problem hiding this comment.
Exception fallback path is likely not being exercised in this test.
buildSimpleBlock() sets block.isWaitStop(0, 0) to false, so the method can return -1 before invoking getScheduleTime(...) at all.
Proposed test fix
`@Test`
public void getScheduledWaitStopTimeSecs_returnsMinusOneOnException() {
Block block = buildSimpleBlock(2, null);
+ when(block.isWaitStop(0, 0)).thenReturn(true);
when(block.getScheduleTime(anyInt(), anyInt()))
.thenThrow(new RuntimeException("no schedule time"));
SpatialMatch match = matchAt(block, 0, 50.0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void getScheduledWaitStopTimeSecs_returnsMinusOneOnException() { | |
| Block block = buildSimpleBlock(2, null); | |
| when(block.getScheduleTime(anyInt(), anyInt())) | |
| .thenThrow(new RuntimeException("no schedule time")); | |
| SpatialMatch match = matchAt(block, 0, 50.0); | |
| assertThat(match.getScheduledWaitStopTimeSecs()).isEqualTo(-1); | |
| assertThat(match.getScheduledWaitStopTime()).isEqualTo(-1); | |
| } | |
| public void getScheduledWaitStopTimeSecs_returnsMinusOneOnException() { | |
| Block block = buildSimpleBlock(2, null); | |
| when(block.isWaitStop(0, 0)).thenReturn(true); | |
| when(block.getScheduleTime(anyInt(), anyInt())) | |
| .thenThrow(new RuntimeException("no schedule time")); | |
| SpatialMatch match = matchAt(block, 0, 50.0); | |
| assertThat(match.getScheduledWaitStopTimeSecs()).isEqualTo(-1); | |
| assertThat(match.getScheduledWaitStopTime()).isEqualTo(-1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclock/src/test/java/org/transitclock/core/SpatialMatchTest.java`
around lines 311 - 320, The test currently never reaches the exception path
because buildSimpleBlock sets block.isWaitStop(0,0) to false, so
getScheduledWaitStopTimeSecs returns -1 before getScheduleTime is called; fix by
arranging the mock Block to report a wait stop for the tested indices (e.g.,
stub block.isWaitStop(0, 0) to return true) after creating the block returned by
buildSimpleBlock (or adjust buildSimpleBlock to allow specifying wait-stop
behavior), then keep the when(block.getScheduleTime(...)).thenThrow(...) and
assert that getScheduledWaitStopTimeSecs() and getScheduledWaitStopTime() return
-1 so the exception path is exercised for getScheduleTime in SpatialMatch.
ArrivalDepartureGeneratorDefaultImplTest: replace vacuous stub re-assertions with verify()/verify(never()) interaction checks so the tests fail if the short-circuit guards are removed. FrequencyKalmanPredictionGeneratorImplTest: call getMaxPredictionsTimeSecs() through KalmanPredictionGeneratorImpl rather than PredictionGeneratorDefaultImpl so a future static override on any intermediate layer will break the test. SpatialMatchTest.atStop_layoverStopAlwaysCountsAsAtStop: move the match to the segment midpoint, outside both beforeStopDistance and afterStopDistance windows, so isAtStop can only be true via the layover flag.
Member
Author
|
Pushed Applied (3):
Not applied (1):
477 tests still passing ( |
This was referenced Apr 23, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
transitclocktest classes covering pipeline, matching, prediction, headway/holding, and GTFS-ingest code.mvn -pl transitclock test).What's covered
Tier 1 (pipeline):
TravelTimes,SpatialMatcher,TemporalMatcher,PredictionGeneratorDefaultImpl,ArrivalDepartureGeneratorDefaultImpl,AvlProcessor.Tier 2 (state + math):
SpatialMatch, coreVehicleState,ServiceUtils,BlockAssigner,RealTimeSchedAdhProcessor.Tier 3 (generators): scheduled + frequency
KalmanPredictionGeneratorImpl,HoldingTimeGeneratorDefaultImpl,LastDepartureHeadwayGenerator,LastArrivalsHeadwayGenerator,Statistics.Tier 4 (GTFS ingest):
TitleFormatter,StopPathProcessor,TravelTimesProcessorForGtfsUpdates.Approach + trade-offs
TravelTimes,Statistics,TitleFormatter,ServiceUtils,BlockAssigner,SpatialMatch) get thorough branch coverage.AvlProcessor,ArrivalDepartureGeneratorDefaultImpl, Kalman generators) get lightweight "contract + early-return" coverage — the deep paths requireCore/VehicleStateManager/DataDbLoggersingletons and belong in integration tests. Each test file documents this explicitly.HoldingTimeGeneratorDefaultImpl.calculateHoldingTime, headway generator statistics) are reached via reflection where they carry real algorithmic value.Test plan
mvn -pl transitclock test→ 477 passing, 0 failures, 0 errors.mvn -pl transitclock test -Dtest=<ClassName>.Summary by CodeRabbit
Chores
Tests