He-Pin opened a new issue, #3696:
URL: https://github.com/apache/pekko/issues/3696
## Summary
`AGENTS.md` states clear test-determinism rules, but nothing in the build
enforces any of them, and known-flaky tests are tracked only as GitHub issues.
The result is measurable: 341 `Thread.sleep` calls in Scala sources, against a
rule that says "Avoid `Thread.sleep`".
A second, related problem is that the testkit's time-dilation contract is
inconsistent across three APIs and is documented only in Scaladoc prose, which
makes dilation mistakes an easy and recurring source of flakes.
## The enforcement gap
`AGENTS.md`, "Test Rules":
```
- Prefer deterministic tests over larger timeouts.
- Avoid `Thread.sleep`.
- Prefer probes, latches, stepped components, `within`, `remaining`, and
`remainingOrDefault`.
- Keep `expectNoMessage` short and intentional.
- Do not weaken assertions to hide production behavior.
- Do not fix flakes only by increasing timeouts.
```
The only style gate in CI is `build.sbt:44`:
```scala
addCommandAlias("checkCodeStyle", "scalafmtCheckAll; scalafmtSbtCheck;
javafmtCheckAll; +headerCheckAll")
```
Formatting and license headers — nothing inspects test sources for
non-deterministic patterns. Meanwhile:
```
rg -c 'Thread\.sleep' --type scala | awk -F: '{s+=$2} END {print s}'
341
```
Concentration (occurrences per file):
```
stream-tests/.../scaladsl/HubSpec.scala 18
docs/src/test/scala/docs/stream/cookbook/RecipeAdhocSource.scala 13
cluster/.../sbr/SplitBrainResolverSpec.scala 11
actor-tests/.../util/BoundedBlockingQueueSpec.scala 11
actor-tests/.../pattern/CircuitBreakerSpec.scala 11
actor-tests/.../actor/SchedulerSpec.scala 11
stream-tests/.../scaladsl/RestartSpec.scala 9
stream-tests/.../impl/TimeoutsSpec.scala 8
```
`pendingUntilFixed` appears 3 times.
### How flakiness is handled today
- **Retry, but still fail** — `project/PekkoDevelocityPlugin.scala:65-66`:
`.withMaxRetries(1)` and `.withFlakyTestPolicy(FlakyTestPolicy.Fail)`,
commented "preserve the original build outcome in case of flaky tests". Honest,
but it does not isolate anything.
- **Exclusion by category, not by test** —
`testkit/src/test/scala/org/apache/pekko/testkit/TestTags.scala:18-23` defines
`timing`, `long-running`, `performance`, `gh-exclude`, `gh-exclude-aeron`, and
CI passes `-Dpekko.test.tags.exclude=gh-exclude,timing`. A single flaky test in
a category cannot be excluded without excluding the whole category.
- **Issues, closed by elapsed time** — `CONTRIBUTING.md:41` describes the
`failed` label and states: "When the same problem isn't seen again over a
period of 6 months we assume it to be a rare flaky test or a problem that might
have since been fixed, so we close the issue until it pops up again." There is
no in-repo list, so nothing connects a known-flaky test to the code that is
allowed to exclude or retry it.
## The dilation contract is inconsistent
Three public testkit APIs treat a caller-supplied duration differently, and
the difference is only discoverable by reading Scaladoc or source.
**`TestKit.expectMsg(max, obj)` dilates** —
`testkit/src/main/scala/org/apache/pekko/testkit/TestKit.scala:456`:
```scala
def expectMsg[T](max: FiniteDuration, obj: T): T =
expectMsg_internal(max.dilated, obj)
```
**`TestKit.expectNoMessage(max)` does not** — `:722-724`, where the Scaladoc
carries the whole contract:
```scala
* Supplied value is not dilated.
...
def expectNoMessage(max: FiniteDuration) = {
```
**`stream-testkit` `expectNext(d)` does not** —
`stream-testkit/src/main/scala/org/apache/pekko/stream/testkit/StreamTestKit.scala:478-479`:
```scala
def expectNext(d: FiniteDuration): I = {
val t = probe.remainingOr(d)
```
The typed testkit splits the same way —
`actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/internal/TestProbeImpl.scala:161`
dilates:
```scala
override def expectMessage[T <: M](max: FiniteDuration, obj: T): T =
expectMessage_internal(max.dilated, obj)
```
while `:210-211` does not:
```scala
override def expectNoMessage(max: FiniteDuration): Unit =
expectNoMessage_internal(max)
```
Two concrete failure modes follow. A test that passes an already-dilated
duration to a dilating method gets dilation applied twice — with the CI setting
`-Dpekko.test.timefactor=2`
(`.github/workflows/build-test-prValidation.yml:93-94`) a 3s budget silently
becomes 12s. A test that passes a raw duration to a non-dilating method gets no
relief on a slow runner and flakes only in CI. Neither is visible at the call
site.
### A related surprise in `within`
`TestKit.scala:432-434` skips the upper-bound assertion when the block's
last assertion was `expectNoMessage`:
```scala
if (!lastWasNoMsg) {
assert(diff <= max_diff, s"block took ${format(_max.unit, diff)},
exceeding ${format(_max.unit, max_diff)}")
}
```
The typed testkit has the same mechanism, and documents the intent —
`TestProbeImpl.scala:91-95`:
```scala
/**
* if last assertion was expectNoMessage, disable timing failure upon
within()
* block end.
*/
private var lastWasNoMessage = false
```
This is deliberate (`expectNoMessage` itself consumes the budget, so the
assertion would always trip), but it means `within` gives no upper-bound
guarantee for such a block, and that is not discoverable from `within`'s own
Scaladoc.
## Proposal
Three parts, independently useful and separately mergeable:
1. **An in-repo known-flaky list.** One row per test: fully-qualified name,
issue link, date first seen, symptom class (timing / ordering / teardown / port
/ resource), and owner. Deriving it from the `failed` label would keep it
honest, and it gives the tag-exclusion mechanism something finer-grained than a
whole category to point at.
2. **A gate on newly-introduced non-deterministic patterns.** A CI check
that fails when a PR *adds* `Thread.sleep`, `pendingUntilFixed`, or a
`GHExcludeTest` tag to test sources, backed by a committed baseline file that
exempts the existing 341 occurrences. The baseline makes the existing debt
visible and bounded without requiring a big-bang cleanup, and it stops the
count from growing. This can start as a shell script over `rg` output before it
becomes an sbt task or a scalafix rule.
3. **Make the dilation contract explicit and checkable.** At minimum, state
on each affected method whether the supplied duration is dilated, in a
consistent wording, and cross-link them. Better: a lint that flags a caller
passing an already-`.dilated` duration into a dilating method, which is the
double-dilation case and is mechanically detectable.
Part 3 is documentation plus an optional lint and carries no
binary-compatibility risk. Parts 1 and 2 are additive CI artifacts. None of the
three changes production behaviour.
## Note on scope
Changing the dilation *behaviour* of any of these methods — for example
making `expectNoMessage` dilate for consistency — is deliberately **not**
proposed here. That would be a public API behaviour change affecting many
existing tests and would need its own discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
