Repository navigation
KTOR-9796 Add flaky-test quarantine infrastructure and transient-only retry - #5838
VGoncharova wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 139f67ed46
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| tasks.register<Test>(FLAKY_TEST_TASK) { | ||
| classpath = jvmTestRuntimeClasspath | ||
| testClassesDirs = jvmTestClassesDirs |
There was a problem hiding this comment.
When a test is quarantined in ktor-server-netty or either Jetty HTTP/2 test module, this independently registered task does not inherit the module's jvmTest configuration, including systemProperty("enable.http2", "true"); EngineTestBaseJvm consequently initializes enableHttp2 to false. The Android client module similarly loses its http.maxConnections JVM argument. Those tests therefore run under different conditions in flakyTest, producing misleading failures and corrupting the tracked flip rate; preserve the original JVM task's execution settings without adding it as a task dependency.
Useful? React with 👍 / 👎.
| tasks.withType<AbstractTestTask>().configureEach { | ||
| if (this !is Test) selectFlakyTests(mode) |
There was a problem hiding this comment.
When Android host tests are enabled, their Gradle tasks are Test tasks too, so this type check exempts them along with the JVM jvmTest task. However, configureJvm() installs the JUnit condition only on the ordinary JVM tasks, not Android host tests; a common _flaky test therefore still runs in the default Android unit-test suite and is not isolated by -Pktor.tests.flaky=only. Exempt only JVM tasks that actually receive FlakyTestCondition, or configure Android host tasks with an equivalent selector.
AGENTS.md reference: AGENTS.md:L18-L21
Useful? React with 👍 / 👎.
| if (deadline.hasPassedNow()) fail("Timed out after $timeout waiting for $description") | ||
| realTimeDelay(interval) |
There was a problem hiding this comment.
When interval exceeds the time remaining before the deadline—for example, timeout = 100.milliseconds and interval = 1.seconds—the deadline check passes and this call sleeps for the entire interval, so the helper fails after roughly one second despite documenting timeout as an upper bound. Delay by at most the remaining duration, or enforce the timeout around the polling loop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces shared infrastructure for quarantining flaky tests and ensuring retries only apply to transient failures, along with a real-time polling helper to reduce flakiness from immediate liveness assertions. It wires a single ktor.tests.flaky mode across Gradle and JVM test execution (via a JUnit ExecutionCondition), and adds targeted self-tests/fixtures to verify the behavior.
Changes:
- Add
@Flaky(ticket)plus JVM enforcement viaFlakyTestConditionand non-JVM selection via the_flakyname token + Gradle filtering. - Introduce transient-only retries by classifying assertion failures as deterministic (
DeterministicFailureGuard) inrunTestWithDataandBaseTest.runTest. - Add
assertEventually(real-time polling) with a default timeout constant and accompanying tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ktor-shared/ktor-test-base/jvm/test/io/ktor/test/junit/FlakyTestConditionTest.kt | Self-test fixtures validating JVM @Flaky exclusion/inclusion behavior. |
| ktor-shared/ktor-test-base/jvm/src/io/ktor/test/junit/FlakyTestCondition.kt | JUnit ExecutionCondition that gates @Flaky tests by ktor.tests.flaky mode. |
| ktor-shared/ktor-test-base/jvm/resources/META-INF/services/org.junit.jupiter.api.extension.Extension | Registers FlakyTestCondition for JUnit extension autodetection. |
| ktor-shared/ktor-test-base/common/test/io/ktor/test/RunTestWithDataTest.kt | Updates retry tests and adds coverage for “assertion failures are not retried”. |
| ktor-shared/ktor-test-base/common/test/io/ktor/test/FlakyNameConventionTest.kt | Cross-platform fixture guarding the _flaky naming convention selection. |
| ktor-shared/ktor-test-base/common/test/io/ktor/test/EventuallyTest.kt | Tests for assertEventually semantics and real-time waiting under runTest. |
| ktor-shared/ktor-test-base/common/test/io/ktor/test/DeterministicFailureGuardTest.kt | Unit tests for deterministic-vs-transient failure classification. |
| ktor-shared/ktor-test-base/common/src/io/ktor/test/TestDefaults.kt | Adds DEFAULT_EVENTUALLY_TIMEOUT. |
| ktor-shared/ktor-test-base/common/src/io/ktor/test/runTestWithData.kt | Implements deterministic-failure guarding to prevent retrying assertion failures. |
| ktor-shared/ktor-test-base/common/src/io/ktor/test/Flaky.kt | Introduces @Flaky(ticket) and documents cross-platform selection rules. |
| ktor-shared/ktor-test-base/common/src/io/ktor/test/Eventually.kt | Adds assertEventually real-time polling helper. |
| ktor-shared/ktor-test-base/common/src/io/ktor/test/DeterministicFailureGuard.kt | Adds guard used to stop retry loops on deterministic assertion failures. |
| ktor-server/ktor-server-test-base/nonJvm/src/io/ktor/server/test/base/BaseTest.nonJvm.kt | Applies deterministic-failure guarding to BaseTest.runTest on non-JVM. |
| ktor-server/ktor-server-test-base/jvm/test/io/ktor/server/test/base/BaseTestRetryJvmTest.kt | JVM-only assertions verifying BaseTest.runTest retry classification. |
| ktor-server/ktor-server-test-base/jvm/src/io/ktor/server/test/base/BaseTestJvm.kt | Applies deterministic-failure guarding to BaseTest.runTest on JVM. |
| ktor-server/ktor-server-test-base/common/test/io/ktor/server/test/base/BaseTestTest.kt | Updates shared retry tests to use transient (retryable) failures. |
| build-logic/src/main/kotlin/ktorbuild/targets/JvmConfig.kt | Wires ktor.tests.flaky into JVM test tasks and adds a dedicated flakyTest task. |
| build-logic/src/main/kotlin/ktorbuild/targets/FlakyTestsConfig.kt | Adds non-JVM Gradle filtering based on _flaky name token and ktor.tests.flaky. |
| build-logic/src/main/kotlin/ktorbuild.kmp.gradle.kts | Enables configureFlakyTests() for KMP builds. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @param retries Number of additional attempts after initial failure (`0` means no retries). | ||
| * Only transient failures are retried: an assertion failed by the test body is deterministic, | ||
| * so it is reported immediately instead of being retried and masked. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 843a84c5a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| .filter { it.isFlaky() } | ||
| .assertTrue( | ||
| additionalMessage = "A @Flaky test compiled for a non-JVM target also needs the " + | ||
| "'$FLAKY_NAME_TOKEN' name token, which is the only selector those targets have.", |
There was a problem hiding this comment.
When @Flaky is placed on a common-source class—an explicitly supported annotation target—this check never inspects that class because it collects only functions. A class without _flaky in its class or method names will therefore be quarantined by JUnit on JVM but continue running in the default Native/JS/Wasm suites, and this convention test will still pass. Include class declarations and inherited/enclosing class markers in the validation.
AGENTS.md reference: AGENTS.md:L18-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looks like a valid point.
There was a problem hiding this comment.
| ) { | ||
| val deadline = TimeSource.Monotonic.markNow() + timeout | ||
| while (true) { | ||
| if (condition()) return |
There was a problem hiding this comment.
When condition itself suspends or runs longer than the remaining timeout, this call is not bounded: a condition that delays for one second with timeout = 100.milliseconds can return successfully after one second, while a false result throws only after the same overshoot. This contradicts the documented total-wait upper bound even though polling delays are now clamped; enforce the deadline around each condition evaluation as well.
AGENTS.md reference: AGENTS.md:L97-L100
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f05e629a36
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
Osip Fatkullin (osipxd)
left a comment
Osip Fatkullin (osipxd)
left a comment
There was a problem hiding this comment.
Great job! I have some comments on how we could deduplicate test tasks configuration and improve assertEventually.
Also I've noticed that the agent (was it Claude?) tends to write too many comments even for cases when code is straightforward. So left comments for some of these places
| maxHeapSize = "2g" | ||
| // A quarantined test flipping is the expected outcome here, not a regression to block on. | ||
| // Failures still land in the test reports and in Develocity, which is where the flip rate is tracked. | ||
| ignoreFailures = true | ||
| // Each run is a fresh sample of whether the test still flips, so an unchanged input tree is | ||
| // no reason to skip it — up-to-date checking would report the previous run's verdict forever. | ||
| outputs.upToDateWhen { false } | ||
| systemProperty(FLAKY_MODE_PROPERTY, FlakyTestsMode.ONLY.propertyValue) | ||
| systemProperty("junit.jupiter.extensions.autodetection.enabled", "true") | ||
| exclude("**/*StressTest*") | ||
| useJUnitPlatform() | ||
| configureJavaToolchain(java.toolchain.languageVersion, ktorBuild.jvmTestToolchain) |
There was a problem hiding this comment.
I think we should apply common configs in tasks.withType<Test>().configureEach { ... } for all JVM test tasks (heap size, properties, JUnit platform, toolchain)
| // A quarantined test flipping is the expected outcome here, not a regression to block on. | ||
| // Failures still land in the test reports and in Develocity, which is where the flip rate is tracked. | ||
| ignoreFailures = true | ||
| // Each run is a fresh sample of whether the test still flips, so an unchanged input tree is | ||
| // no reason to skip it — up-to-date checking would report the previous run's verdict forever. | ||
| outputs.upToDateWhen { false } |
There was a problem hiding this comment.
After extracting common configs to another place, these two properties will be visible, so comments won't be necessary
| // Take the test classpath from the JVM test *compilation*, not from the `jvmTest` task. A | ||
| // provider derived from `tasks.named("jvmTest")` carries that task as its producer, so depending | ||
| // on it makes Gradle run the whole default suite before `stressTest` or `flakyTest`. The | ||
| // compilation's own file collections are built by the compile tasks, which is all these need. | ||
| val jvmTestCompilation = kotlin.jvm().compilations.named("test") | ||
| val jvmTestRuntimeClasspath = files( | ||
| jvmTestCompilation.map { it.output.allOutputs }, | ||
| jvmTestCompilation.map { it.runtimeDependencyFiles }, | ||
| ) | ||
| val jvmTestClassesDirs = files(jvmTestCompilation.map { it.output.classesDirs }) |
There was a problem hiding this comment.
Won't the same trick as in inheritExecutionSettingsFrom work in this case?
| // | ||
| // The module is added as a source directory rather than as a project dependency because it is a | ||
| // Kotlin Multiplatform project built by the `ktorbuild.project.internal` convention plugin — which | ||
| // this build produces, so including it here would be circular. |
There was a problem hiding this comment.
Could we reduce this comment to something like this?
We want to share test constants logic between tests and build-logic.
Adding that module as a project dependency would be circular as it uses plugins from build-logic.
| suspend fun assertEventually( | ||
| description: String, | ||
| timeout: Duration = DEFAULT_EVENTUALLY_TIMEOUT, | ||
| interval: Duration = (timeout / 20).coerceIn(1.milliseconds, 100.milliseconds), | ||
| condition: suspend () -> Boolean, | ||
| ) { | ||
| val deadline = TimeSource.Monotonic.markNow() + timeout | ||
| while (true) { | ||
| if (condition()) return | ||
| val remaining = -deadline.elapsedNow() | ||
| if (remaining <= Duration.ZERO) { | ||
| throw EventuallyTimeoutException("Timed out after $timeout waiting for $description") | ||
| } | ||
| // Never sleep past the deadline: `timeout` is documented as the upper bound on the wait, so | ||
| // an interval larger than what is left would overshoot it (`interval = 1s` with a 100ms | ||
| // timeout would fail after a second). | ||
| realTimeDelay(minOf(interval, remaining)) | ||
| } | ||
| } |
There was a problem hiding this comment.
My idea was to throw the native CoroutineTimeoutException so it could be later recognized by runTest and it could print coroutines trace. I think the following shape with a separate watcher coroutine should do the trick:
suspend fun assertEventually(
timeout: Duration = DEFAULT_EVENTUALLY_TIMEOUT,
interval: Duration = (timeout / 20).coerceIn(1.milliseconds, 100.milliseconds),
condition: () -> Boolean,
) = coroutineScope {
val realTimeDispatcher = Dispatchers.Default.limitedParallelism(1)
val completion = CompletableDeferred<Unit>()
val watcher = async(realTimeDispatcher + CoroutineName("timeout-watcher")) {
withTimeout(timeout) { completion.join() }
}
while (true) {
watcher.ensureActive()
if (condition()) {
completion.complete(Unit)
break
}
withContext(realTimeDispatcher) { delay(interval) }
}
watcher.await()
}One important detail. The condition shouldn't be a suspend lambda as if it hangs we can't limit its execution time
There was a problem hiding this comment.
a simpler version of the same idea: a single withTimeout(timeout) { while (...) { delay(interval) } } block throws the native TimeoutCancellationException directly, giving runTest/kotlinx-coroutines-debug the coroutine dump without a second watcher coroutine.
| .filter { it.isFlaky() } | ||
| .assertTrue( | ||
| additionalMessage = "A @Flaky test compiled for a non-JVM target also needs the " + | ||
| "'$FLAKY_NAME_TOKEN' name token, which is the only selector those targets have.", |
There was a problem hiding this comment.
Looks like a valid point.
| * expects the conventional `src/<name>` directories. | ||
| */ | ||
| private fun nonJvmTestFunctions(): List<KoFunctionDeclaration> = Konsist | ||
| .scopeFromProject() |
There was a problem hiding this comment.
Does this Konsist.scopeFromProject() produces a scope only for this project? Will it cover other modules?
There was a problem hiding this comment.
it covers every module, not just ktor-test-base
| } | ||
|
|
||
| jvmTest.dependencies { | ||
| // Enforces the `@Flaky` / `_flaky` naming convention across the repository's sources. |
There was a problem hiding this comment.
I think we can drop this comment
| val mode = flakyTestsMode() | ||
|
|
||
| tasks.withType<AbstractTestTask>().configureEach { | ||
| if (name !in ANNOTATION_DRIVEN_TEST_TASKS) selectFlakyTests(mode) |
There was a problem hiding this comment.
Btw, what if change this check to if (this is Test) and configure the property here, while in the else branch call the selectFlakyTests? This would remove applying the flaky test mode from JvmConfig.kt.
There was a problem hiding this comment.
Tried this, but is Test doesn't discriminate correctly here: Android host unit tests are Test tasks too, but JvmConfig only wires useJUnitPlatform()/autodetection (and therefore FlakyTestCondition) onto ANNOTATION_DRIVEN_TEST_TASKS. Branching on type instead of name would route Android host tests into the "set the property" arm with no condition installed to read it, silently un-quarantining flaky tests there, which is why this ended up name-based.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3ac5f4f19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
… retry
Add `@Flaky(ticket)` with two selectors driven by one `ktor.tests.flaky`
property: a JUnit `ExecutionCondition` on JVM and a `_flaky` name token
filtered by Gradle on the targets that have no JUnit Platform, plus a
JVM-only `flakyTest` task that runs the quarantined tests so they stay
reported instead of disappearing behind `@Ignore`.
The two selectors are kept apart per target rather than stacked: applying the
name filter to JVM tasks as well would intersect it with the annotation
condition, so `only` would run just the tests that are both annotated and
named `*_flaky*` and silently skip every `@Flaky` test carrying no token.
JVM test tasks therefore get the mode as a system property and no name
filter, `stressTest` included.
Take the classpath of `flakyTest` and `stressTest` from the JVM test
compilation instead of the `jvmTest` task. A provider derived from
`tasks.named("jvmTest")` carries that task as its producer, so both tasks ran
the entire default suite before doing any work.
Stop retrying deterministic failures: `DeterministicFailureGuard` classifies
an `AssertionError` thrown by the test body as non-retryable, so it is
reported by the first attempt instead of being masked as flakiness.
`runTest`'s own timeout error stays retryable, being thrown around the body
rather than by it.
Add `assertEventually` as a real-time polling helper for liveness asserts,
so they no longer check state the instant a call returns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Share the flaky selectors between the two builds. `ktor-test-constants` holds the name token, the property name and the mode enum, and is compiled into build-logic as a source directory rather than a project dependency: it is built by a convention plugin that build-logic itself produces, so including it as a project would be circular. It takes its own package because JPMS rejects a package split across two modules. Make `@Flaky` inherited through `@JvmInherited`, an expect annotation aliasing `java.lang.annotation.Inherited` on the JVM and doing nothing elsewhere. JUnit walks superclasses only for inherited annotations, so this replaces the manual hierarchy walk in `FlakyTestCondition`. Skip `flakyTest` where `FlakyTestCondition` isn't on the test runtime classpath. Selection there is by annotation, so a module without it had nothing deselecting its ordinary tests and ran its whole suite with `ignoreFailures` hiding the result. Such a module can't carry @flaky at all, the annotation shipping in the same module, so skipping is the right outcome. Give `assertEventually` its own `EventuallyTimeoutException` instead of failing with an `AssertionError`. `DeterministicFailureGuard` classifies assertion errors as deterministic and stops retrying, which is wrong for a wait that ran out of time: that is the load-dependent failure retries exist for. Add a Konsist check that `@Flaky` and the `_flaky` token agree outside `jvm/test`, where only one of the two selectors would otherwise apply. Source sets are matched by path because the repository uses the flattened layout, and the annotation by simple name because Konsist resolves `hasAnnotationOf` through imports and would miss same-package uses. Also read the flaky mode once per condition instance, drop the redundant `TestResult` return types, and extract the shared retry scaffolding of the `BaseTest.runTest` actuals into `runTestAttempts`. # Conflicts: # gradle/libs.versions.toml
`retryTest` retried anything thrown, so `BaseTest` had no way to say "stop" and
faked it: after a deterministic failure it re-threw the recorded error at the top
of every remaining attempt, spinning through the loop to reach a verdict the
first attempt already produced. `runTestWithData` already avoided that by
deciding inside its `recover` callback, so the same guard had two shapes.
Give the loop a `shouldRetry` predicate instead, defaulted to retrying
everything, and let `BaseTest` pass `{ !guard.hasFailure }`. `failFast` then has
no callers and is gone.
Classification still happens inside the `runTest` body, where it has to: the
timeout `runTest` raises is `UncompletedCoroutinesError`, itself an
`AssertionError`, and it is thrown around the body rather than by it. Only code
wrapped directly around the body can tell the two apart, so the guard records
there and the predicate merely reads the verdict.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…Exception` for improved coroutine debugging and consistency. Simplify `assertEventually` to suspend on retryable conditions. Refactor `Flaky` annotations and test tasks for streamlined flaky-test handling.
d3ac5f4 to
9b3cc02
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b3cc025c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| .scopeFromProject() | ||
| .files | ||
| .filter { "/test/" in it.path && "/jvm/test/" !in it.path } | ||
| .flatMap { it.functions(includeNested = true) } |
There was a problem hiding this comment.
When @Flaky is applied to a common test class containing setup or private helper functions, includeNested = true collects those non-test functions and isFlaky() inherits the class annotation for each of them. The assertion then requires every helper name to end in _flaky, even though Gradle and JUnit never select those helpers as tests, so a valid class-level quarantine causes FlakyConventionTest to fail. Filter this collection to actual test declarations and account for the token on an enclosing test class.
Useful? React with 👍 / 👎.
| // Inherited so that a test class extending an annotated base class is quarantined too — the shared | ||
| // test suite pattern used across Ktor. JUnit walks superclasses only for inherited annotations. | ||
| @JvmInherited | ||
| annotation class Flaky(val ticket: String) |
There was a problem hiding this comment.
When a test uses @Flaky("") or any arbitrary string, the annotation and convention test accept it, and the execution condition quarantines the test without a usable tracking issue. This defeats the stated requirement that every quarantine link a YouTrack issue; validate the annotation argument, at least for nonblank ticket-shaped values, in the repository convention check.
Useful? React with 👍 / 👎.
| * stays retryable because it is thrown around the body rather than by it. | ||
| * Used by [runTestWithData] and by `BaseTest.runTest`. | ||
| */ | ||
| class DeterministicFailureGuard { |
There was a problem hiding this comment.
DeterministicFailureGuard exists solely to coordinate the internal retry plumbing in runTestWithData and BaseTest, but this declaration exposes it as an unrestricted public API. Since cross-module use prevents simply making it internal, mark it with @InternalAPI or hide it behind an intended abstraction so consumers do not treat this mutable implementation detail as stable API.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
…oss flaky-test handling and retry utilities. Apply `InternalAPI` annotations where required.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c17a9a63a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| * @property data The input data for the test case. | ||
| * @property retry The current retry attempt number for this test case. `0` means the initial test run before retries. | ||
| */ | ||
| data class TestCase<T>(val data: T, val retry: Int) |
There was a problem hiding this comment.
This change removes documentation from TestCase, TestFailure, TestSuccess, and TestExecutionResult while leaving all four public. These are the result types exposed by runTestWithData, so deleting their property and semantic KDoc makes the public API violate the repository's explicit documentation requirement; restore the removed KDoc, or make the types internal if they are not intended API.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
| interval: Duration = (timeout / 20).coerceIn(1.milliseconds, 100.milliseconds), | ||
| condition: () -> Boolean, | ||
| ) { | ||
| if (condition()) return |
There was a problem hiding this comment.
When the predicate performs blocking work—for example, Thread.sleep(1_000) on JVM with timeout = 100.milliseconds—this initial call occurs before withTimeout, so the function cannot time out until the predicate returns; subsequent calls are likewise not preemptible because the lambda is non-suspending. The documented timeout is therefore not an upper bound, and a stuck predicate can hang indefinitely. Fresh evidence relative to the earlier report is that changing condition to non-suspending does not prevent blocking predicates; either explicitly require a quick nonblocking predicate or evaluate it in a form the deadline can bound.
Useful? React with 👍 / 👎.
…unTestWithData` and `Eventually`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb68054fbc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| try { | ||
| block() |
There was a problem hiding this comment.
When block launches a structured child that throws an AssertionError after the block returns, runAttempt waits for that child only after this try has exited, so the guard never records the deterministic failure and retryTest retries it. The same placement around test(testCase) in runTestWithData has this behavior, allowing a later attempt to mask assertion failures from child coroutines; classify the completed attempt's failure while still exempting the surrounding test timeout.
Useful? React with 👍 / 👎.
| private fun nonJvmTestFunctions(): List<KoFunctionDeclaration> = Konsist | ||
| .scopeFromProject() | ||
| .files | ||
| .filter { "/test/" in it.path && "/jvm/test/" !in it.path } |
There was a problem hiding this comment.
_flaky tests
When a test under jvm/test is named with the _flaky token but accidentally lacks @Flaky, this blanket path exclusion prevents the inverse convention check from seeing it. Because jvmTest deliberately receives no name filter, that test remains in the default suite, while flakyTest disables it as non-annotated; exempt JVM-only sources only from the requirement to add the name token, not from the requirement that an existing token have @Flaky.
Useful? React with 👍 / 👎.
| ignoreFailures = true | ||
| outputs.upToDateWhen { false } | ||
| systemProperty(FLAKY_MODE_PROPERTY, FlakyTestsMode.ONLY.propertyValue) | ||
| exclude("**/*StressTest*") |
There was a problem hiding this comment.
When a *StressTest is annotated @Flaky, the default stressTest task disables it because its forwarded mode is exclude, jvmTest already excludes stress classes, and this exclusion removes it from flakyTest before FlakyTestCondition can select it. Such a quarantined stress test therefore disappears from all standard reporting tasks; allow annotated stress tests into this task or provide an equivalent dedicated quarantine task.
Useful? React with 👍 / 👎.
Subsystem
Test Infrastructure —
build-logic,ktor-test-base(common,jvm),ktor-server-test-base(
common,jvm,nonJvm).Motivation
KTOR-9796, part of the epic KTOR-9789.
A 28-day cross-source analysis found ~840 flaky and 619 failed test-executions across ~4,091 builds.
They collapse into ~8 cross-cutting patterns rather than ~50 independent bad tests, and two habits kept them alive:
@Ignore("flaky")— the test disappears from every report, so nobody learns whether it stillflips or whether the underlying bug got fixed. Several had been disabled for over a year.
retries = 3..10— retries apply to any failure, so a deterministic assertionfailure is re-run and eventually reported as "flaky", masking a real bug.
There was also no shared primitive for "wait until", so liveness assertions checked state the instant
a call returned.
This is the infrastructure half of #5833, extracted on review request so it can be reviewed and
merged ahead of the per-test changes. It adds primitives and build wiring only — no test is
quarantined by this PR, so
flakyTestcurrently selects just the three self-test fixtures.Solution
Covers the five work items of KTOR-9796.
1. Transient-only retry
DeterministicFailureGuardclassifies anAssertionErrorthrown by the test body as deterministic:it is reported by the first attempt instead of being retried.
runTest's own timeout error staysretryable, since it is thrown around the body rather than by it. Wired into
runTestWithDataandinto
BaseTest.runTeston both JVM and non-JVM.DEFAULT_RETRIESis unchanged (0on JVM,1elsewhere).
2.
@Flaky(ticket)New annotation in
ktor-test-base, requiring a YouTrack issue. Flaky tests are excluded from thedefault run but still executed and reported by a dedicated task, so they stay visible. Enforced
on JVM by
FlakyTestCondition(anExecutionConditionmodelled on the existingStressTestCondition), auto-registered through JUnit extension autodetection so@Flakyworks oncommon-source tests without a per-test
@ExtendWith.3. Cross-platform selection by name
Native/JS/Wasm have no JUnit Platform, so selection there uses a
_flakyname token, applied byFlakyTestsConfigto everyAbstractTestTask.FlakyNameConventionTestguards the convention.4.
flakyTesttaskJVM-only, pins the mode to
only, and setsignoreFailures = true: a quarantined test flipping isthe expected outcome, not a regression to block on. Failures still land in the reports and in
Develocity, which is where the flip rate is tracked.
Both selectors are driven by one property,
ktor.tests.flaky, with the same three values as aGradle property and as the system property JVM test tasks forward to the condition:
./gradlew :module:jvmTest./gradlew :module:jsNodeTest -Pktor.tests.flaky=only./gradlew :module:macosArm64Test -Pktor.tests.flaky=all./gradlew :module:flakyTest@Flakyonly, JVM (pinsonly)An unrecognized value fails at configuration time with the accepted values listed.
5.
assertEventuallyA real-time-delay polling helper in
ktor-test-base, available to client and server tests alike,with
DEFAULT_EVENTUALLY_TIMEOUT = 10s.waitForConditionis left in place here; its call sitesmigrate in the follow-up PR.
Known follow-ups (kept out to keep this reviewable, all raised in #5833 review)
Cache the system property in
FlakyTestCondition; express@Flakyasexpect/actualor add a@JvmInheritedalias instead of walking the superclass chain by hand; use Konsist for the namingrule; share
FLAKY_NAME_TOKEN/FlakyTestsModebetween build-logic andktor-test-basevia anincluded module;
outputs.upToDateWhen { false }onflakyTest; extract the JVM/nonJvmBaseTestduplication; and an engine-scoped
clientTests(flaky = ...)so a flake confined to one engine nolonger needs a duplicated test.