From 2b0622b0d6a5155c0b2980bb5cffb0fe8763b164 Mon Sep 17 00:00:00 2001 From: Serhii Vecherenko Date: Mon, 14 Sep 2026 16:57:55 -0700 Subject: [PATCH] test: stop sampling a live animation in the dropdown fixed-point checks `Tests (Cargo.toml profile) (macos-latest)` went red on master with a menu measured 158px tall where the line before had measured 157, on a commit that changed no Rust. The profile was innocent too: the cause is the wall clock. The panel is wrapped in `entering_zoom(Motion::POPOVER_IN)`, which reproduces v3's `zoom-in-90` by growing the panel's own vertical padding from 90% to 100% over 150ms, so the menu is a different height on every frame while it runs. gpui drives a oneshot `Animation` from `scheduler::Instant` -- the real clock -- and neither `run_until_parked` nor `advance_clock` moves it, so `settle` never settled the zoom. It sampled it wherever the clock happened to be. Locally the test finishes inside 10ms and every sample lands at 157; a runner that stalls about 50ms between two samples gets 157 and then 158. Injecting a 30ms stall into the unmodified test reproduces the CI assertion exactly, origin and all. So there is no geometry defect: the resting panel is a fixed point at 158px, and the height change is the animation doing its job. What was wrong is that assertions about the positioner were reading a surface that was still moving, and passing on timing luck. Every host in the file now opens with the entry zoom off through the public `MenuStyle` theme default -- the same switch an application would use, and the motionless layout is the animated resting layout. The fixed-point samples are now taken across 30ms of real time, which strengthens them: they mean "nothing moves on its own" rather than "nothing moved within a microsecond". No assertion was loosened, ignored or profile-gated. `the_entry_zoom_lands_on_the_motionless_layout` keeps the stock theme, waits the zoom out, and pins both halves -- a finished zoom is a fixed point, and it converges on exactly the motionless layout. Mutation-tested both ways: leaving the zoom running fails 8 tests deterministically, and a `POPOVER_IN` that never settles fails the new one with the original 158-against-157. Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/dropdown_viewport_deep.rs | 203 +++++++++++++++--- docs/agents/components.md | 11 + docs/parity/coverage-report.json | 4 +- docs/parity/coverage-report.md | 2 +- docs/parity/interaction-inventory.json | 6 +- 5 files changed, 187 insertions(+), 39 deletions(-) diff --git a/crates/herogpui-components/tests/dropdown_viewport_deep.rs b/crates/herogpui-components/tests/dropdown_viewport_deep.rs index d4e392d..bda2d13 100644 --- a/crates/herogpui-components/tests/dropdown_viewport_deep.rs +++ b/crates/herogpui-components/tests/dropdown_viewport_deep.rs @@ -34,6 +34,7 @@ use gpui::{ VisualTestContext, }; use herogpui_components::{Button, Dropdown, MenuItem, Placement}; +use herogpui_theme::{set_theme, ComponentTheme, ComponentThemes, MenuStyle, Theme}; use harness::{click, open_host}; @@ -47,11 +48,69 @@ fn near(a: Pixels, b: f32) -> bool { (f32::from(a) - b).abs() < 1.5 } +/// Opens one host window with the Dropdown menu's entry zoom turned off. +/// +/// Every assertion in this file is about the positioner -- where the panel +/// lands and how big the viewport lets it be -- and none of it is about +/// motion. The entry zoom has to go because it *moves the panel's layout box*: +/// `Motion::POPOVER_IN` reproduces v3's `zoom-in-90` by growing the panel's own +/// vertical padding from 90% to 100% over 150ms (`anim::ZoomBox::panel`), so +/// the menu is genuinely a different height on every frame while it runs. +/// +/// That would merely be noisy if the harness could step it, but it cannot: +/// gpui drives a oneshot `Animation` from `scheduler::Instant` -- the real +/// wall clock -- and neither `run_until_parked` nor +/// `executor().advance_clock` moves it. [`settle`] therefore never settles the +/// zoom; it just samples it wherever the wall clock happens to be, which is +/// why the fixed-point assertions below used to depend on how fast the machine +/// running them was. On this repository's `Tests (Cargo.toml profile)` runner +/// one sample landed at 157px and the next at 158px, and the run went red on +/// code that had not changed. +/// +/// `Dropdown` composes its `Menu` internally and never sets the instance flag, +/// so the switch is the public `MenuStyle` theme default, which is exactly the +/// path an application would use to opt out of the motion. +fn open_menu_host( + cx: &mut TestAppContext, + content: impl Fn() -> gpui::AnyElement + 'static, +) -> &mut VisualTestContext { + let cx = open_host(cx, content); + stop_entry_motion(cx); + cx +} + +/// The stock theme with the Menu's entry zoom switched off, and nothing else +/// touched: every other `MenuStyle` field stays `None`, so the component's own +/// defaults still resolve. +fn motionless_menu_theme() -> Theme { + Theme::builder("no-entry-motion", Theme::light()) + .components(ComponentThemes::default().menu(ComponentTheme::new( + MenuStyle::default().animate_entry(false), + ))) + .build() +} + +/// Switches the entry zoom off on an already-open window. Applying it while a +/// panel is on screen is deliberate in +/// [`the_entry_zoom_lands_on_the_motionless_layout`]: it is how that test +/// compares the converged layout with the motionless one without rebuilding +/// the window under it. +fn stop_entry_motion(cx: &mut VisualTestContext) { + cx.update(|window, cx| { + set_theme(motionless_menu_theme(), cx); + window.refresh(); + }); + cx.run_until_parked(); +} + /// Sizes the window, then lets the measured position settle. /// /// The correction is measured rather than predicted, so the frame that first /// lays the menu out is the one that reports its size. These extra frames /// prove the position is stable rather than papering over a wobble. +/// +/// They can only prove it because [`open_menu_host`] took the entry zoom out: +/// these frames cost no wall-clock time the animation would notice. fn settle(cx: &mut VisualTestContext, width: f32, height: f32) { cx.simulate_resize(size(px(width), px(height))); for _ in 0..4 { @@ -60,13 +119,32 @@ fn settle(cx: &mut VisualTestContext, width: f32, height: f32) { } } +/// The real-time gap every fixed-point sample in this file is taken across. +/// +/// The gap is the whole point. gpui drives a oneshot `Animation` from +/// `scheduler::Instant` -- the wall clock -- so a surface that is still +/// animating reports a different geometry on every real millisecond, while +/// three back-to-back frames of it can look perfectly stable because no +/// measurable time passed between them. Sampling across a gap is what turns +/// "nothing changed" into "nothing moves on its own", which is the property +/// these assertions are actually for. 30ms is a tenth of the longest overlay +/// entry in `anim::Motion`, so anything still running is well clear of a +/// rounded pixel by the third sample. +const FIXED_POINT_GAP: std::time::Duration = std::time::Duration::from_millis(30); + +/// One frame, taken `FIXED_POINT_GAP` of real time after the last one. +fn refresh_after_a_gap(cx: &mut VisualTestContext) { + std::thread::sleep(FIXED_POINT_GAP); + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); +} + /// The position must be a fixed point: refreshing without input changes /// nothing, or the panel would visibly oscillate. fn assert_settled(cx: &mut VisualTestContext, selector: &'static str) { let bounds = cx.debug_bounds(selector).unwrap(); for _ in 0..3 { - cx.update(|window, _| window.refresh()); - cx.run_until_parked(); + refresh_after_a_gap(cx); assert_eq!(cx.debug_bounds(selector).unwrap(), bounds); } } @@ -113,32 +191,35 @@ fn plain_items(count: usize) -> Vec { /// A trigger pushed to the right of a `pad`-wide spacer, so the menu it opens /// would hang off the window's right edge. +fn trigger_at(pad: f32, placement: Placement) -> gpui::AnyElement { + gpui::div() + .flex() + .flex_row() + .items_start() + .child(gpui::div().w(px(pad)).h(px(1.))) + .child( + gpui::div() + .debug_selector(|| "dd-trigger".to_owned()) + .child( + Dropdown::uncontrolled( + "ddv", + Button::new("ddv-trigger").label("Merge"), + describing_items(), + ) + .id("dd-viewport") + .placement(placement), + ), + ) + .into_any_element() +} + +/// That trigger in a motionless host window. fn host_with_trigger_at( cx: &mut TestAppContext, pad: f32, placement: Placement, ) -> &mut VisualTestContext { - open_host(cx, move || { - gpui::div() - .flex() - .flex_row() - .items_start() - .child(gpui::div().w(px(pad)).h(px(1.))) - .child( - gpui::div() - .debug_selector(|| "dd-trigger".to_owned()) - .child( - Dropdown::uncontrolled( - "ddv", - Button::new("ddv-trigger").label("Merge"), - describing_items(), - ) - .id("dd-viewport") - .placement(placement), - ), - ) - .into_any_element() - }) + open_menu_host(cx, move || trigger_at(pad, placement)) } /// A trigger pushed down by a tall spacer, so a bottom-placed menu cannot fit @@ -148,7 +229,7 @@ fn host_with_trigger_low( spacer: f32, placement: Placement, ) -> &mut VisualTestContext { - open_host(cx, move || { + open_menu_host(cx, move || { gpui::div() .flex() .flex_col() @@ -178,7 +259,7 @@ fn host_with_trigger_mid( pad: f32, placement: Placement, ) -> &mut VisualTestContext { - open_host(cx, move || { + open_menu_host(cx, move || { gpui::div() .flex() .flex_col() @@ -296,13 +377,69 @@ fn a_centered_menu_tracks_the_viewport_when_resized(cx: &mut TestAppContext) { )); } for _ in 0..3 { - cx.update(|window, _| window.refresh()); - cx.run_until_parked(); + refresh_after_a_gap(cx); assert_eq!(cx.debug_bounds("dropdown-menu").unwrap(), menu); } } } +/// The other side of the trade [`open_menu_host`] makes: this one keeps the +/// entry zoom, lets it finish, and pins the two properties the rest of the +/// file then relies on -- that a finished zoom is a fixed point, and that it +/// lands on *exactly* the motionless layout rather than near it. +/// +/// Both matter. The zoom grows the panel's own vertical padding, so a curve +/// that stopped a hair short of 1.0, or an animator that left any residual, +/// would park the menu a fraction of a pixel away from its natural size +/// forever -- and a fraction of a pixel is all it takes to round to a +/// different height, which is the whole failure this file's idempotence +/// assertions exist to catch. +/// +/// It sleeps because the clock the zoom reads is the real one: gpui measures a +/// oneshot `Animation` with `scheduler::Instant::elapsed`, which both +/// `run_until_parked` and `executor().advance_clock` leave alone. The wait is +/// one-sided -- past the duration the animation is done for good -- so a +/// slower machine only makes it more settled, never less. +#[gpui::test] +fn the_entry_zoom_lands_on_the_motionless_layout(cx: &mut TestAppContext) { + // `anim::Motion::POPOVER_IN` runs for 150ms; the rest is margin for a + // runner that stalls between the sleep and the frame after it. + const PAST_POPOVER_IN: std::time::Duration = std::time::Duration::from_millis(500); + + // Deliberately *not* `open_menu_host`: this window keeps the stock theme, + // so the menu animates in exactly as it does for a real user. + let cx = open_host(cx, move || trigger_at(420., Placement::Bottom)); + settle(cx, 1200., 600.); + click(cx, 460., 18.); + settle(cx, 1200., 600.); + + std::thread::sleep(PAST_POPOVER_IN); + cx.update(|window, _| window.refresh()); + cx.run_until_parked(); + let converged = cx + .debug_bounds("dropdown-menu") + .expect("the open menu must be laid out"); + + for _ in 0..3 { + refresh_after_a_gap(cx); + assert_eq!( + cx.debug_bounds("dropdown-menu").unwrap(), + converged, + "a finished entry zoom must leave the panel at a fixed point" + ); + } + + // Now take the zoom out from under the settled panel. Nothing may move: + // at rest the animated padding *is* the panel's natural padding, so the + // two layouts are the same layout. + stop_entry_motion(cx); + assert_eq!( + cx.debug_bounds("dropdown-menu").unwrap(), + converged, + "the entry zoom must land on the panel's natural geometry, not near it" + ); +} + /// An end-aligned menu is pinned by its *right* edge, so it overflows the /// opposite way. The correction has to push it right, which is the arm that /// keeps the shift on the `right` inset. @@ -646,7 +783,7 @@ fn a_tall_menu_caps_to_the_available_height_and_wheels_to_its_last_row(cx: &mut let height = 420.; let actions = harness::events(); let recorded = actions.clone(); - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { let recorded = recorded.clone(); gpui::div() .flex() @@ -745,7 +882,7 @@ fn a_tall_menu_caps_to_the_available_height_and_wheels_to_its_last_row(cx: &mut /// A short menu keeps its natural height: the cap is a maximum, not a size. #[gpui::test] fn a_short_menu_keeps_its_natural_height(cx: &mut TestAppContext) { - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { gpui::div() .flex() .flex_row() @@ -784,7 +921,7 @@ fn an_open_submenu_is_its_own_popover_and_stays_inside(cx: &mut TestAppContext) let width = 620.; let actions = harness::events(); let recorded = actions.clone(); - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { let recorded = recorded.clone(); gpui::div() .flex() @@ -882,7 +1019,7 @@ fn a_tall_submenu_near_a_low_row_caps_and_wheels_to_its_last_child(cx: &mut Test let (width, height) = (600., 500.); let actions = harness::events(); let recorded = actions.clone(); - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { let recorded = recorded.clone(); let mut items = plain_items(8); items.push( @@ -982,7 +1119,7 @@ fn a_click_in_the_submenu_gap_dismisses_without_action(cx: &mut TestAppContext) let recorded = actions.clone(); let opens = harness::events(); let opened = opens.clone(); - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { let recorded = recorded.clone(); let opened = opened.clone(); gpui::div() @@ -1070,7 +1207,7 @@ fn a_click_outside_with_a_submenu_open_dismisses_without_action(cx: &mut TestApp let recorded = actions.clone(); let opens = harness::events(); let opened = opens.clone(); - let cx = open_host(cx, move || { + let cx = open_menu_host(cx, move || { let recorded = recorded.clone(); let opened = opened.clone(); gpui::div() diff --git a/docs/agents/components.md b/docs/agents/components.md index 2c3757e..1b12ae7 100644 --- a/docs/agents/components.md +++ b/docs/agents/components.md @@ -229,6 +229,17 @@ Harness rules: and remeasure after error content changes layout. - Prove a closed surface by clicking where its row used to be and asserting no callback, not merely by checking an open-change callback. +- gpui drives a oneshot `Animation` from `scheduler::Instant`, the **real** + clock. Neither `run_until_parked` nor `executor().advance_clock` moves it, so + a surface whose entry motion changes its layout box — an `anim::ZoomBox` that + grows a panel's own padding, for instance — keeps resizing for the whole + duration, and back-to-back frames only look stable because no measurable time + passed between them. Assert geometry with the motion switched off (the + component's `animate_entry`, or its theme default where the composing + component owns the instance) and sample any fixed point across a real-time + gap. `harness::still()` is *not* the lever here: reduced motion also removes + `anim::pressed`'s slot, which changes the intrinsic width of the surrounding + layout. An intentionally failing test is useful only after its expectation is checked against the exact upstream contract. Correct the expectation when it encodes diff --git a/docs/parity/coverage-report.json b/docs/parity/coverage-report.json index c324fd8..8a83c65 100644 --- a/docs/parity/coverage-report.json +++ b/docs/parity/coverage-report.json @@ -3,8 +3,8 @@ "target": "v3.2.5", "target_commit": "5f13f6ed355bdbd5d5f69e5944685438a3591793", "inventory_recorded_on": "2026-09-14", - "inventory_verification_sha256": "e143b05d7b68e94ae98ef557abb2e007005b16f0582dc764e9721bd1e255d57c", - "source_snapshot_sha256": "4a6c503edb52b99355c4c56cf881a029cd121a7dea8b7995d9b40aa7830652f8", + "inventory_verification_sha256": "7e9338983d2c4fb9e3dd40779e6c102dcf4d3219f8a6d8bd83f7db803e665ef8", + "source_snapshot_sha256": "ec373a2aa991362a4a93c321769ed47a7b6e6773e94cd002ea115276e37ad7ff", "last_evidence_commit": null, "last_evidence_commit_recorded": false, "totals": { diff --git a/docs/parity/coverage-report.md b/docs/parity/coverage-report.md index 519e62a..c10feca 100644 --- a/docs/parity/coverage-report.md +++ b/docs/parity/coverage-report.md @@ -3,7 +3,7 @@ Target: `v3.2.5` (`5f13f6ed355bdbd5d5f69e5944685438a3591793`) Inventory date: `2026-09-14` -Inventory verification: `e143b05d7b68e94ae98ef557abb2e007005b16f0582dc764e9721bd1e255d57c` +Inventory verification: `7e9338983d2c4fb9e3dd40779e6c102dcf4d3219f8a6d8bd83f7db803e665ef8` Last evidence commit: `(none recorded)` This is a maintainer work-queue summary. It does not turn gallery section headings, static audits, or successful builds into parity evidence. diff --git a/docs/parity/interaction-inventory.json b/docs/parity/interaction-inventory.json index eb21878..4b0c962 100644 --- a/docs/parity/interaction-inventory.json +++ b/docs/parity/interaction-inventory.json @@ -16913,7 +16913,7 @@ "sha256": "99816a71d82bb9d62f9b86bf6f61ec67566cd63e478735f22dd5ba5c97784291" }, "source_snapshot": { - "sha256": "4a6c503edb52b99355c4c56cf881a029cd121a7dea8b7995d9b40aa7830652f8", + "sha256": "ec373a2aa991362a4a93c321769ed47a7b6e6773e94cd002ea115276e37ad7ff", "files": { ".cargo/config.toml": "656172d7bc0b7cda27bccdffaf87ca34d6c1b3857e3c95f989d9dc164f3395be", ".gitattributes": "77b98c2399fed202e770babce5b84b8c24e1e2b5e4ac94c5f1ce3ba1981d446f", @@ -17093,7 +17093,7 @@ "crates/herogpui-components/tests/dropdown_anatomy_deep.rs": "33ab1aa7aa9cc06e8e33f0e72c2602c86d39591f357780b67460002086f8c1bf", "crates/herogpui-components/tests/dropdown_close.rs": "074683c2d46a1faa46e3c22d0948148560fa8a05165825d4042fc403dba3fb42", "crates/herogpui-components/tests/dropdown_seed_deep.rs": "0fc190ad5141f66d379faa12ed58cdc942b64f1b8dbed4543233fd159493074f", - "crates/herogpui-components/tests/dropdown_viewport_deep.rs": "6083d16d47c28bb7e28323ecba9007bf2210f0b994f99a5d737b5df7fd146242", + "crates/herogpui-components/tests/dropdown_viewport_deep.rs": "4642a657d57885fab9b240454dad756c522262864302c8e2d5e861d397c03712", "crates/herogpui-components/tests/feedback.rs": "662a56a57506dc443bde96336cf520ebe41ee447881a7728f062e5f991619808", "crates/herogpui-components/tests/feedback_compose.rs": "f4a728bfc5fbeff17fab9a20cf94c941096c807562aa3e45cc3c90909b363ff0", "crates/herogpui-components/tests/field_keyboard_contracts.rs": "eecb655b1a25466e8805acb2c5c43ecc0ad30391e6ac9dd5be74d73455fb9142", @@ -45383,7 +45383,7 @@ } } ], - "verification_sha256": "e143b05d7b68e94ae98ef557abb2e007005b16f0582dc764e9721bd1e255d57c", + "verification_sha256": "7e9338983d2c4fb9e3dd40779e6c102dcf4d3219f8a6d8bd83f7db803e665ef8", "retired_source_changes": [], "retired_upstream_demos": {}, "upstream_source_snapshot": {