Pipeline tests - #3
Merged
Merged
Conversation
Covers pure-logic utilities and thin wrappers that had no coverage: MapKey, IntervalTimer, Encryption, OrderedCollection, ChinaGpsOffset in utils; TripPatternKey, Arrival/Departure in db/structs; and VehicleAtStopInfo in core. 73 tests total, all passing, no changes to production code.
Introduces a TransitFixtures helper in the test tree that assembles GtfsTrip, Trip, Block, and StopPath graphs via real constructors, then uses it to cover the logic that the first round skipped: Trip's addScheduleTimes inference, block-id fallback chain, and frequency copy constructors; Block's route-id dedup and lookup methods; and StopPath's setLocations pathLength calculation and flag getters. 30 new tests, all passing.
Adds a new Maven module that boots a real Core against an in-memory HSQL database populated with the WMATA 5A GTFS fixture, opt-in via an include-pipeline-tests profile so the default build is unchanged. CoreHarness is a JUnit ClassRule exposing helpers for subsequent behavior tests (dbConfig, setNow, clock). CoreHarnessSmokeTest proves the boot works. Two constraints surfaced and are worked around here: - GtfsData.isCalendarActiveInTheFuture hard-codes System.currentTimeMillis and the WMATA calendars expired in 2018. The harness stages the GTFS into a temp dir with rewritten far-future end_dates. - Hibernate's hbm2ddl.auto=create would drop+recreate tables every time Core rebuilds its SessionFactory (which it does during startup), wiping the imported data. Switched this module's hibernate config to hbm2ddl.auto=update.
Five behavior tests covering AvlProcessor#processAvlReport against the WMATA 5A dataset via CoreHarness: lastAvlReport updates, off-route unassigned vehicles are not predictable, invalid block assignments are tolerated, sequential reports share VehicleState, and the happy-path report does not throw. Each test advances Core's clock via a per-JVM monotonic counter because AvlProcessor#setLastAvlReport silently drops reports whose timestamp is not strictly greater than the last stored report's — JUnit 4's default (hash-based) method ordering would otherwise make the result dependent on which test ran last.
- Rename MapKeyTest.twoArgAndThreeArgKeysAreNotEqualEvenWhenLeadingFieldsMatch to AreEqualWhenTrailingSlotsAreNull. The body asserts equality, so the old name was a direct contradiction. - Fail loudly on malformed calendar.txt rows in CoreHarness. The old code silently passed through rows with <10 columns, which would leave the stale 2016 end_date intact and defeat the whole point of the rewrite. Now throws IOException with line number and content. New CoreHarnessCalendarRewriteTest pins the new loud-failure contract plus the canonical/empty-line cases. - Restore JVM system properties in CoreHarness.after() and assert Core is not already booted on entry. Belt-and-suspenders against surefire fork- config drift: reuseForks=false isolates classes today, but a change would otherwise leak configFiles/agencyId between sibling test classes silently.
- Add happy-path match test to AvlProcessorBehaviorTest. Picks trip 868588900 on block SE-08 (service 8) at its first stop (Dulles Airport Main Terminal), anchors Core's clock to 2016-06-20 11:50 EDT — a Monday when service 8 runs with no calendar_dates exclusion — and asserts the vehicle becomes predictable with a concrete TemporalMatch. This replaces the prior processAvlReportDoesNotThrowForTypicalNonMatchingReport test, which just rethrew RuntimeExceptions as AssertionErrors and added no value. - Route every clock advance through the nextTime AtomicLong. Adds advanceClockBy(deltaMs) and jumpClockTo(epochMs) helpers; the jump variant max-merges into nextTime so subsequent tests' @before hooks can't silently rewind the clock. - Add assertThat(state.getMatch()).isNull() to the off-route unassigned test, so "spatial match failed" is distinguished from "short-circuited before matching" — same failure signal today, different regression cause tomorrow. - Throw on non-regular entries in GTFS staging. The WMATA 5A fixture is flat but a fixture with a subdirectory or symlink would otherwise be silently skipped, producing confusing "file not found" errors far from the staging step. - Correct the GTFS column-order comment in rewriteCalendarWithFutureEndDates. GTFS doesn't require column ordering; the rewriter relies on the canonical order used by WMATA fixtures and would need a header-aware parser for a different layout. - Rename IntervalTimerTest.elapsedMsecStrHasThreeDecimalDigits to elapsedMsecStrParsesAsNonNegativeNumber — the old name promised a format check the body never performed. - Delete unused CoreHarness.withGtfs. No caller, speculative comment.
- Soften AvlProcessorBehaviorTest class-javadoc: stop enumerating specific
service_ids that would rot if the fixture is refreshed, point at the
fixture path and the HAPPY_PATH_* constants instead.
- TripTest.exactTimesHeadwayCopyConstructorOffsetsTimes now asserts that
each ScheduleTime in the copy has its arrival/departure offset by the
same shift. Previously only start/end/blockId were checked, so a
regression that left the schedule-time list at the base (unshifted)
values would pass. Also added an explicit isExactTimesHeadway() check.
- TripPatternKeyTest.constructorRejectsNullStopPathsList asserts on the
exception message fragment ("stopPaths") so a refactor that drops the
explicit guard in favor of an implicit dereference NPE is noticed.
- EncryptionTest.decryptOfTamperedCiphertextThrows truncates 4 trailing
chars instead of flipping a single char. The char-flip variant could
land on a base64 padding char for some schemes and produce a still-
decryptable value; truncation reliably destroys the ciphertext.
- Bump pipeline-tests module's logback-classic from 1.1.2 to 1.1.3 to
match transitclockCore's declared version.
- Add trailing newline to root .gitignore.
- README gains a "Running tests" section that lists the default, pipeline, and integration test invocations, including the first-run cost note. - CLAUDE.md gets (a) a bullet under "Build and test" that mirrors the existing integration-tests bullet, and (b) a transitclockPipelineTests entry in the module layout (bumping the module count from seven to eight). - CI runs the pipeline suite as a separate step after the unit-test build so failures are attributed distinctly. The existing surefire-reports upload-artifact already uses a **/target/surefire-reports glob and picks up the new module's reports automatically.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new opt-in Maven pipeline test module ( Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Pipeline Test
participant Harness as CoreHarness
participant Core as Core
participant DB as HSQL DB
participant Avl as AvlProcessor
rect rgba(200,200,255,0.5)
Test->>Harness: request boot with WMATA 5A fixture & setNow(t)
Harness->>DB: stage GTFS files and rewrite calendar.txt
Harness->>Core: initialize Core (reads DB/config)
Core->>DB: load persisted entities
end
rect rgba(200,255,200,0.5)
Test->>Avl: build AvlReport stamped with harness clock
Avl->>Core: submit report for processing
Core->>Avl: update/return VehicleState (predictable or unpredictable)
Avl->>Test: assertions on VehicleState and last report time
end
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: 8
🧹 Nitpick comments (10)
transitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.java (3)
59-60: Prefer value assertions over identity assertions forDatefields.Using
assertSamehere over-constrains implementation details and can block safe defensive-copy changes.Suggested test adjustment
- assertSame(avl, a.getAvlTime()); + assertEquals(avl, a.getAvlTime()); ... - assertSame(freqStart, a.getFreqStartTime()); + assertEquals(freqStart, a.getFreqStartTime()); ... - assertSame(freqStart, d.getFreqStartTime()); + assertEquals(freqStart, d.getFreqStartTime());Also applies to: 84-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.java` around lines 59 - 60, The test currently uses identity assertion assertSame(avl, a.getAvlTime()) which ties behavior to object identity; change this to a value assertion (e.g., assertEquals(avl, a.getAvlTime())) so Date equality is checked rather than reference equality, and apply the same change for the other Date assertions in the test (those around the 84-89 block) to avoid preventing defensive-copy implementations.
122-126: Make equality test use distinctDateinstances with equal values.Right now both objects share the same
Datereferences, so this test can pass even if equality accidentally relies on reference identity.Suggested test hardening
- Date t = time(1_000_000); - Date avl = time(999_000); - Arrival a = new Arrival(CONFIG_REV, VEHICLE_ID, t, avl, null, 0, 1, null); - Arrival b = new Arrival(CONFIG_REV, VEHICLE_ID, t, avl, null, 0, 1, null); + Arrival a = new Arrival(CONFIG_REV, VEHICLE_ID, time(1_000_000), time(999_000), null, 0, 1, null); + Arrival b = new Arrival(CONFIG_REV, VEHICLE_ID, time(1_000_000), time(999_000), null, 0, 1, null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.java` around lines 122 - 126, The test in ArrivalDepartureSubclassesTest creates Arrival instances using the same Date object references (t and avl) so equality may pass via reference identity; instead instantiate distinct Date objects with identical time values (e.g., new Date(time(...).getTime()) or use time(...) twice into separate variables) when constructing Arrival a and Arrival b so both have equal but not identical Date instances; update the lines that set Date t and avl and/or the second Arrival constructor call to use those distinct Date objects while keeping the same millisecond values.
14-17: Align class-level test description with actual coverage.The Javadoc says
withUpdatedTimeis verified, but this suite currently does not test it. Either add that test or trim the statement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.java` around lines 14 - 17, The class Javadoc claims withUpdatedTime is tested but no such test exists; add a unit test in ArrivalDepartureSubclassesTest that constructs an Arrival (and optionally a Departure), calls Arrival.withUpdatedTime(newTime) and asserts the returned object has the updated time (via getTime/getTimestamp), retains other fields unchanged, and still reports isArrival true (and similarly verify Departure behavior if applicable), referencing the Arrival class and its withUpdatedTime method as well as isArrival/isDeparture and getter methods to locate code to assert against.transitclock/src/test/java/org/transitclock/utils/ChinaGpsOffsetTest.java (1)
18-38: Add boundary-value assertions foroutOfChina()thresholds.Nice coverage for obvious in/out cases, but Lines 19-38 don’t verify exact cutoff behavior (
72.004,137.8347,0.8293,55.8271). Since the implementation uses strict</>, explicit edge tests would protect against future off-by-one regressions.Suggested test additions
`@Test` public void insideChinaBoundingBox() { + // Exact threshold values should be considered inside. + assertFalse(ChinaGpsOffset.outOfChina(0.8293, 110.0)); + assertFalse(ChinaGpsOffset.outOfChina(55.8271, 110.0)); + assertFalse(ChinaGpsOffset.outOfChina(30.0, 72.004)); + assertFalse(ChinaGpsOffset.outOfChina(30.0, 137.8347)); + // Beijing assertFalse(ChinaGpsOffset.outOfChina(39.9042, 116.4074)); // Zhengzhou (used in the class's own main()) assertFalse(ChinaGpsOffset.outOfChina(34.79521, 113.69259)); // Shanghai assertFalse(ChinaGpsOffset.outOfChina(31.2304, 121.4737)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/utils/ChinaGpsOffsetTest.java` around lines 18 - 38, Add explicit boundary-value assertions for ChinaGpsOffset.outOfChina to protect against off-by-one regressions: call outOfChina with the exact longitude thresholds 72.004 and 137.8347 (e.g., outOfChina(<anyLatWithinRange>, 72.004) and 137.8347) and assertFalse, and call outOfChina with the exact latitude thresholds 0.8293 and 55.8271 (e.g., outOfChina(0.8293, <anyLonWithinRange>) and 55.8271) and assertFalse; also consider adding one test just inside and one just outside each threshold (e.g., 72.0039 and 72.0041) to verify the strict comparison behavior in ChinaGpsOffset.outOfChina.transitclock/src/test/java/org/transitclock/utils/EncryptionTest.java (2)
54-79: Consider extracting repeated decrypt-failure assertion logic.The two exception tests repeat the same try/catch pattern; a helper would reduce duplication and keep tests focused on inputs.
🧹 Possible refactor
public class EncryptionTest { + + private void assertDecryptThrows(String input, String message) { + try { + Encryption.decrypt(input); + fail(message); + } catch (EncryptionOperationNotPossibleException expected) { + // expected + } + } `@Test` public void decryptOfTamperedCiphertextThrows() { String encrypted = Encryption.encrypt("original"); @@ - try { - Encryption.decrypt(tampered); - fail("Expected EncryptionOperationNotPossibleException on tampered ciphertext"); - } catch (EncryptionOperationNotPossibleException expected) { - // expected - } + assertDecryptThrows( + tampered, + "Expected EncryptionOperationNotPossibleException on tampered ciphertext"); } @@ `@Test` public void decryptOfGarbageThrows() { - try { - Encryption.decrypt("not-a-real-ciphertext"); - fail("Expected EncryptionOperationNotPossibleException on garbage input"); - } catch (EncryptionOperationNotPossibleException expected) { - // expected - } + assertDecryptThrows( + "not-a-real-ciphertext", + "Expected EncryptionOperationNotPossibleException on garbage input"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/utils/EncryptionTest.java` around lines 54 - 79, Extract the repeated try/catch/fail pattern in decryptOfTamperedCiphertextThrows and decryptOfGarbageThrows into a small test helper (e.g., assertDecryptFails(String ciphertext)) that calls Encryption.decrypt(ciphertext) and fails the test unless an EncryptionOperationNotPossibleException is thrown; then replace the try/catch blocks in both tests with calls to assertDecryptFails("not-a-real-ciphertext") and assertDecryptFails(tampered) to remove duplication and keep tests focused on inputs.
43-52: Strengthen Line 51 by asserting against the original plaintext directly.At Line 51, comparing
decrypt(a)todecrypt(b)is weaker than checking both against"sameInput".♻️ Suggested tweak
String a = Encryption.encrypt("sameInput"); String b = Encryption.encrypt("sameInput"); assertNotEquals(a, b); - // But both decrypt back to the same plaintext. - assertEquals(Encryption.decrypt(a), Encryption.decrypt(b)); + // But both decrypt back to the original plaintext. + assertEquals("sameInput", Encryption.decrypt(a)); + assertEquals("sameInput", Encryption.decrypt(b));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/utils/EncryptionTest.java` around lines 43 - 52, Test currently only compares Encryption.decrypt(a) to Encryption.decrypt(b); strengthen it by asserting both decryptions equal the original plaintext "sameInput". In the test method encryptionIsSaltedSoSameInputProducesDifferentCiphertexts, replace or supplement the existing assertEquals(Encryption.decrypt(a), Encryption.decrypt(b)) with two assertions that assertEquals("sameInput", Encryption.decrypt(a)) and assertEquals("sameInput", Encryption.decrypt(b)) so both decrypted values are explicitly validated against the original input while keeping the assertNotEquals(a, b) check for differing ciphertexts.transitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.java (1)
60-65: Use independent lists/instances when validating logical equalityBoth equality/hash tests currently reuse the same
pathslist reference for both keys. Using separate list/object instances would better guard against accidental identity-based regressions and make the tests stronger.Possible test hardening diff
- List<StopPath> paths = Arrays.asList( - stopPath(1, "p1", "s1", 0, "r1"), - stopPath(1, "p2", "s2", 1, "r1")); - TripPatternKey a = new TripPatternKey("shapeA", paths); - TripPatternKey b = new TripPatternKey("shapeA", paths); + TripPatternKey a = new TripPatternKey("shapeA", Arrays.asList( + stopPath(1, "p1", "s1", 0, "r1"), + stopPath(1, "p2", "s2", 1, "r1"))); + TripPatternKey b = new TripPatternKey("shapeA", Arrays.asList( + stopPath(1, "p1", "s1", 0, "r1"), + stopPath(1, "p2", "s2", 1, "r1")));Also applies to: 151-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.java` around lines 60 - 65, The test currently reuses the same List instance `paths` for both TripPatternKey objects which hides identity-based bugs; update the test(s) to construct independent but equal lists/objects (e.g., create a second List with newly instantiated StopPath objects that have the same values) before creating the second TripPatternKey so equality/hashCode are validated logically; apply the same change to the similar assertions around the other occurrence (the test near the block that corresponds to lines 151-156) referencing the same classes `TripPatternKey` and `StopPath`.transitclock/src/test/java/org/transitclock/core/VehicleAtStopInfoTest.java (1)
45-86: Comprehensive equality testing with good coverage of edge cases.The tests correctly verify:
- getClass-based equality prevents VehicleAtStopInfo from equaling plain Indices
- Both directions of inequality (lines 63-64)
- Field-specific inequality conditions
♻️ Optional: Consider using assertEquals consistently for equality assertions
For consistency, you could use
assertEqualsthroughout instead of mixing withassertTrue(x.equals(y)):public void twoVehicleAtStopInfosWithSameFieldsAreEqual() { VehicleAtStopInfo a = new VehicleAtStopInfo(null, 1, 2); VehicleAtStopInfo b = new VehicleAtStopInfo(null, 1, 2); - assertTrue(a.equals(b)); + assertEquals(a, b); }Both approaches are valid; this just improves consistency with line 52.
🤖 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/VehicleAtStopInfoTest.java` around lines 45 - 86, Change the equality assertion in twoVehicleAtStopInfosWithSameFieldsAreEqual to use assertEquals for consistency: replace the assertTrue(a.equals(b)) call in that test with assertEquals(a, b) (keeping the existing variable setup in VehicleAtStopInfo a and b) so all equality checks in this class use the same assertEquals style.transitclockPipelineTests/pom.xml (1)
71-74: Upgrade maven-surefire-plugin from 2.19.1 to 3.5.5 or later.The 2.19.1 release is outdated for JDK 17. All surefire 3.x versions officially support Java 17, with 3.5.5 (latest stable) providing improved reliability, better test-platform compatibility, and full JUnit 4 support via the built-in surefire-junit4 provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclockPipelineTests/pom.xml` around lines 71 - 74, Update the maven-surefire-plugin version to a JDK17-compatible release by changing the plugin declaration for org.apache.maven.plugins:maven-surefire-plugin (the <version> element currently set to 2.19.1) to 3.5.5 (or later); ensure the <configuration> remains valid for the 3.x provider and adjust any deprecated configuration keys if build warnings appear after the upgrade (target the plugin coordinates groupId org.apache.maven.plugins and artifactId maven-surefire-plugin).transitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarness.java (1)
210-212: Temp directory contents may not be cleaned up.
deleteOnExit()only deletes the directory itself (and only if empty). The GTFS files copied intostagedwill persist until JVM exit and won't be automatically removed, potentially leaving orphaned files in the temp directory if the JVM crashes or if many test runs accumulate.Consider using a shutdown hook or try-with-resources pattern with explicit cleanup, or accept this as a minor tradeoff for test simplicity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarness.java` around lines 210 - 212, The temp directory created via Files.createTempDirectory and stored in the staged Path in CoreHarness is only registered with staged.toFile().deleteOnExit(), which won't remove non-empty directories; change to explicitly clean up the directory contents either by registering a JVM shutdown hook that recursively deletes staged (including files) or by ensuring tests use a try/finally (or try-with-resources style wrapper) that recursively deletes staged after use; locate the staged variable and replace the single deleteOnExit() call with a recursive delete routine invoked from a shutdown hook or final cleanup block to ensure files copied into staged are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 37: The opening sentence "Eight Maven modules under the root aggregator
`pom.xml`" is incorrect relative to the bullets that enumerate nine modules;
update that sentence to match the list (e.g., change to "Nine Maven modules
under the root aggregator `pom.xml`" or rephrase to "Multiple Maven modules
across default and opt-in profiles under the root aggregator `pom.xml`") so the
summary aligns with the enumerated bullets. Ensure you edit the exact phrase
"Eight Maven modules under the root aggregator `pom.xml`" to the chosen
corrected wording.
In
`@transitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.java`:
- Around line 117-125: The test assertions contradict
TripPatternKey.equals/hashCode (which compares shapeId and ordered
StopPath#getStopPathIndex() values), so update the tests rather than production
code: in TripPatternKeyTest methods like differentStopIdBreaksEquality (and the
similar one at 128-134) either change assertNotEquals(a, b) to assertEquals(a,
b) to reflect that differing StopPath.stopId/configRev does not affect equality,
or alter the StopPath fixtures to produce different stopPathIndex values (via
stopPath(...) inputs) so equality should legitimately differ; reference
TripPatternKey.equals/hashCode and StopPath#getStopPathIndex() when making the
change.
In `@transitclock/src/test/java/org/transitclock/testutil/TransitFixtures.java`:
- Around line 115-129: In blockOf (TransitFixtures.java) the current
startTime/endTime computation can return Integer.MIN_VALUE or drop valid end
times; change the post-loop logic to handle partial times: after scanning
tripList (using startTime, endTime), if both bounds stayed at their sentinels
set both to 0; if only startTime is set (endTime still MIN_VALUE) set endTime =
startTime; if only endTime is set (startTime still MAX_VALUE) set startTime =
endTime; keep the existing sentinel initializations and ensure the final new
Block(DEFAULT_CONFIG_REV, blockId, serviceId, startTime, endTime, tripList)
receives valid bounds.
In `@transitclock/src/test/java/org/transitclock/utils/IntervalTimerTest.java`:
- Around line 63-81: The tests use Double.parseDouble on the locale-sensitive
string returned by IntervalTimer.elapsedMsecStr() (and
IntervalTimer.toString()), which fails for locales that use commas; update the
tests elapsedMsecStrParsesAsNonNegativeNumber and
toStringReturnsSameAsElapsedMsecStr to parse using
NumberFormat.getNumberInstance() (or NumberFormat.getInstance()) and
NumberFormat.parse(...) to obtain a Number and then compare its doubleValue(),
ensuring locale-aware parsing for both timer.elapsedMsecStr() and
timer.toString() and keeping the same non-negative and delta assertions.
In `@transitclockPipelineTests/pom.xml`:
- Around line 6-10: This module POM (artifactId transitclockPipelineTests) is
missing a <parent> declaration so it doesn't inherit shared build/plugins
(including JaCoCo/reporting); add a <parent> element that references the root
project's groupId, artifactId and version and set relativePath to ../pom.xml so
the module inherits the root POM's build and reporting configuration (thereby
enabling JaCoCo settings for transitclockPipelineTests).
- Around line 38-64: Update the test dependency versions in the pom.xml to
address known CVEs: bump org.hsqldb:hsqldb from 2.2.4 to at least 2.7.1,
ch.qos.logback:logback-classic from 1.1.3 to 1.2.0+, junit:junit from 4.11 to
4.13.1+, and org.assertj:assertj-core from 3.24.2 down/side-step to a safe
patched line (use 3.17.0+ as recommended); locate and modify the <dependency>
blocks for artifactIds hsqldb, logback-classic, junit and assertj-core in the
file and update the <version> elements accordingly, then run your build/CI to
verify tests still pass under Java 17.
In
`@transitclockPipelineTests/src/test/resources/hsql_pipeline_test_hibernate.cfg.xml`:
- Around line 79-84: Update the HSQLDB JDBC URLs for the properties
hibernate.connection.url and hibernate.ro.connection.url to use the correct
in-memory format by removing the "//localhost/" segment; change the values from
"jdbc:hsqldb:mem://localhost/xdb" to the standard "jdbc:hsqldb:mem:xdb" so the
database initializes correctly.
In `@transitclockPipelineTests/src/test/resources/transitclockConfigHsql.xml`:
- Around line 28-32: Fix the typo in the XML comment that currently reads
"VechiclesServer" — change it to "VehiclesServer"; locate the comment near the
<dwelltime> and <fuzzytime> entries (the comment referencing matching the last
stop of a vehicle to a time) and update the text only.
---
Nitpick comments:
In `@transitclock/src/test/java/org/transitclock/core/VehicleAtStopInfoTest.java`:
- Around line 45-86: Change the equality assertion in
twoVehicleAtStopInfosWithSameFieldsAreEqual to use assertEquals for consistency:
replace the assertTrue(a.equals(b)) call in that test with assertEquals(a, b)
(keeping the existing variable setup in VehicleAtStopInfo a and b) so all
equality checks in this class use the same assertEquals style.
In
`@transitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.java`:
- Around line 59-60: The test currently uses identity assertion assertSame(avl,
a.getAvlTime()) which ties behavior to object identity; change this to a value
assertion (e.g., assertEquals(avl, a.getAvlTime())) so Date equality is checked
rather than reference equality, and apply the same change for the other Date
assertions in the test (those around the 84-89 block) to avoid preventing
defensive-copy implementations.
- Around line 122-126: The test in ArrivalDepartureSubclassesTest creates
Arrival instances using the same Date object references (t and avl) so equality
may pass via reference identity; instead instantiate distinct Date objects with
identical time values (e.g., new Date(time(...).getTime()) or use time(...)
twice into separate variables) when constructing Arrival a and Arrival b so both
have equal but not identical Date instances; update the lines that set Date t
and avl and/or the second Arrival constructor call to use those distinct Date
objects while keeping the same millisecond values.
- Around line 14-17: The class Javadoc claims withUpdatedTime is tested but no
such test exists; add a unit test in ArrivalDepartureSubclassesTest that
constructs an Arrival (and optionally a Departure), calls
Arrival.withUpdatedTime(newTime) and asserts the returned object has the updated
time (via getTime/getTimestamp), retains other fields unchanged, and still
reports isArrival true (and similarly verify Departure behavior if applicable),
referencing the Arrival class and its withUpdatedTime method as well as
isArrival/isDeparture and getter methods to locate code to assert against.
In
`@transitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.java`:
- Around line 60-65: The test currently reuses the same List instance `paths`
for both TripPatternKey objects which hides identity-based bugs; update the
test(s) to construct independent but equal lists/objects (e.g., create a second
List with newly instantiated StopPath objects that have the same values) before
creating the second TripPatternKey so equality/hashCode are validated logically;
apply the same change to the similar assertions around the other occurrence (the
test near the block that corresponds to lines 151-156) referencing the same
classes `TripPatternKey` and `StopPath`.
In `@transitclock/src/test/java/org/transitclock/utils/ChinaGpsOffsetTest.java`:
- Around line 18-38: Add explicit boundary-value assertions for
ChinaGpsOffset.outOfChina to protect against off-by-one regressions: call
outOfChina with the exact longitude thresholds 72.004 and 137.8347 (e.g.,
outOfChina(<anyLatWithinRange>, 72.004) and 137.8347) and assertFalse, and call
outOfChina with the exact latitude thresholds 0.8293 and 55.8271 (e.g.,
outOfChina(0.8293, <anyLonWithinRange>) and 55.8271) and assertFalse; also
consider adding one test just inside and one just outside each threshold (e.g.,
72.0039 and 72.0041) to verify the strict comparison behavior in
ChinaGpsOffset.outOfChina.
In `@transitclock/src/test/java/org/transitclock/utils/EncryptionTest.java`:
- Around line 54-79: Extract the repeated try/catch/fail pattern in
decryptOfTamperedCiphertextThrows and decryptOfGarbageThrows into a small test
helper (e.g., assertDecryptFails(String ciphertext)) that calls
Encryption.decrypt(ciphertext) and fails the test unless an
EncryptionOperationNotPossibleException is thrown; then replace the try/catch
blocks in both tests with calls to assertDecryptFails("not-a-real-ciphertext")
and assertDecryptFails(tampered) to remove duplication and keep tests focused on
inputs.
- Around line 43-52: Test currently only compares Encryption.decrypt(a) to
Encryption.decrypt(b); strengthen it by asserting both decryptions equal the
original plaintext "sameInput". In the test method
encryptionIsSaltedSoSameInputProducesDifferentCiphertexts, replace or supplement
the existing assertEquals(Encryption.decrypt(a), Encryption.decrypt(b)) with two
assertions that assertEquals("sameInput", Encryption.decrypt(a)) and
assertEquals("sameInput", Encryption.decrypt(b)) so both decrypted values are
explicitly validated against the original input while keeping the
assertNotEquals(a, b) check for differing ciphertexts.
In `@transitclockPipelineTests/pom.xml`:
- Around line 71-74: Update the maven-surefire-plugin version to a
JDK17-compatible release by changing the plugin declaration for
org.apache.maven.plugins:maven-surefire-plugin (the <version> element currently
set to 2.19.1) to 3.5.5 (or later); ensure the <configuration> remains valid for
the 3.x provider and adjust any deprecated configuration keys if build warnings
appear after the upgrade (target the plugin coordinates groupId
org.apache.maven.plugins and artifactId maven-surefire-plugin).
In
`@transitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarness.java`:
- Around line 210-212: The temp directory created via Files.createTempDirectory
and stored in the staged Path in CoreHarness is only registered with
staged.toFile().deleteOnExit(), which won't remove non-empty directories; change
to explicitly clean up the directory contents either by registering a JVM
shutdown hook that recursively deletes staged (including files) or by ensuring
tests use a try/finally (or try-with-resources style wrapper) that recursively
deletes staged after use; locate the staged variable and replace the single
deleteOnExit() call with a recursive delete routine invoked from a shutdown hook
or final cleanup block to ensure files copied into staged are removed.
🪄 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: b80bb7f2-360c-4206-8427-151a1ba20535
📒 Files selected for processing (26)
.github/workflows/ci.yml.gitignoreCLAUDE.mdREADME.mdpom.xmltransitclock/src/test/java/org/transitclock/core/VehicleAtStopInfoTest.javatransitclock/src/test/java/org/transitclock/db/structs/ArrivalDepartureSubclassesTest.javatransitclock/src/test/java/org/transitclock/db/structs/BlockTest.javatransitclock/src/test/java/org/transitclock/db/structs/StopPathExtendedTest.javatransitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.javatransitclock/src/test/java/org/transitclock/db/structs/TripTest.javatransitclock/src/test/java/org/transitclock/testutil/TransitFixtures.javatransitclock/src/test/java/org/transitclock/utils/ChinaGpsOffsetTest.javatransitclock/src/test/java/org/transitclock/utils/EncryptionTest.javatransitclock/src/test/java/org/transitclock/utils/IntervalTimerTest.javatransitclock/src/test/java/org/transitclock/utils/MapKeyTest.javatransitclock/src/test/java/org/transitclock/utils/OrderedCollectionTest.javatransitclockPipelineTests/.gitignoretransitclockPipelineTests/pom.xmltransitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/AvlProcessorBehaviorTest.javatransitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarness.javatransitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarnessCalendarRewriteTest.javatransitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/CoreHarnessSmokeTest.javatransitclockPipelineTests/src/test/resources/hsql_pipeline_test_hibernate.cfg.xmltransitclockPipelineTests/src/test/resources/logback-test.xmltransitclockPipelineTests/src/test/resources/transitclockConfigHsql.xml
Comment on lines
+117
to
+125
| public void differentStopIdBreaksEquality() { | ||
| StopPath p1 = stopPath(1, "p1", "s1", 0, "r1"); | ||
| StopPath p1DiffStop = stopPath(1, "p1", "sX", 0, "r1"); | ||
| TripPatternKey a = new TripPatternKey("shapeA", | ||
| Collections.singletonList(p1)); | ||
| TripPatternKey b = new TripPatternKey("shapeA", | ||
| Collections.singletonList(p1DiffStop)); | ||
| assertNotEquals(a, b); | ||
| } |
There was a problem hiding this comment.
Assertions here contradict TripPatternKey.equals/hashCode and will fail
These tests assume stopId and configRev affect key equality, but TripPatternKey currently compares shapeId plus ordered StopPath#getStopPathIndex() values (see transitclock/src/main/java/org/transitclock/db/structs/TripPatternKey.java, Lines 86-140).
Please align contract and tests: either update TripPatternKey semantics or change these assertions to match current behavior.
Also applies to: 128-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@transitclock/src/test/java/org/transitclock/db/structs/TripPatternKeyTest.java`
around lines 117 - 125, The test assertions contradict
TripPatternKey.equals/hashCode (which compares shapeId and ordered
StopPath#getStopPathIndex() values), so update the tests rather than production
code: in TripPatternKeyTest methods like differentStopIdBreaksEquality (and the
similar one at 128-134) either change assertNotEquals(a, b) to assertEquals(a,
b) to reflect that differing StopPath.stopId/configRev does not affect equality,
or alter the StopPath fixtures to produce different stopPathIndex values (via
stopPath(...) inputs) so equality should legitimately differ; reference
TripPatternKey.equals/hashCode and StopPath#getStopPathIndex() when making the
change.
Comment on lines
+38
to
+64
| <dependency> | ||
| <groupId>org.hsqldb</groupId> | ||
| <artifactId>hsqldb</artifactId> | ||
| <version>2.2.4</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>ch.qos.logback</groupId> | ||
| <artifactId>logback-classic</artifactId> | ||
| <!-- Matches transitclockCore's logback-classic version to avoid | ||
| classpath surprises during test runs. --> | ||
| <version>1.1.3</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <version>4.11</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <version>3.24.2</version> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Maven artifacts org.hsqldb:hsqldb, ch.qos.logback:logback-classic, org.assertj:assertj-core, and junit:junit, list current GHSA/CVE advisories and the first patched versions compatible with Java 17.
💡 Result:
org.hsqldb:hsqldb - GHSA/CVE: GHSA-77xx-rxvh-q682 / CVE-2022-41853 (Remote Code Execution via untrusted input in Statement/PreparedStatement) - First patched version compatible with Java 17: 2.7.1 (main jar requires JRE 11+, tested up to Java 17; current latest 2.7.4) ch.qos.logback:logback-classic - GHSA/CVE: GHSA-25qh-j22f-pwp8 / CVE-2025-11226 (Arbitrary Code Execution in conditional config file processing, requires Janino+Spring); other recent CVEs like CVE-2024-12798, CVE-2024-12801 backported to 1.3.x, CVE-2026-1225 fixed in 1.5.25 - First patched version compatible with Java 17: 1.2.0 (all modern versions 1.3+ require JDK8+, 1.5+ JDK11+ built/tested on JDK21 but run on 17; current latest 1.5.32) org.assertj:assertj-core - GHSA/CVE: GHSA-rqfh-9r24-8c9r / CVE-2026-24400 (XXE in isXmlEqualTo assertion via XmlStringPrettyFormatter with untrusted XML) - First patched version compatible with Java 17: 3.17.0 (dropped Java 8 support; current latest 3.27.7) junit:junit (JUnit 4) - GHSA/CVE: GHSA-269g-pwp5-87pp / CVE-2020-15250 (TemporaryFolder JUnit rule creates world-readable files on Unix-like systems) - First patched version compatible with Java 17: 4.13.1 (all JUnit 4 versions compatible with Java 17; current latest 4.13.2)
Citations:
- 1: https://security.snyk.io/vuln/SNYK-JAVA-ORGHSQLDB-3040860
- 2: https://osv.dev/vulnerability/GHSA-77xx-rxvh-q682
- 3: GHSA-rqfh-9r24-8c9r
- 4: https://nvd.nist.gov/vuln/detail/CVE-2026-24400
- 5: https://hsqldb.org/doc/2.0/changelist_2_0.txt
- 6: https://versionlog.com/hsqldb/
- 7: https://github.com/ryenus/hsqldb/blob/f124cd92/doc-src/changelist_2_0.txt
- 8: https://www.hsqldb.org/index.html
- 9: GHSA-25qh-j22f-pwp8
- 10: https://github.com/qos-ch/logback/releases/tag/v_1.5.25
- 11: https://advisories.gitlab.com/pkg/maven/ch.qos.logback/logback-core/CVE-2025-11226/
- 12: https://mvnrepository.com/artifact/ch.qos.logback/logback-classic
- 13: https://logback.qos.ch/download.html
- 14: https://security.snyk.io/package/maven/org.assertj%3Aassertj-core
- 15: https://mvnrepository.com/artifact/org.hsqldb/hsqldb/2.7.0
- 16: https://repo1.maven.org/maven2/org/hsqldb/hsqldb/
- 17: https://github.com/junit-team/junit-framework/security
- 18: CVE-2020-15250 doesn't affect versions prior to 4.7 but claims it did junit-team/junit4#1676
- 19: https://github.com/junit-team/junit4/security
🏁 Script executed:
#!/bin/bash
# First, locate and read the specific pom.xml file
fd -t f "pom.xml" | head -20Repository: OneBusAway/thetransitclock
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Read the transitclockPipelineTests/pom.xml file
cat -n transitclockPipelineTests/pom.xml | head -100Repository: OneBusAway/thetransitclock
Length of output: 4085
🏁 Script executed:
#!/bin/bash
# Check the root pom for Java version target
cat -n pom.xml | grep -A 5 -B 5 "java\|Java\|source\|target" | head -50Repository: OneBusAway/thetransitclock
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Broader search for Java version in root pom
cat pom.xml | grep -i "java\|source\|target\|release" | head -30Repository: OneBusAway/thetransitclock
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check transitclock (core) pom for Java version and logback version
cat transitclock/pom.xml | grep -A 2 -B 2 "java\|Java\|logback" | head -80Repository: OneBusAway/thetransitclock
Length of output: 2463
🏁 Script executed:
#!/bin/bash
# Search for maven-compiler-plugin configuration in root pom
cat pom.xml | grep -A 10 "maven-compiler-plugin"Repository: OneBusAway/thetransitclock
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if there's a parent pom reference and look for full <properties> section in root
cat pom.xml | head -150Repository: OneBusAway/thetransitclock
Length of output: 2604
🏁 Script executed:
#!/bin/bash
# Read the full transitclock/pom.xml to check for Java version and compiler settings
wc -l transitclock/pom.xmlRepository: OneBusAway/thetransitclock
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Get sections with build, properties, and compiler from transitclock/pom.xml
cat transitclock/pom.xml | grep -A 20 "<build>" | head -40Repository: OneBusAway/thetransitclock
Length of output: 1200
Upgrade vulnerable test dependencies before merge.
The pinned versions are affected by known security advisories that impact CI and developer environments despite test scope:
hsqldb2.2.4 → upgrade to 2.7.1+ (RCE via CVE-2022-41853)logback-classic1.1.3 → upgrade to 1.2.0+ (Arbitrary Code Execution via CVE-2025-11226)junit4.11 → upgrade to 4.13.1+ (file permission exposure via CVE-2020-15250)assertj-core3.24.2 → upgrade to 3.17.0+ (XXE via CVE-2026-24400)
All patched versions are compatible with Java 17 (the project's target).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@transitclockPipelineTests/pom.xml` around lines 38 - 64, Update the test
dependency versions in the pom.xml to address known CVEs: bump org.hsqldb:hsqldb
from 2.2.4 to at least 2.7.1, ch.qos.logback:logback-classic from 1.1.3 to
1.2.0+, junit:junit from 4.11 to 4.13.1+, and org.assertj:assertj-core from
3.24.2 down/side-step to a safe patched line (use 3.17.0+ as recommended);
locate and modify the <dependency> blocks for artifactIds hsqldb,
logback-classic, junit and assertj-core in the file and update the <version>
elements accordingly, then run your build/CI to verify tests still pass under
Java 17.
3 tasks
- Fix module count in CLAUDE.md (nine, not eight).
- Fix HSQLDB JDBC URL format to the standard jdbc:hsqldb:mem:<name>.
- Fix VechiclesServer typo in transitclockConfigHsql.xml.
- Inherit root parent pom in transitclockPipelineTests so JaCoCo and other
shared plugins apply. Preserve the jacoco-set argLine via @{argLine} so
the agent isn't dropped when we add -Dtransitclock.logging.dir.
- Bump junit (4.11 -> 4.13.2) and assertj-core (3.24.2 -> 3.26.3) in
transitclockPipelineTests to match transitclockCore. Left hsqldb and
logback-classic deliberately aligned with core per the explicit comment,
since diverging risks classpath surprises when the pipeline tests boot a
real Core.
- Handle partial trip times in TransitFixtures.blockOf — previously a trip
with only a start or only an end could leave startTime or endTime at
Integer.MAX_VALUE / Integer.MIN_VALUE, or a present end could be reset
to zero.
- Parse IntervalTimer.elapsedMsecStr()/toString() via NumberFormat so the
tests don't fail on JVMs whose default locale uses a comma decimal
separator.
3 tasks
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 by CodeRabbit
New Features
Documentation
Tests
Chores