Use monotonic clock and inclusive lower bounds in dart_async test - #160
Merged
Conversation
chavic
changed the title
spacebear21
approved these changes
Aug 31, 2026
Comment on lines
+4
to
+8
| // Uses a monotonic Stopwatch, not DateTime.now(). DateTime.now() is the wall | ||
| // clock, which NTP can slew or step during a measurement on a CI runner, so an | ||
| // interval it reports can be shorter than the time that actually elapsed. That | ||
| // makes a lower-bound assertion fail even though the sleep ran for its full | ||
| // duration (#139). |
Collaborator
There was a problem hiding this comment.
This kind of comment belongs in the commit message, not a code comment (common frustrating pattern from the robots to overly explain things in comments...)
Contributor
Author
There was a problem hiding this comment.
Agreed, nice catch, I'll clean it up
Contributor
Author
There was a problem hiding this comment.
Agreed, nice catch, I'll clean it up
…wer bounds measureTime used DateTime.now(), which is the wall clock. NTP can slew or step the wall clock on a CI runner during a measurement, so the interval it reports can be shorter than the time that actually elapsed. Rust's thread::sleep never returns early, so a lower-bound failure such as the one on Uniffi-Dart#149 (sleep(200) measured as <= 200ms) can only come from the clock, not from the code under test. Stopwatch is monotonic and immune to this. Three lower-bound assertions also used a strict '>' where the rest of the file uses '>='. A sleep of exactly N ms truncates to N milliseconds and fails a strict bound, so they are now inclusive. Local measurements of sleep(200), five runs each with the release cdylib: uniffi 0.31.2 (main) 202.7-204.2ms, uniffi 0.32 (Uniffi-Dart#149) 202.5-203.0ms. The distributions are identical, which rules out a runtime change as the cause. Refs Uniffi-Dart#139. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDnKiUL8NDSpeaPvyKJeoR
chavic
force-pushed
the
fix-async-timing-clock
branch
from
September 2, 2026 08:41
8b07d78 to
1fd2728
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
measureTimenow uses a monotonicStopwatchinstead ofDateTime.now().>now use>=, like the rest of the file.Why
DateTime.now()is the wall clock. NTP can slew or step it on a CI runner during a measurement, so the interval it reports can be shorter than the time that actually elapsed. Rust'sthread::sleepnever returns early, so the lower-bound failure on #149 (sleep(200)measured as<= 200ms) can only come from the clock, not from the code under test.A sleep of exactly N ms also truncates to N milliseconds, which fails a strict bound. Two of the three strict bounds sit on a single sleep, so this is latent on any fast runner.
Refs #139.
🤖 Generated with Claude Code