Skip to content

Commit 1e68064

Browse files
bedaHovorkaclaude
andcommitted
fix: address PR #421 review findings from Copilot and Claude Code Review
- Fix misleading "statically-linked" comment in fast-sim/build.gradle.kts (C3) - Switch buildFastSim task to release binary, matching CI (R1) - Fix SimulationEvent 2-part message duplication: source="" when only 2 parts (R3) - Remove obsolete TRAIN_EVENTS backward compat fallback in TextReporter (R4) - Use cp -rn in Dockerfile.fast-sim for BuildKit cache consistency (R5) - Make SIGINT_EXIT_CODE internal (R6) - Fix NativeExampleRegistry error message formatting (R7) - Remove 2 legacy fallback tests, update affected test assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1757b29 commit 1e68064

9 files changed

Lines changed: 11 additions & 46 deletions

File tree

Dockerfile.fast-sim

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ COPY docker-kdisco/ /root/kdisco-prebuild/
6060
RUN --mount=type=cache,target=/root/.gradle/caches,id=fast-sim-gradle \
6161
--mount=type=cache,target=/root/.gradle/wrapper,id=fast-sim-wrapper \
6262
--mount=type=cache,target=/root/.m2/repository,id=fast-sim-m2 \
63-
cp -r /root/kdisco-prebuild/. /root/.m2/repository/ && \
63+
cp -rn /root/kdisco-prebuild/. /root/.m2/repository/ && \
6464
./gradlew dependencies --no-daemon --warning-mode=summary
6565

6666
# Layer 4: Copy source code (changes most frequently)
@@ -76,7 +76,7 @@ ENV GRADLE_OPTS="-Xmx2g"
7676
RUN --mount=type=cache,target=/root/.gradle/caches,id=fast-sim-gradle \
7777
--mount=type=cache,target=/root/.gradle/wrapper,id=fast-sim-wrapper \
7878
--mount=type=cache,target=/root/.m2/repository,id=fast-sim-m2 \
79-
cp -r /root/kdisco-prebuild/. /root/.m2/repository/ && \
79+
cp -rn /root/kdisco-prebuild/. /root/.m2/repository/ && \
8080
./gradlew :fast-sim:linkReleaseExecutableLinuxX64 --no-daemon --warning-mode=summary
8181

8282
# Verify binary was created

build.gradle.kts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ listOf(
7979
tasks.register(name) { dependsOn(":desktop-ui:$name") }
8080
}
8181

82-
tasks.register("buildFastSim") { dependsOn(":fast-sim:linkDebugExecutableLinuxX64") } // debug; flip to Release once #415 lands
82+
tasks.register("buildFastSim") { dependsOn(":fast-sim:linkReleaseExecutableLinuxX64") }
8383
tasks.register("runFastSim") { dependsOn(":fast-sim:runDebugExecutableLinuxX64") }
8484
tasks.register("buildFastSimRelease") { dependsOn(":fast-sim:linkReleaseExecutableLinuxX64") }
8585
tasks.register("runFastSimRelease") { dependsOn(":fast-sim:runReleaseExecutableLinuxX64") }

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/sim/SimulationEvent.kt

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ data class SimulationEvent(
3535
val fullMessage = event.newValue?.toString() ?: return null
3636
val parts = fullMessage.trim().split(Regex("\\s+"), limit = 3)
3737
val simulationTime = parts[0].toDoubleOrNull() ?: return null
38-
val source = if (parts.size > 1) parts[1] else ""
39-
// When only one non-time token exists (degenerate input), source and message are the same token.
38+
val source = if (parts.size > 2) parts[1] else ""
4039
val message = if (parts.size > 2) parts[2] else if (parts.size > 1) parts[1] else fullMessage
4140
return SimulationEvent(simulationTime, reportType, source, message)
4241
}

core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/sim/TextReporter.kt

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,6 @@ class TextReporter(
4848
if (match != null) {
4949
trainNames.add(match.groupValues[1])
5050
}
51-
} else if (simEvent.eventType == ReportType.TRAIN_EVENTS) {
52-
// Backward compatibility for legacy logs that still emit TRAIN_EVENTS with
53-
// the word "approved" in the message (pre-TRAIN_APPROVED). This path can be
54-
// removed once all emitters migrate to TRAIN_APPROVED.
55-
val combined = "${simEvent.source} ${simEvent.message}"
56-
val approvedIndex = combined.indexOf("approved")
57-
if (approvedIndex > 0) {
58-
trainNames.add(combined.substring(0, approvedIndex).trim())
59-
}
6051
}
6152

6253
if (simEvent.eventType !in allowedTypes) return

core/src/commonTest/kotlin/cz/vutbr/fit/interlockSim/sim/SimulationEventTest.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class SimulationEventTest {
7878
val cce = ContextChangeEvent("TRAIN_EVENTS", null, "5.0 vlak1")
7979
val event = SimulationEvent.fromContextChangeEvent(cce)!!
8080
assertEquals(5.0, event.simulationTime)
81-
assertEquals("vlak1", event.source)
81+
assertEquals("", event.source)
8282
assertEquals("vlak1", event.message)
8383
}
8484

