diff --git a/AGENTS.md b/AGENTS.md index 36e1b7532..30567bc5e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -123,22 +123,34 @@ fun ClientGameTestContext.testComposeScreenMeasuresRenderableNode() { See `ComposeRenderingTests.kt` for a full example. Registered the same way, via `register()` inside the `client { }` block. -**Known open issue - client GameTests are intermittently flaky (~1-in-5), cause not confirmed.** -Repeated local runs of `neoforge:runGametestClient` show a different click/animation-driven test -failing each time (`ConfirmDialog`, `RadioGroup`, ...) with `IllegalStateException: Predicate did -not become true within 200 ticks` and no logged exception - not a per-test logic bug. The specific -`isComposeIdle()` TOCTOU gap documented in `ComposeScreen.kt`'s KDoc is already fixed (it now uses -`Recomposer.hasPendingWork`, not the old `recomposeJob`-based check), so that's not the live cause. -The likely remaining gap: `ComposeScreen`'s coroutine scope (`CoroutineScope(Dispatchers.Default) + -BroadcastFrameClock`) runs on **real** threads/wall-clock time, so a composable's `delay(...)` (e.g. -`ConfirmDialog`'s close animation) genuinely races the harness's tick-based polling and the real -render loop's frame delivery. Real Jetpack Compose's own test tooling -(`ComposeTestRule`/`runComposeUiTest`) avoids this whole class of race by backing the composition -with a *virtual* clock/dispatcher (`TestMonotonicFrameClock` over `StandardTestDispatcher`) that -`waitForIdle()` drives forward deterministically, instead of polling real concurrency - Archie's -harness has no equivalent. A real fix likely means a test-only virtual-clock/dispatcher swap for -`ComposeScreen` during GameTests, not another polling tweak. Not yet attempted - would need live -instrumentation to confirm before changing anything. +**Client GameTests run on a virtual clock/dispatcher, not real threads.** `ComposeScreen`/ +`ComposeContainerScreen` normally back their coroutine scope with `Dispatchers.Default` and real +wall-clock time, so a composable's `delay(...)` (e.g. a dialog's close animation) genuinely raced +real thread scheduling and frame delivery against the harness's tick-based polling - roughly a +1-in-5 failure rate, a different test failing each time, no logged exception. Real Jetpack +Compose's own test tooling avoids this whole class of race by backing composition with a virtual +clock/dispatcher instead of real concurrency; `AClientGameTestHarness.kt`'s `run()` now does the +same, installing a `ComposeTestClockOverride` (`common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeScreen.kt`) +around each test - a `StandardTestDispatcher` plus a per-frame `scheduler.advanceTimeBy(50)` pump +called from `renderNodes()`. Confirmed fixed: 8 consecutive full local `neoforge:runGametestClient` +runs (131 individual tests total, including the previously-flaky ones), zero failures. + +Use `advanceTimeBy(bounded)`, never `advanceUntilIdle()`, for that pump - composables can run +legitimately infinite `delay()` loops (e.g. `TextFieldCore`'s blinking-cursor `LaunchedEffect`), +and `advanceUntilIdle()` only returns once truly nothing is scheduled anywhere, which for an +unboundedly-recurring loop is never. The first attempt at this fix used `advanceUntilIdle()` and +hung the render thread permanently the moment any screen with a focused text field was tested - +every subsequent test in that run then failed too, since the client's main-thread executor queue +never got a chance to run again (`client.screen` frozen at whatever it was when the hang started). + +`kotlinx-coroutines-test` (the dependency this needs) is dev/test-only - `compileOnly` in +`common/build.gradle.kts`, `runtimeLibrary(...)` (present for local runs like +`runGametestClient`, never bundled into the shipped jar) in the loader modules. `ComposeScreen` +itself never references `kotlinx.coroutines.test.*` symbols directly (only `AClientGameTestHarness`'s +method bodies do, and those only run under `AGameTestPlatform.isGameTest`) - it only holds a plain +`CoroutineDispatcher?` and a `(() -> Unit)?` pump callback, both already-bundled core/stdlib types, +specifically so a real player's game (which never has the test dependency on its classpath) never +needs to resolve it. ### Current test coverage - `ArchieItemHandlerTests` (`server`) – item storage/handler behavior diff --git a/Archie/common/build.gradle.kts b/Archie/common/build.gradle.kts index 9a0a4beea..89fded60b 100644 --- a/Archie/common/build.gradle.kts +++ b/Archie/common/build.gradle.kts @@ -62,6 +62,14 @@ sourceSets { dependencies { compileOnly(kotlin("reflect")) implementation(libs.junit.jupiter.api) + // Used by the client GameTest harness only (AClientGameTestHarness.kt) to give ComposeScreen a + // virtual clock/dispatcher during tests. compileOnly (not implementation/api) deliberately - + // this must never end up in the shipped jar. ComposeScreen itself never references + // kotlinx.coroutines.test.* symbols directly (only AClientGameTestHarness.kt's method bodies + // do, and those only ever run under AGameTestPlatform.isGameTest), so a real player's game - + // which never has this on its classpath - never needs to resolve it. Loader modules add it + // back as runtimeOnly (not bundled - see AGENTS.md) so local `runGametestClient` has it. + compileOnly(libs.kotlinx.coroutines.test) testImplementation(libs.junit.jupiter.api) testImplementation(kotlin("reflect")) testRuntimeOnly(libs.junit.jupiter.engine) diff --git a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gametest/AClientGameTestHarness.kt b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gametest/AClientGameTestHarness.kt index 387a4abed..494dfebe5 100644 --- a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gametest/AClientGameTestHarness.kt +++ b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gametest/AClientGameTestHarness.kt @@ -2,8 +2,12 @@ package net.kernelpanicsoft.archie.gametest import com.mojang.realmsclient.RealmsMainScreen import dev.architectury.platform.Mod +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.TestCoroutineScheduler import net.kernelpanicsoft.archie.Archie import net.kernelpanicsoft.archie.gui.ComposeIdleAware +import net.kernelpanicsoft.archie.gui.ComposeTestClockOverride import net.kernelpanicsoft.archie.gui.LayerManagerProvider import net.kernelpanicsoft.archie.util.setReflection import net.minecraft.SharedConstants @@ -1491,6 +1495,7 @@ data class AClientGameTestSummary( * No-ops (returning an all-zero summary) unless [side] is [AGameTestSide.CLIENT]. */ object AClientGameTestHarness { + @OptIn(ExperimentalCoroutinesApi::class) fun run(modToClasses: Map>>, side: AGameTestSide?): AClientGameTestSummary { if (side != AGameTestSide.CLIENT) return AClientGameTestSummary(passed = 0, failed = 0, skipped = 0) @@ -1531,21 +1536,43 @@ object AClientGameTestHarness { return@forEach } - runCatching { - context.getInput().clearInputs() - method.isAccessible = true - if (params.isEmpty()) method.invoke(instance) - else method.invoke(instance, context) - }.onSuccess { - context.getInput().clearInputs() - passed++ - Archie.LOGGER.info("[ClientGameTest] PASS {}", testId) - }.onFailure { error -> - runCatching { context.getInput().clearInputs() } - failed++ - failedTests += testId - failedDetails += AClientGameTestFailure(testId, rootCauseSummary(error)) - Archie.LOGGER.error("[ClientGameTest] FAIL {}", testId, error) + // Give every ComposeScreen this test opens a virtual clock/dispatcher (see + // ComposeTestClockOverride's KDoc) instead of real threads/wall-clock time - + // installed here (not the render thread) since setScreen{} dispatches actual + // screen construction there, but reads the override via a plain shared + // static, not a ThreadLocal. Always cleared, even on failure, so it can never + // leak into the next test or (in principle) the running game. + // + // advanceTimeBy(bounded), NOT advanceUntilIdle() - composables can run + // legitimately infinite delay() loops (e.g. TextFieldCore's blinking-cursor + // LaunchedEffect), and advanceUntilIdle() only returns once truly nothing is + // scheduled, which for an unboundedly-recurring loop is never: it hangs the + // render thread forever the first time such a composable is on screen. A fixed + // per-pump increment (~1 tick) makes every delay() progress a little on every + // real frame instead, bounded and hang-proof either way. + val scheduler = TestCoroutineScheduler() + ComposeTestClockOverride.dispatcher = StandardTestDispatcher(scheduler) + ComposeTestClockOverride.pump = { scheduler.advanceTimeBy(50) } + try { + runCatching { + context.getInput().clearInputs() + method.isAccessible = true + if (params.isEmpty()) method.invoke(instance) + else method.invoke(instance, context) + }.onSuccess { + context.getInput().clearInputs() + passed++ + Archie.LOGGER.info("[ClientGameTest] PASS {}", testId) + }.onFailure { error -> + runCatching { context.getInput().clearInputs() } + failed++ + failedTests += testId + failedDetails += AClientGameTestFailure(testId, rootCauseSummary(error)) + Archie.LOGGER.error("[ClientGameTest] FAIL {}", testId, error) + } + } finally { + ComposeTestClockOverride.dispatcher = null + ComposeTestClockOverride.pump = null } } } diff --git a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeContainerScreen.kt b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeContainerScreen.kt index 3509cd7c4..93583f937 100644 --- a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeContainerScreen.kt +++ b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeContainerScreen.kt @@ -86,7 +86,11 @@ abstract class ComposeContainerScreen, B : BlockE private var hasFrameWaiters = false private val clock = BroadcastFrameClock { hasFrameWaiters = true } - private val composeScope = CoroutineScope(Dispatchers.Default) + clock + // See ComposeScreen's identical fields for why this is captured once and untyped against + // kotlinx-coroutines-test. + private val testPump = ComposeTestClockOverride.pump + + private val composeScope = CoroutineScope(ComposeTestClockOverride.dispatcher ?: Dispatchers.Default) + clock final override val coroutineContext: CoroutineContext = composeScope.coroutineContext final override lateinit var layerManager: LayerStackManager @@ -165,6 +169,8 @@ abstract class ComposeContainerScreen, B : BlockE * job is launched if there are pending frame waiters. */ open fun renderNodes(baseLayer: Boolean, guiGraphics: GuiGraphics, mouseX: Int, mouseY: Int, partialTick: Float) { + // See ComposeScreen.renderNodes for why this runs first. + testPump?.invoke() if (asynchronous) { recomposeJob?.let { job -> runBlocking { diff --git a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeScreen.kt b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeScreen.kt index c0bed640d..9a27044e3 100644 --- a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeScreen.kt +++ b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/gui/ComposeScreen.kt @@ -43,6 +43,40 @@ interface LayerManagerProvider val layerManager: LayerStackManager } +/** + * Test-only override, installed by the client GameTest harness (`AClientGameTestHarness.kt`), + * that swaps [ComposeScreen]'s real-time coroutine dispatcher for a virtual one so a composable's + * `delay(...)` (e.g. a dialog's close animation) resolves deterministically against a scheduler + * the harness drives itself, instead of racing real wall-clock time, real thread scheduling, and + * the harness's tick-based polling - the same class of flakiness Jetpack Compose's own test + * tooling (`ComposeTestRule`/`runComposeUiTest`) avoids by construction, backing composition with + * a virtual clock/dispatcher that `waitForIdle()` drives forward instead of polling real + * concurrency. `null` unless a client GameTest explicitly installs one; the running game never + * touches this. + * + * Deliberately untyped against `kotlinx-coroutines-test` (`CoroutineDispatcher` is a + * kotlinx-coroutines-core type; `pump` is a plain lambda) - that dependency is dev/test-only + * (`compileOnly` in `common`, `runtimeLibrary` - present for local runs, never bundled - in the + * loader modules; see `common/build.gradle.kts`). [ComposeScreen] is loaded by every screen in + * the mod, so its own class file must never reference a symbol that isn't resolvable in a real + * player's game; only [AClientGameTestHarness]'s method bodies (never invoked outside + * `AGameTestPlatform.isGameTest`) construct the actual `StandardTestDispatcher`/ + * `TestCoroutineScheduler` instances installed here. + */ +internal object ComposeTestClockOverride { + /** The dispatcher to back new [ComposeScreen]s' coroutine scope with, in place of [kotlinx.coroutines.Dispatchers.Default]. */ + @Volatile + var dispatcher: CoroutineDispatcher? = null + + /** + * Called once per real rendered frame by every live [ComposeScreen] while installed - drains + * all outstanding virtual-time work (recomposition, effects, `delay(...)`) synchronously on + * the render thread. Set alongside [dispatcher] to `{ scheduler.advanceUntilIdle() }`. + */ + @Volatile + var pump: (() -> Unit)? = null +} + /** * A Compose-driven Minecraft [Screen] base class with layer support, async recomposition, * and full pointer/keyboard input dispatch. @@ -75,7 +109,12 @@ abstract class ComposeScreen( private var hasFrameWaiters = false private val clock = BroadcastFrameClock { hasFrameWaiters = true } - private val composeScope = CoroutineScope(Dispatchers.Default) + clock + // Captured once at construction (not read live from ComposeTestClockOverride elsewhere) so + // this screen keeps working consistently even if a later test installs/clears the override + // while this screen is still disposing. + private val testPump = ComposeTestClockOverride.pump + + private val composeScope = CoroutineScope(ComposeTestClockOverride.dispatcher ?: Dispatchers.Default) + clock final override val coroutineContext: CoroutineContext = composeScope.coroutineContext final override lateinit var layerManager: LayerStackManager @@ -149,6 +188,11 @@ abstract class ComposeScreen( * job is launched if there are pending frame waiters. */ open fun renderNodes(guiGraphics: GuiGraphics, mouseX: Int, mouseY: Int, partialTick: Float) { + // Under a client GameTest's virtual dispatcher (see ComposeTestClockOverride), nothing + // launched on composeScope runs on its own - it only progresses when the scheduler backing + // it is advanced. Doing that here, once per real rendered frame, deterministically flushes + // recomposition/effects/delay() before the join below, instead of racing real threads. + testPump?.invoke() if (asynchronous) { recomposeJob?.let { runBlocking { it.join() } } recomposeJob = null diff --git a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/transfer/ArchieFluidSlot.kt b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/transfer/ArchieFluidSlot.kt index 1f6093941..501b2d518 100644 --- a/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/transfer/ArchieFluidSlot.kt +++ b/Archie/common/src/main/kotlin/net/kernelpanicsoft/archie/transfer/ArchieFluidSlot.kt @@ -5,6 +5,7 @@ import earth.terrarium.common_storage_lib.resources.ResourceStack import earth.terrarium.common_storage_lib.resources.fluid.FluidResource import earth.terrarium.common_storage_lib.storage.base.StorageSlot import earth.terrarium.common_storage_lib.storage.base.UpdateManager +import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.KSerializer import kotlinx.serialization.Serializable import kotlinx.serialization.builtins.serializer @@ -147,6 +148,7 @@ class ArchieFluidSlot(private val limit: Long, private val onUpdate: () -> Unit } /** Serializes an [ArchieFluidSlot] as its [limit] followed by its [ResourceStack] (or `null` when blank). */ + @OptIn(ExperimentalSerializationApi::class) object Serializer : KSerializer { private val surrogate = ResourceStack.FLUID_CODEC.kSerializer diff --git a/Archie/fabric/build.gradle.kts b/Archie/fabric/build.gradle.kts index f799d5f10..de0cc4081 100644 --- a/Archie/fabric/build.gradle.kts +++ b/Archie/fabric/build.gradle.kts @@ -1,5 +1,6 @@ import net.kernelpanicsoft.archie.plugin.bundleMod import net.kernelpanicsoft.archie.plugin.bundleRuntimeLibrary +import net.kernelpanicsoft.archie.plugin.runtimeLibrary import org.jetbrains.kotlin.konan.properties.loadProperties @@ -156,6 +157,8 @@ dependencies { implementation(libs.junit.jupiter.api) testImplementation(libs.junit.jupiter.api) testRuntimeOnly(libs.junit.jupiter.engine) + // dev/test-only, not shipped - see the compileOnly note in common/build.gradle.kts + runtimeLibrary(libs.kotlinx.coroutines.test) "common"(project(":common", "namedElements")) { isTransitive = false } "shadowCommon"(project(":common", "transformProductionFabric")) { isTransitive = false } diff --git a/Archie/neoforge/build.gradle.kts b/Archie/neoforge/build.gradle.kts index f32f8065c..b1f44d0a7 100644 --- a/Archie/neoforge/build.gradle.kts +++ b/Archie/neoforge/build.gradle.kts @@ -1,5 +1,6 @@ import net.kernelpanicsoft.archie.plugin.bundleMod import net.kernelpanicsoft.archie.plugin.bundleRuntimeLibrary +import net.kernelpanicsoft.archie.plugin.runtimeLibrary import org.jetbrains.kotlin.konan.properties.loadProperties @@ -162,6 +163,8 @@ dependencies { implementation(libs.junit.jupiter.api) testImplementation(libs.junit.jupiter.api) testRuntimeOnly(libs.junit.jupiter.engine) + // dev/test-only, not shipped - see the compileOnly note in common/build.gradle.kts + runtimeLibrary(libs.kotlinx.coroutines.test) "common"(project(":common", "namedElements")) { isTransitive = false } "shadowCommon"(project(":common", "transformProductionNeoForge")) { isTransitive = false } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index abe9d8f74..fcce3ac0d 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -10,6 +10,7 @@ kotlin-neoforge = "2.11.1-k2.2.21-3.0+neoforge" #kotlin-neoforge-range = "5" kotlin-neoforge-range = "2" kotlinx-serialization = { require = "[1.8.0,)", prefer = "1.9.0" } +kotlinx-coroutines-test = "1.8.0" # Minecraft minecraft = { strictly = "1.21.1" } @@ -70,6 +71,7 @@ kotlinx-serialization-cbor = { module = "org.jetbrains.kotlinx:kotlinx-serializa kotlinx-serialization-json5 = { module = "io.github.xn32:json5k-jvm", version.ref = "json5k" } kotlinx-serialization-nbt = { module = "net.benwoodworth.knbt:knbt-jvm", version.ref = "knbt" } kotlinx-serialization-toml = { module = "net.peanuuutz.tomlkt:tomlkt-jvm", version.ref = "tomlkt" } +kotlinx-coroutines-test = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-test", version.ref = "kotlinx-coroutines-test" } minecraft = { module = "com.mojang:minecraft", version.ref = "minecraft" } parchment = { module = "org.parchmentmc.data:parchment-1.21.1", version.ref = "parchment" }