From 5382a1b02ba6c14235fcf7feb601a3766929714f Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Sun, 16 Aug 2026 13:34:16 +0900 Subject: [PATCH 1/4] fix(dart_async): assert lower time bounds only for async timing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #139 (and previously #105, #123). The `sleep` test is the one that trips most often. Root cause (per #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 #139. --- fixtures/dart_async/test/futures_test.dart | 28 +++++++++++++++------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/fixtures/dart_async/test/futures_test.dart b/fixtures/dart_async/test/futures_test.dart index 3af14e0..0648784 100644 --- a/fixtures/dart_async/test/futures_test.dart +++ b/fixtures/dart_async/test/futures_test.dart @@ -8,6 +8,16 @@ Future measureTime(Future Function() action) async { return end.difference(start); } +// Timing assertions below check a LOWER bound only: that an async operation +// waited at least its expected delay (proving the async plumbing actually +// suspends). They deliberately do not assert an upper bound — wall-clock upper +// bounds measure host/CI scheduling speed, not binding correctness, and are the +// source of the flaky failures tracked in #139. This mirrors uniffi-rs's own +// futures fixture (`test_futures.py`), which uses `assertGreater` with no upper +// bound. Verified locally: the whole suite runs in ~9s with correct results; on +// a loaded CI runner it can take ~200s, which is what tripped the old `< 300ms` +// style bounds. + class ErroringAsyncParser extends AsyncParser { @override Future asString(int delayMs, int value) async => value.toString(); @@ -73,7 +83,7 @@ void main() { await sleep(ms: 200); }); - expect(time.inMilliseconds > 200 && time.inMilliseconds < 300, true); + expect(time.inMilliseconds > 200, true); }); test('sequential_future', () async { @@ -83,7 +93,7 @@ void main() { expect(resultAlice, 'Hello, Alice!'); expect(resultBob, 'Hello, Bob!'); }); - expect(time.inMilliseconds > 300 && time.inMilliseconds < 400, true); + expect(time.inMilliseconds > 300, true); }); test('concurrent_future', () async { @@ -97,7 +107,7 @@ void main() { expect(results[1], 'Hello, Bob!'); }); - expect(time.inMilliseconds >= 200 && time.inMilliseconds <= 300, true); + expect(time.inMilliseconds >= 200, true); }); test('with_tokio_runtime', () async { @@ -105,7 +115,7 @@ void main() { final resultAlice = await sayAfterWithTokio(ms: 200, who: 'Alice'); expect(resultAlice, 'Hello, Alice (with Tokio)!'); }); - expect(time.inMilliseconds > 200 && time.inMilliseconds < 300, true); + expect(time.inMilliseconds > 200, true); }); test('fallible_function_and_method', () async { @@ -155,7 +165,7 @@ void main() { ); // calls the waker a second time after 1s await sleep(ms: 200); // wait for possible failure }); - expect(time.inMilliseconds >= 400 && time.inMilliseconds <= 600, true); + expect(time.inMilliseconds >= 400, true); }); test('udl_async_function', () async { @@ -190,7 +200,7 @@ void main() { final result = await megaphone.sayAfter(ms: 100, who: 'Alice'); expect(result, 'HELLO, ALICE!'); }); - expect(time.inMilliseconds >= 100 && time.inMilliseconds < 200, true); + expect(time.inMilliseconds >= 100, true); // Test async silence method final silenceTime = await measureTime(() async { @@ -218,7 +228,7 @@ void main() { final result = await megaphone.sayAfterWithTokio(ms: 100, who: 'Charlie'); expect(result, 'HELLO, CHARLIE (WITH TOKIO)!'); }); - expect(time.inMilliseconds >= 100 && time.inMilliseconds < 200, true); + expect(time.inMilliseconds >= 100, true); }); test('proc_macro_megaphone_fallible_method', () async { @@ -260,7 +270,7 @@ void main() { final result = await udlMegaphone.sayAfter(ms: 100, who: 'Dave'); expect(result, 'HELLO, DAVE (FROM UDL MEGAPHONE)!'); }); - expect(time.inMilliseconds >= 100 && time.inMilliseconds < 200, true); + expect(time.inMilliseconds >= 100, true); }); test('async_object_creation_functions', () async { @@ -291,7 +301,7 @@ void main() { ); expect(result, 'HELLO, EVE!'); }); - expect(time.inMilliseconds >= 100 && time.inMilliseconds < 200, true); + expect(time.inMilliseconds >= 100, true); }); test('fallible_struct_creation', () async { From 10cb1923050089c7605d056009c844cab0047c70 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Sun, 16 Aug 2026 13:56:05 +0900 Subject: [PATCH 2/4] style: drop redundant references in format!/println! args (clippy 1.97) `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. --- src/gen/callback_interface.rs | 2 +- src/gen/enums.rs | 2 +- src/gen/render/mod.rs | 2 +- src/gen/stream/mod.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/gen/callback_interface.rs b/src/gen/callback_interface.rs index b65b95b..357fd80 100644 --- a/src/gen/callback_interface.rs +++ b/src/gen/callback_interface.rs @@ -359,7 +359,7 @@ pub fn generate_callback_functions( // Generate the function body let callback_method_name = - &format!("{}{}", &DartCodeOracle::fn_name(callback_name), &DartCodeOracle::class_name(m.name())); + &format!("{}{}", DartCodeOracle::fn_name(callback_name), DartCodeOracle::class_name(m.name())); if m.is_async() { let completion_base = foreign_future_completion_name(m); diff --git a/src/gen/enums.rs b/src/gen/enums.rs index c181d4c..3ca0c57 100644 --- a/src/gen/enums.rs +++ b/src/gen/enums.rs @@ -36,7 +36,7 @@ impl CodeType for EnumCodeType { } fn ffi_converter_name(&self) -> String { - format!("FfiConverter{}", &DartCodeOracle::class_name(&self.id)) + format!("FfiConverter{}", DartCodeOracle::class_name(&self.id)) } } diff --git a/src/gen/render/mod.rs b/src/gen/render/mod.rs index 93bf68c..1ee2d60 100644 --- a/src/gen/render/mod.rs +++ b/src/gen/render/mod.rs @@ -57,7 +57,7 @@ pub trait Renderable { }; if !type_helper.include_once_check(&ty.as_codetype().canonical_name(), ty) { - println!("{} Added", &ty.as_codetype().canonical_name()); + println!("{} Added", ty.as_codetype().canonical_name()); } type_name diff --git a/src/gen/stream/mod.rs b/src/gen/stream/mod.rs index d5d2b7e..b2a41d9 100644 --- a/src/gen/stream/mod.rs +++ b/src/gen/stream/mod.rs @@ -8,7 +8,7 @@ pub fn generate_stream(obj: &Object, _type_helper: &dyn TypeHelperRenderer) -> d let obj_name = obj.name(); let fn_name = DartCodeOracle::fn_name(&obj_name.replace("StreamExt", "")); let obj_var_name = &DartCodeOracle::var_name(&fn_name); - let create_obj_fn_name = format!("createStream{}", &obj_name.replace("StreamExt", "")); + let create_obj_fn_name = format!("createStream{}", obj_name.replace("StreamExt", "")); quote! { $fn_name() async* { From 607e4cc61670aa7e7bb67c6f5613ae6d28cdb915 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Sun, 16 Aug 2026 14:28:56 +0900 Subject: [PATCH 3/4] test(dart_async): keep concurrency check via relative comparison; scope comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #139. --- fixtures/dart_async/test/futures_test.dart | 47 +++++++++++++++++----- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/fixtures/dart_async/test/futures_test.dart b/fixtures/dart_async/test/futures_test.dart index 0648784..0b027f7 100644 --- a/fixtures/dart_async/test/futures_test.dart +++ b/fixtures/dart_async/test/futures_test.dart @@ -8,15 +8,25 @@ Future measureTime(Future Function() action) async { return end.difference(start); } -// Timing assertions below check a LOWER bound only: that an async operation -// waited at least its expected delay (proving the async plumbing actually -// suspends). They deliberately do not assert an upper bound — wall-clock upper -// bounds measure host/CI scheduling speed, not binding correctness, and are the -// source of the flaky failures tracked in #139. This mirrors uniffi-rs's own -// futures fixture (`test_futures.py`), which uses `assertGreater` with no upper -// bound. Verified locally: the whole suite runs in ~9s with correct results; on -// a loaded CI runner it can take ~200s, which is what tripped the old `< 300ms` -// style bounds. +// The DELAY-based timing assertions below check a LOWER bound only: that an +// async operation waited at least its expected delay (proving the async +// plumbing actually suspends). They deliberately do not assert an upper bound — +// wall-clock upper bounds measure host/CI scheduling speed, not binding +// correctness, and are the source of the flaky failures tracked in #139. This +// mirrors uniffi-rs's own futures fixture (`test_futures.py`), which uses +// `assertGreater` with no upper bound. (Illustrative, not enforced: locally the +// whole suite runs in a few seconds with correct results; on a heavily loaded +// CI runner it can take vastly longer, which is what tripped the old two-sided +// per-operation bounds like `< 300ms`.) +// +// Two groups are intentionally different: +// - `concurrent_future` keeps a concurrency check, but as a *relative* +// comparison (concurrent run < sequential run) rather than a fragile +// absolute ceiling — load dilates both sides, so the inequality holds. +// - The immediate-operation checks (`always_ready`, `void`, sync methods, +// constructors) still assert an upper bound only. They have no lower bound +// to fall back on and share the same latent wall-clock fragility; tightening +// those is deliberately left for a separate change. class ErroringAsyncParser extends AsyncParser { @override @@ -97,7 +107,12 @@ void main() { }); test('concurrent_future', () async { - final time = await measureTime(() async { + // Run the same two delays concurrently and sequentially, then compare. + // A relative check (concurrent < sequential) verifies the futures actually + // overlap without a fragile absolute wall-clock ceiling: CI load dilates + // both measurements, so the inequality survives while an absolute `<= 300` + // would not. (Concurrent ≈ max(100, 200) = 200ms; sequential ≈ 300ms.) + final concurrentTime = await measureTime(() async { final results = await Future.wait([ sayAfter(ms: 100, who: 'Alice'), sayAfter(ms: 200, who: 'Bob'), @@ -107,7 +122,17 @@ void main() { expect(results[1], 'Hello, Bob!'); }); - expect(time.inMilliseconds >= 200, true); + final sequentialTime = await measureTime(() async { + await sayAfter(ms: 100, who: 'Alice'); + await sayAfter(ms: 200, who: 'Bob'); + }); + + // Lower bound: the longer of the two delays actually elapsed. + expect(concurrentTime.inMilliseconds >= 200, true); + // Concurrency: overlapping must be faster than summing the delays. If the + // binding regressed to serializing `Future.wait`, concurrentTime would rise + // to ~sequentialTime and this would fail. + expect(concurrentTime < sequentialTime, true); }); test('with_tokio_runtime', () async { From df52f35f01f2a85d377c26a7d5a00b43c30c35c5 Mon Sep 17 00:00:00 2001 From: Sasha Denisov Date: Mon, 17 Aug 2026 00:34:15 +0900 Subject: [PATCH 4/4] test(dart_async): verify concurrency structurally, not by comparing durations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #139. --- fixtures/dart_async/test/futures_test.dart | 53 ++++++++++++++-------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/fixtures/dart_async/test/futures_test.dart b/fixtures/dart_async/test/futures_test.dart index 0b027f7..7921e9d 100644 --- a/fixtures/dart_async/test/futures_test.dart +++ b/fixtures/dart_async/test/futures_test.dart @@ -20,9 +20,9 @@ Future measureTime(Future Function() action) async { // per-operation bounds like `< 300ms`.) // // Two groups are intentionally different: -// - `concurrent_future` keeps a concurrency check, but as a *relative* -// comparison (concurrent run < sequential run) rather than a fragile -// absolute ceiling — load dilates both sides, so the inequality holds. +// - `concurrent_future` verifies concurrency structurally (completion order), +// not by comparing durations — see the note in that test for why a timing +// comparison can't reliably catch a serialization regression. // - The immediate-operation checks (`always_ready`, `void`, sync methods, // constructors) still assert an upper bound only. They have no lower bound // to fall back on and share the same latent wall-clock fragility; tightening @@ -107,11 +107,17 @@ void main() { }); test('concurrent_future', () async { - // Run the same two delays concurrently and sequentially, then compare. - // A relative check (concurrent < sequential) verifies the futures actually - // overlap without a fragile absolute wall-clock ceiling: CI load dilates - // both measurements, so the inequality survives while an absolute `<= 300` - // would not. (Concurrent ≈ max(100, 200) = 200ms; sequential ≈ 300ms.) + // Comparing concurrent vs. sequential wall-clock time cannot reliably detect + // a serialization regression: a serialized `Future.wait` is structurally the + // same work as running the calls sequentially (100+200 either way), so the + // comparison becomes a coin flip, while any fixed margin or ratio re-adds the + // load-sensitive flakiness #139 is about (the Rust-side `thread::sleep` + // delays don't dilate under load, but scheduling jitter adds unbounded time). + // Instead assert two load-robust properties. + + // (1) Correct positional results + a lower bound. The lower bound is + // jitter-immune — sleeps only ever lengthen a run — and catches a future + // resolving before its delay elapsed. final concurrentTime = await measureTime(() async { final results = await Future.wait([ sayAfter(ms: 100, who: 'Alice'), @@ -121,18 +127,25 @@ void main() { expect(results[0], 'Hello, Alice!'); expect(results[1], 'Hello, Bob!'); }); - - final sequentialTime = await measureTime(() async { - await sayAfter(ms: 100, who: 'Alice'); - await sayAfter(ms: 200, who: 'Bob'); - }); - - // Lower bound: the longer of the two delays actually elapsed. - expect(concurrentTime.inMilliseconds >= 200, true); - // Concurrency: overlapping must be faster than summing the delays. If the - // binding regressed to serializing `Future.wait`, concurrentTime would rise - // to ~sequentialTime and this would fail. - expect(concurrentTime < sequentialTime, true); + expect(concurrentTime.inMilliseconds, greaterThanOrEqualTo(200)); + + // (2) Completion ORDER, not duration: start a slow future, then a fast one, + // and require the fast one to finish while the slow one is still pending. If + // the bindings serialized FFI futures, the slow call would have to complete + // before the fast one could even start, failing this deterministically — + // with ~2000ms of structural headroom (not a tuned threshold), so it can't + // reflake on a loaded runner. + var slowDone = false; + final slow = + sayAfter(ms: 2000, who: 'Slow').whenComplete(() => slowDone = true); + await sayAfter(ms: 1, who: 'Fast'); + expect( + slowDone, + isFalse, + reason: 'a 1ms future completed only after a 2000ms future that started ' + 'earlier — FFI futures appear to be serialized', + ); + await slow; // let the fixture call finish before the test ends }); test('with_tokio_runtime', () async {