core/src/commonTest/kotlin/cz/vutbr/fit/interlockSim/sim/TextReporterTest.kt

Lines changed: 3 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -229,29 +229,6 @@ class TextReporterTest {
229229
assertTrue(summary.contains("0 trains"), "No train should be counted without regex match: $summary")
230230
}
231231

232-
@Test
233-
fun legacyTrainEventsFallbackCountsApprovedTrains() {
234-
// Legacy path: TRAIN_EVENTS with "approved" keyword (backward compat)
235-
val output = mutableListOf<String>()
236-
val reporter = TextReporter(Verbosity.DEFAULT) { output.add(it) }
237-
fireEvent(reporter, ReportType.TRAIN_EVENTS, "1.0 vlak1 approved IO1->IO2")
238-
reporter.printSummary()
239-
val summary = output.last()
240-
assertTrue(summary.contains("1 trains"), "Legacy fallback should count 1 train: $summary")
241-
}
242-
243-
@Test
244-
fun legacyFallbackIgnoresApprovedAtStartOfCombined() {
245-
// approvedIndex must be > 0 (not at position 0) to extract train name
246-
val output = mutableListOf<String>()
247-
val reporter = TextReporter(Verbosity.DEFAULT) { output.add(it) }
248-
// SimulationEvent parsing: "1.0 approved" -> source="approved", message="approved"
249-
// combined = "approved approved", approvedIndex=0 -> skipped
250-
fireEvent(reporter, ReportType.TRAIN_EVENTS, "1.0 approved something")
251-
reporter.printSummary()
252-
val summary = output.last()
253-
assertTrue(summary.contains("0 trains"), "Should not count when 'approved' is at position 0: $summary")
254-
}
255232

256233
@Test
257234
fun summaryWithZeroTrains() {
@@ -298,9 +275,7 @@ class TextReporterTest {
298275
fun formatEventWithEmptySource() {
299276
val output = mutableListOf<String>()
300277
val reporter = TextReporter(Verbosity.DEFAULT) { output.add(it) }
301-
// Two-part message: "5.0 msg" -> source="msg", message="msg"
302-
// But source is not empty so it goes through else branch
303-
// To test empty source, we need a degenerate one-part message that still parses
278+
// Three-part message: "5.0 pathSet some-details" -> source="pathSet", message="some-details"
304279
fireEvent(reporter, ReportType.PATH_SETTING, "5.0 pathSet some-details")
305280
assertTrue(output[0].contains("pathSet"))
306281
}
@@ -326,14 +301,14 @@ class TextReporterTest {
326301
}
327302

328303
@Test
329-
fun legacyFallbackDoesNotCountNonApprovedTrainEvents() {
304+
fun trainEventsDoNotCountTrains() {
330305
val output = mutableListOf<String>()
331306
val reporter = TextReporter(Verbosity.DEFAULT) { output.add(it) }
332307
fireEvent(reporter, ReportType.TRAIN_EVENTS, "1.0 vlak1 stopped at signal")
333308
fireEvent(reporter, ReportType.TRAIN_EVENTS, "2.0 vlak1 exiting system")
334309
reporter.printSummary()
335310
val summary = output.last()
336-
assertTrue(summary.contains("0 trains"), "Non-approved events should not count trains: $summary")
311+
assertTrue(summary.contains("0 trains"), "TRAIN_EVENTS should not count trains: $summary")
337312
}
338313

339314
// --- Contract tests: verify Train.formatApprovalMessage produces TextReporter-parseable output ---

fast-sim/build.gradle.kts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* fast-sim/build.gradle.kts
33
*
44
* :fast-sim — native linuxX64 simulation CLI
5-
* Compiles to a statically-linked native binary with no JVM dependency.
5+
* Compiles to a native binary with no JVM dependency.
66
* Depends on :core (commonMain provides simulation engine, XML parsing, domain model).
77
*/
88

fast-sim/src/linuxX64Main/kotlin/cz/vutbr/fit/interlockSim/fastsim/Main.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import kotlin.concurrent.AtomicInt
2626
import kotlin.system.exitProcess
2727

2828
/** Unix convention exit code for processes terminated by SIGINT (128 + 2). */
29-
const val SIGINT_EXIT_CODE = 130
29+
internal const val SIGINT_EXIT_CODE = 130
3030

3131
/**
3232
* Holds the atomic SIGINT flag. Encapsulated in an object because the signal handler

fast-sim/src/linuxX64Main/kotlin/cz/vutbr/fit/interlockSim/fastsim/NativeExampleRegistry.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ internal object NativeExampleRegistry {
3030
fun create(name: String, endTime: Long, factory: NativeContextFactory): DefaultSimulationContext =
3131
when (name) {
3232
EXAMPLE_SHUNTING_LOOP -> createShuntingLoop(endTime, factory)
33-
else -> throw IllegalArgumentException("Unknown example: '$name'. Available: $AVAILABLE")
33+
else -> throw IllegalArgumentException("Unknown example: '$name'. Available: ${AVAILABLE.sorted().joinToString()}")
3434
}
3535

3636
@Suppress("TooGenericExceptionCaught")

0 commit comments

Comments
 (0)