Merge pull request #6 from OneBusAway/di-seam

more test coverage

Author: aaronbrethorst

README.md

@@ -1,15 +1,23 @@
-core [![Build Status](https://travis-ci.org/TheTransitClock/transitime.svg?branch=develop)](https://travis-ci.org/TheTransitClock/transitime)
+TheTransitClock
 ====
 
-The complete core Java software for the Transitime real-time transit information project. The goal is to use any type of real-time GPS data to generate useful public transportation information. The system is for both letting passengers know the status of their vehicles and helping agencies more effectively manage their systems. By providing a complete open-source system, agencies can have a cost effective system and have full ownership of it. 
+This is a fork of TheTransitClock, a [GTFS-RT Trip Updates](https://gtfs.org/documentation/realtime/feed-entities/trip-updates/) generation engine used by public transit agencies around the world, including in Minneapolis, MN where [the software was found to outperform proprietary alternatives](http://berrebi.net/wp-content/uploads/2020/01/trbws07_FileUploads_2020-AM-Presentations_3470_pdf_13557_P20-20421_2020-01-14-09-24-54.pdf).
 
-<b>Build</b>
+[TheTransitClock](https://thetransitclock.github.io) was developed by Sean Óg Crudden and Simon J. Berrebi, Ph.D., itself a fork of [Swiftly Transitime](https://transitime.github.io/core/).
 
-The software is made up of three modules which can each be built with maven. See BUILD.md
+## About this Repo
+
+The complete core Java software for the Transitime real-time transit information project. The purpose of the software is to use any type of real-time GPS data to generate useful public transportation information, namely a GTFS-RT Trip Updates feed.
+
+The system is for both letting passengers know the status of their vehicles and helping agencies more effectively manage their systems. By providing a complete open-source system, agencies can have a cost effective system and have full ownership of it. 
+
+## Build
+
+The software is made up of three modules which can each be built with maven. See [BUILD.md](./BUILD.md).
 
 The core functionality is in the transitime project. The REST api is in transitimeApi and the user Web applicaton is in transitimeWebapp.
 
-<b>Setup</b>
+## Setup
 
 The main module is transitTime. This has several standalone programs in the org.transitime.applications package.
 
@@ -29,12 +37,14 @@ The transitimeWebapp in turn is a web application which uses the transitTimeAPI
 
 The transitimeQuickStart can be built with mvn install and ran using java -jar transitimeQuickStart it is currently a work in progress but the gui elements can be seen.
 
-<b>Running tests</b>
+## Running tests
 
-- Default unit tests across all modules: `mvn verify` (not `mvn test` — see CLAUDE.md for why).
+- Default unit tests across all modules: `mvn verify` (not `mvn test` — see below).
 - Single module: `mvn -pl transitclock test`
 - Single class: `mvn -pl transitclock test -Dtest=TestAPIKeyManager`
 
+`mvn test` on the full reactor fails because `transitclockQuickStart` binds `maven-dependency-plugin:copy` to `generate-resources` to pull the `transitclockApi` WAR into its resources, but the `test` phase never packages that WAR (MDEP-187: "Artifact has not been packaged yet"). Use `mvn verify` / `mvn package` / `mvn install` to exercise all tests, or scope to a single module with `-pl`, or skip QuickStart with `mvn test -pl '!transitclockQuickStart'`.
+
 Two additional test suites are opt-in via Maven profiles and excluded from the default build:
 
 - **Pipeline tests** (`transitclockPipelineTests`) — boot a real Core against an in-memory HSQL database populated with a small WMATA GTFS fixture, then exercise matcher / generator behavior end-to-end. Run with:
@@ -47,13 +57,10 @@ Two additional test suites are opt-in via Maven profiles and excluded from the d
   mvn install -P include-integration-tests
   ```
 
-<b>Code coverage</b>
+### Code coverage
 
 JaCoCo generates coverage reports as part of the Maven `verify` phase.
 
 - Per-module HTML reports: `<module>/target/site/jacoco/index.html`
 - Aggregate report across Core + thin clients: `coverage-report/target/site/jacoco-aggregate/index.html`
 - Regenerate just the aggregate (fastest): `mvn verify -pl coverage-report -am`
-
-[![Build Status](https://zenodo.org/badge/DOI/10.5281/zenodo.3550975.svg)](https://zenodo.org/record/3550975#.XdgmVedKjOQ)
-

transitclock/src/main/java/org/transitclock/core/holdingmethod/HoldingTimeGeneratorDefaultImpl.java

@@ -67,6 +67,19 @@ public class HoldingTimeGeneratorDefaultImpl implements HoldingTimeGenerator {
 	protected static IntegerConfigValue  plannedHeadwayMsec = new IntegerConfigValue("transitclock.holding.plannedHeadwayMsec", 60*1000*9, "Planned Headway");
 	protected static StringListConfigValue controlStopList = new StringListConfigValue("transitclock.holding.controlStops", null, "This is a list of stops to generate holding times for.");
 
+	public HoldingTimeGeneratorDefaultImpl() {
+		// Operator opted into this generator via transitclock.core.holdingTimeGeneratorClass
+		// but left controlStops unset; the feature will produce no holding times until
+		// transitclock.holding.controlStops is configured. Surfaced once here because the
+		// per-event callers (isControlStop, handleDeparture) would flood the logs.
+		if (controlStopList.getValue() == null) {
+			logger.warn(
+					"HoldingTimeGeneratorDefaultImpl is active but transitclock.holding.controlStops is unset — "
+							+ "no holding times will be produced. Configure a stop-id list, or clear "
+							+ "transitclock.core.holdingTimeGeneratorClass to silence this warning.");
+		}
+	}
+
 	public HoldingTime generateHoldingTime(VehicleState vehicleState, IpcArrivalDeparture event) {
 
 		PredictionDataCache predictionCache = PredictionDataCache.getInstance();
@@ -659,7 +672,16 @@ public List<ControlStop> getControlPointStops() {
 
 		ArrayList<ControlStop> controlStops=new ArrayList<ControlStop>();
 
-		for(String stopEntry: controlStopList.getValue())
+		// Treat an unset controlStops list as "no control stops configured" — the
+		// feature is a no-op in that case. Without this guard, every caller of
+		// getControlPointStops (isControlStop, handleDeparture, generateHoldingTime)
+		// would NPE iterating a null list.
+		List<String> configured = controlStopList.getValue();
+		if (configured == null) {
+			return controlStops;
+		}
+
+		for(String stopEntry: configured)
 		{
 			controlStops.add(new ControlStop(stopEntry));
 		}
@@ -669,16 +691,16 @@ public List<ControlStop> getControlPointStops() {
 
 	private Long[] predictionsToLongArray(List<IpcPrediction> predictions)
 	{
+		if(predictions==null)
+		{
+			return new Long[0];
+		}
 		Long[] list=new Long[predictions.size()];
-
-		if(predictions!=null)
+		int i=0;
+		for(IpcPrediction prediction:predictions)
 		{
-			int i=0;
-			for(IpcPrediction prediction:predictions)
-			{
-				list[i]=new Long(prediction.getPredictionTime());
-				i++;
-			}
+			list[i]=new Long(prediction.getPredictionTime());
+			i++;
 		}
 		return list;
 	}
@@ -698,14 +720,11 @@ ArrayList<Long> predictionsToLongArrayList(List<IpcPrediction> predictions, Arra
 	private boolean isControlStop(String stopId)
 	{
 		ControlStop controlStop=new ControlStop( null, stopId);
-		if(getControlPointStops()!=null)
+		for(ControlStop controlStopInList:getControlPointStops())
 		{
-			for(ControlStop controlStopInList:getControlPointStops())
+			if(controlStopInList.getStopId().equals(controlStop.getStopId()))
 			{
-				if(controlStopInList.getStopId().equals(controlStop.getStopId()))
-				{
-					return true;
-				}
+				return true;
 			}
 		}
 		return false;

transitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/AvlProcessorBehaviorTest.java

@@ -115,18 +115,6 @@ private long advanceClockBy(long deltaMs) {
 		return next;
 	}
 
-	/**
-	 * Jumps the harness clock to a specific epoch (for tests that need a
-	 * real-world date, e.g. a known active service day). Updates
-	 * {@link #nextTime} to the larger of its current value and the jump
-	 * target, so subsequent tests' {@code @Before} advances never rewind
-	 * the clock.
-	 */
-	private void jumpClockTo(long epochMs) {
-		CORE.setNow(epochMs);
-		nextTime.updateAndGet(cur -> Math.max(cur, epochMs));
-	}
-
 	private static AvlReport avlReport(String vehicleId, double lat, double lon) {
 		// Use CORE.clock() so the report's time advances with setNow(). Tests
 		// that want a time offset from "now" can construct their own Date.
@@ -140,6 +128,30 @@ private static AvlReport avlReport(String vehicleId, double lat, double lon) {
 		return report;
 	}
 
+	/**
+	 * Pins Core's clock to an epoch within the known-good SE-08 service window
+	 * that is strictly greater than any timestamp prior tests already used.
+	 * Multiple happy-path tests in the same JVM would otherwise all pin the
+	 * same {@code HAPPY_PATH_EPOCH_MS}; the second test's AVL report would fail
+	 * {@code setLastAvlReport}'s "only store newer" guard and be silently
+	 * dropped.
+	 */
+	private long pinClockToHappyPathAnchor() {
+		long anchor = nextTime.updateAndGet(
+				cur -> Math.max(cur, HAPPY_PATH_EPOCH_MS) + 100L);
+		CORE.setNow(anchor);
+		return anchor;
+	}
+
+	private static AvlReport reportAtFirstStop(String vehicleId, long epochMs,
+			String assignmentId, AssignmentType assignmentType) {
+		AvlReport report = new AvlReport(vehicleId, epochMs,
+				HAPPY_PATH_LAT, HAPPY_PATH_LON,
+				Float.NaN, Float.NaN, "test");
+		report.setAssignment(assignmentId, assignmentType);
+		return report;
+	}
+
 	// ---------- Tests ----------
 
 	@Test
@@ -233,9 +245,9 @@ public void reportAtFirstStopOfActiveBlockProducesPredictableVehicle() {
 		// it starts failing, the matching pipeline has regressed and the
 		// rest of the suite — all of which asserts !isPredictable — would
 		// silently approve broken code.
-		jumpClockTo(HAPPY_PATH_EPOCH_MS);
-		AvlReport report = avlReport("v-happy", HAPPY_PATH_LAT, HAPPY_PATH_LON);
-		report.setAssignment(HAPPY_PATH_BLOCK_ID, AssignmentType.BLOCK_ID);
+		long when = pinClockToHappyPathAnchor();
+		AvlReport report = reportAtFirstStop("v-happy", when,
+				HAPPY_PATH_BLOCK_ID, AssignmentType.BLOCK_ID);
 
 		AvlProcessor.getInstance().processAvlReport(report);
 
@@ -249,4 +261,105 @@ public void reportAtFirstStopOfActiveBlockProducesPredictableVehicle() {
 				.isNotNull();
 		assertThat(state.getAssignmentId()).isEqualTo(HAPPY_PATH_BLOCK_ID);
 	}
+
+	@Test
+	public void reportWithTripIdAssignmentProducesPredictableVehicle() {
+		long when = pinClockToHappyPathAnchor();
+		AvlReport report = reportAtFirstStop("v-happy-tid", when,
+				"868588900", AssignmentType.TRIP_ID);
+
+		AvlProcessor.getInstance().processAvlReport(report);
+
+		VehicleState state = VehicleStateManager.getInstance().getVehicleState("v-happy-tid");
+		assertThat(state).isNotNull();
+		assertThat(state.isPredictable())
+				.as("vehicle with TRIP_ID assignment on an active block should be predictable")
+				.isTrue();
+		assertThat(state.getMatch())
+				.as("TRIP_ID-predictable vehicle must have a concrete TemporalMatch")
+				.isNotNull();
+		// The assignment-id on the state carries through as the block id
+		// once BlockAssigner resolves the trip, not the raw TRIP_ID.
+		assertThat(state.getAssignmentId()).isEqualTo(HAPPY_PATH_BLOCK_ID);
+	}
+
+	@Test
+	public void cacheAvlReportWithoutProcessingUpdatesStateButDoesNotMatch() {
+		long when = advanceClockBy(1_000L);
+		AvlReport report = avlReport("v-cache-only", NEAR_ROUTE_LAT, NEAR_ROUTE_LON);
+
+		AvlProcessor.getInstance().cacheAvlReportWithoutProcessing(report);
+
+		VehicleState state = VehicleStateManager.getInstance().getVehicleState("v-cache-only");
+		assertThat(state).isNotNull();
+		assertThat(state.getAvlReport())
+				.as("cached report should be attached to the vehicle state")
+				.isNotNull();
+		assertThat(state.getAvlReport().getVehicleId()).isEqualTo("v-cache-only");
+		assertThat(state.getAvlReport().getTime()).isEqualTo(when);
+		assertThat(state.isPredictable())
+				.as("cacheAvlReportWithoutProcessing must never attempt matching")
+				.isFalse();
+		assertThat(state.getMatch())
+				.as("no match should be produced when skipping processing")
+				.isNull();
+	}
+
+	@Test
+	public void makeVehicleUnpredictableClearsMatchOnPredictableVehicle() {
+		long when = pinClockToHappyPathAnchor();
+		AvlReport report = reportAtFirstStop("v-unwind", when,
+				HAPPY_PATH_BLOCK_ID, AssignmentType.BLOCK_ID);
+		AvlProcessor.getInstance().processAvlReport(report);
+		VehicleState beforeUnwind =
+				VehicleStateManager.getInstance().getVehicleState("v-unwind");
+		assertThat(beforeUnwind.isPredictable())
+				.as("setup assumption: happy path must produce a predictable vehicle")
+				.isTrue();
+
+		AvlProcessor.getInstance().makeVehicleUnpredictable(
+				"v-unwind",
+				"behavior-test-triggered unwind",
+				org.transitclock.db.structs.VehicleEvent.ASSIGNMENT_CHANGED);
+
+		VehicleState afterUnwind =
+				VehicleStateManager.getInstance().getVehicleState("v-unwind");
+		assertThat(afterUnwind)
+				.as("VehicleStateManager should still yield the same state object")
+				.isSameAs(beforeUnwind);
+		assertThat(afterUnwind.isPredictable())
+				.as("vehicle must be unpredictable after makeVehicleUnpredictable")
+				.isFalse();
+		assertThat(afterUnwind.getMatch())
+				.as("match must be cleared after makeVehicleUnpredictable")
+				.isNull();
+	}
+
+	// Schedule-based-predictions AVL reports are synthetic — they exist
+	// to produce predictions for runs with no real vehicle assigned. They
+	// must NOT update lastRegularReportProcessed, because that timestamp
+	// drives the "AVL feed is up" monitoring check.
+	@Test
+	public void schedBasedPredsReportDoesNotUpdateLastRegularReport() {
+		long baselineEpoch = advanceClockBy(1_000L);
+		AvlReport baseline = avlReport("v-regular", NEAR_ROUTE_LAT, NEAR_ROUTE_LON);
+		AvlProcessor.getInstance().processAvlReport(baseline);
+		long baselineLast = AvlProcessor.getInstance().lastAvlReportTime();
+		assertThat(baselineLast)
+				.as("regular report should update lastAvlReportTime")
+				.isEqualTo(baselineEpoch);
+
+		long schedEpoch = advanceClockBy(30_000L);
+		AvlReport schedBased = new AvlReport("v-schedbased",
+				schedEpoch,
+				NEAR_ROUTE_LAT, NEAR_ROUTE_LON,
+				Float.NaN, Float.NaN, "test");
+		schedBased.setAssignment(HAPPY_PATH_BLOCK_ID,
+				AssignmentType.BLOCK_FOR_SCHED_BASED_PREDS);
+		AvlProcessor.getInstance().processAvlReport(schedBased);
+
+		assertThat(AvlProcessor.getInstance().lastAvlReportTime())
+				.as("schedule-based-preds report must not advance the regular-AVL clock")
+				.isEqualTo(baselineLast);
+	}
 }

transitclockPipelineTests/src/test/java/org/transitclock/pipelinetests/HoldingTimeGeneratorBehaviorTest.java

@@ -0,0 +1,81 @@
+package org.transitclock.pipelinetests;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Date;
+
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.transitclock.core.holdingmethod.HoldingTimeGeneratorDefaultImpl;
+import org.transitclock.db.structs.HoldingTime;
+import org.transitclock.ipc.data.IpcArrivalDeparture;
+
+/**
+ * Behavior tests for {@link HoldingTimeGeneratorDefaultImpl}. The existing
+ * unit test in {@code transitclock/src/test} only covers the private
+ * {@code calculateHoldingTime} math helper via reflection; the public
+ * {@code generateHoldingTime} contract had no coverage.
+ */
+public class HoldingTimeGeneratorBehaviorTest {
+
+	@ClassRule
+	public static final CoreHarness CORE = CoreHarness.withWmata5A();
+
+	private static final String NON_CONTROL_STOP_ID = "14253";
+	private static final String ROUTE_ID = "5A";
+	private static final String TRIP_ID = "868588900";
+
+	private static IpcArrivalDeparture event(String vehicleId, String stopId,
+			boolean isArrival, long epochMs) {
+		IpcArrivalDeparture ad = new IpcArrivalDeparture();
+		ad.setVehicleId(vehicleId);
+		ad.setStopId(stopId);
+		ad.setRouteId(ROUTE_ID);
+		ad.setTripId(TRIP_ID);
+		ad.setArrival(isArrival);
+		ad.setTime(new Date(epochMs));
+		ad.setAvlTime(new Date(epochMs));
+		return ad;
+	}
+
+	// ---------- Tests ----------
+
+	@Test
+	public void generateHoldingTime_nonArrivalEventReturnsNull() {
+		IpcArrivalDeparture departure = event(
+				"v-ht-depart", NON_CONTROL_STOP_ID, /*isArrival*/ false,
+				1466437800000L);
+
+		HoldingTime result = new HoldingTimeGeneratorDefaultImpl()
+				.generateHoldingTime(/*vehicleState*/ null, departure);
+
+		assertThat(result)
+				.as("departure events must not produce a holding time")
+				.isNull();
+	}
+
+	@Test
+	public void generateHoldingTime_arrivalAtNonControlStopReturnsNull() {
+		IpcArrivalDeparture arrival = event(
+				"v-ht-arr-nonctrl", NON_CONTROL_STOP_ID, /*isArrival*/ true,
+				1466437800000L);
+
+		HoldingTime result = new HoldingTimeGeneratorDefaultImpl()
+				.generateHoldingTime(/*vehicleState*/ null, arrival);
+
+		assertThat(result)
+				.as("arrivals at non-control stops must not produce a holding time")
+				.isNull();
+	}
+
+	@Test
+	public void getControlPointStops_defaultsToEmptyList() {
+		// Callers of getControlPointStops iterate without null-guarding, so an
+		// unset controlStops config must produce an empty list, not null.
+		assertThat(new HoldingTimeGeneratorDefaultImpl().getControlPointStops())
+				.as("with no transitclock.holding.controlStops configured, "
+						+ "getControlPointStops() must return an empty (non-null) list")
+				.isNotNull()
+				.isEmpty();
+	}
+}