Skip to content

fix(dart_async): assert lower time bounds only for async timing tests (#139) - #157

Merged
chavic merged 4 commits into
Uniffi-Dart:mainfrom
DenisovAV:fix-dart-async-flake
Aug 22, 2026
Merged

fix(dart_async): assert lower time bounds only for async timing tests (#139)#157
chavic merged 4 commits into
Uniffi-Dart:mainfrom
DenisovAV:fix-dart-async-flake

Conversation

@DenisovAV

@DenisovAV DenisovAV commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Fixes the flaky dart_async timing failures tracked in #139 (previously #105, #123).

Root cause (investigated, per #139's ask)

The timing tests asserted a narrow wall-clock window around each async delay — e.g. sleep(ms: 200) required time > 200 && time < 300. The upper bound measures host/CI scheduling speed, not binding correctness:

  • The binding is correct and fast. Locally the whole dart_async suite runs in ~9s with every test passing, including sleep.
  • On a loaded CI runner the same suite can take ~200s of wall-clock. The async operations still complete correctly and in the right order, but the elapsed-time upper bounds are exceeded.

So the failure is test design (narrow wall-clock assertions), not an async-runtime or generated-binding bug. sleep is simply the one that trips most often; across reruns it also jumps between the stable and 1.91 legs while nightly stays green — consistent with load, not a version issue.

Fix

For the delay-based tests, keep the lower bound (which proves the async plumbing actually suspended for the expected time) and drop the upper bound. This matches uniffi-rs's own futures fixture (test_futures.py), which asserts only assertGreater(elapsed, expected) with no upper bound.

9 delay assertions change from > LOW && < HIGH to > LOW; a comment documents the rationale.

Scope

This touches only the delay tests that have a lower bound. The immediate-operation checks (always_ready < 200, void <= 10, sync/constructor < N) share the same latent wall-clock fragility but have no lower bound to fall back on — leaving those for a separate decision rather than widening scope here.

Refs #139.

Update (review follow-up)

  • concurrent_future was the one test where the upper bound (<= 300) was load-bearing — it distinguished concurrent (~200ms) from a regression that serializes Future.wait (~300ms). Rather than drop it to a lower bound only (which would let a serialized regression pass), it now measures the same two delays both concurrently and sequentially and asserts concurrent < sequential — a relative comparison that survives CI load where an absolute ceiling doesn't.
  • Scope comment corrected: only the delay-based assertions became lower-bound-only; the immediate-operation checks (always_ready, void, sync methods, constructors) still assert an upper bound only and are a known, deliberately-deferred fragility.
  • A small clippy commit (clippy::useless_borrows_in_formatting, -D warnings on the stable Lints job) is included so this branch's CI is green — it removes redundant & in five format!/println! args, unrelated to the timing change but sharing the same job.

The `dart_async` timing tests asserted a narrow wall-clock window around each
async delay (e.g. `sleep(200ms)` required `> 200 && < 300`). The upper bound
measures host/CI scheduling speed rather than binding correctness, so under load
it fails on clean runs — the flakiness tracked in Uniffi-Dart#139 (and previously Uniffi-Dart#105,
Uniffi-Dart#123). The `sleep` test is the one that trips most often.

Root cause (per Uniffi-Dart#139, investigated rather than assumed):
- The binding is correct and fast. Locally the whole `dart_async` suite runs in
  ~9s with every test passing, including `sleep`.
- On a loaded CI runner the same suite can take ~200s of wall-clock; the async
  operations still complete correctly, but the elapsed-time upper bounds are
  exceeded. So the failure is test design (narrow wall-clock assertions), not an
  async runtime or generated-binding bug.

Fix: for the delay-based tests, keep the lower bound (which proves the async
plumbing actually suspended for the expected time) and drop the upper bound.
This matches uniffi-rs's own futures fixture (`test_futures.py`), which asserts
only `assertGreater(elapsed, expected)` with no upper bound.

Scope: this touches only the delay tests that have a lower bound. The immediate-
operation checks (`always_ready < 200`, `void <= 10`, sync/constructor `< N`)
share the same latent wall-clock fragility but have no lower bound to fall back
on; leaving those for a separate decision.

Refs Uniffi-Dart#139.
`clippy::useless_borrows_in_formatting` (denied via `-D warnings`) fails the
stable Lints job on `main`: `format!`/`println!` arguments that are already
`Display`/`Debug` don't need a leading `&`. Removes the redundant `&` at the
five sites clippy 1.97 flags (callback_interface, enums, render, stream).

Unblocks CI for this branch; unrelated to the timing-assertion change but the
two share the stable Lints job.
…pe comment

Review follow-up. Two fixes to the timing-assertion change:

- `concurrent_future` previously relied on its upper bound (`<= 300`) to prove
  the two `Future.wait` calls overlapped — dropping it to a lower bound only
  would let a regression that serializes the futures (~300ms) pass. Restore the
  concurrency check as a *relative* comparison: measure the same two delays
  concurrently and sequentially, assert concurrent < sequential. This survives
  CI load (both sides dilate) where the old absolute ceiling did not.

- Scope the header comment: only the delay-based assertions became lower-bound-
  only; the immediate-operation checks still assert an upper bound only and are
  a known, deliberately-deferred fragility. The old wording ("assertions below
  check a LOWER bound only") over-generalized. Also mark the ~seconds figures as
  illustrative rather than enforced.

Refs Uniffi-Dart#139.
@DenisovAV DenisovAV mentioned this pull request Aug 16, 2026
…urations

Review follow-up. The previous `concurrent < sequential` check could not
reliably catch a serialization regression: a serialized `Future.wait` does the
same work as running the two calls sequentially (100+200 either way), so the
comparison is a coin flip — while adding a fixed margin or ratio to detect it
would re-introduce the load-sensitive flakiness Uniffi-Dart#139 exists to remove (the
Rust-side thread::sleep delays don't dilate under load, but scheduling jitter
adds unbounded time).

Replace it with two load-robust assertions:
- a lower bound (jitter only lengthens a run, so `>= max(delay)` never flakes),
  now via the `greaterThanOrEqualTo` matcher so a failure prints the value; and
- a completion-ORDER check: a 1ms future started after a 2000ms future must
  finish while the slow one is still pending. A serialized binding would force
  the slow call to complete first, failing this deterministically with ~2000ms
  of structural headroom — a property, not a tuned threshold.

Drops the coin-flip sequential measurement (~300ms of suite time) in exchange.

Refs Uniffi-Dart#139.
spacebear21 pushed a commit that referenced this pull request Aug 21, 2026
Clippy 1.97 extended useless_borrows_in_formatting to flag these five
borrows, and CI runners now ship stable 1.97.1, so the Lints (stable)
job fails on main (see the run on #156, which flags these exact lines
without touching them). Verified: clippy 1.98.0 reports five
'redundant reference' errors on main and none with this change.

Display for &T forwards to Display for T, so output is byte-identical.

The same change rode along in #149, #152, #157 (and was reverted from
#150 when it could not be reproduced locally on an outdated toolchain).
Landing it once on main lets those branches rebase clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDnKiUL8NDSpeaPvyKJeoR

@chavic chavic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, thanks! This should un-red the other PRs too.

@DenisovAV

Copy link
Copy Markdown
Contributor Author

Thanks for the approval @chavic! 🙏 Since this un-reds the Test Suite (stable/1.91) failures on #149 and #152 (they're the same dart_async timing flake), would you be able to merge it when you get a chance? That clears one of the two red checks currently blocking #149.

@chavic
chavic merged commit 2c61298 into Uniffi-Dart:main Aug 22, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants