Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,34 @@ fun ClientGameTestContext.testComposeScreenMeasuresRenderableNode() {
See `ComposeRenderingTests.kt` for a full example. Registered the same way, via `register<TestClass>()`
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
Expand Down
8 changes: 8 additions & 0 deletions Archie/common/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Mod, List<Class<*>>>, side: AGameTestSide?): AClientGameTestSummary {
if (side != AGameTestSide.CLIENT) return AClientGameTestSummary(passed = 0, failed = 0, skipped = 0)

Expand Down Expand Up @@ -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
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ abstract class ComposeContainerScreen<T : ComposeContainerMenu<B, T>, 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
Expand Down Expand Up @@ -165,6 +169,8 @@ abstract class ComposeContainerScreen<T : ComposeContainerMenu<B, T>, 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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<ArchieFluidSlot>
{
private val surrogate = ResourceStack.FLUID_CODEC.kSerializer
Expand Down
3 changes: 3 additions & 0 deletions Archie/fabric/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -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


Expand Down Expand Up @@ -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 }
Expand Down
3 changes: 3 additions & 0 deletions Archie/neoforge/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -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


Expand Down Expand Up @@ -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 }
Expand Down
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down Expand Up @@ -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" }
Expand Down
Loading