From d8f86f19370adcb8a5ff00ea31f0defa8c354ecd Mon Sep 17 00:00:00 2001 From: Oz Date: Mon, 13 Jul 2026 21:55:17 +0000 Subject: [PATCH] Fix flaky diff_pipeline_computes_added_lines_and_ghost_blocks test The test was flaky because its synchronization could not deterministically wait for two background-thread hops: 1. Stale first DiffUpdated: reset_content seeds the diff base and triggers a base==new diff (task A) on the background executor; the subsequent apply_diffs aborts A and spawns the real diff (task B). The abort is best-effort (only effective on the next poll), and A's body has no yield_now (zero diff ops), so A can complete on its first poll before the abort is issued. When that happens A's callback fires a spurious first CodeEditorModelEvent::DiffUpdated for an *empty* diff. The test's oneshot fired on that first event, so expand_diffs ran before task B landed. 2. Background layout hop: ghost blocks are delivered via the render state's async layout channel, which spawn_stream_local polls on a *background* thread and forwards to a foreground task. The test's fixed 100x yield_now ghost poll only drives the foreground executor, so under load it could not reliably wait for the background diff round-trip (task B) plus the background layout hop. Fix (test-only synchronization): - Fire the oneshot only once the diff state actually reflects a change (added_or_changed_lines non-empty), checked inside the subscription callback. This ignores the stale empty DiffUpdated and signals on the real post-edit diff, so expand_diffs runs after task B. - Replace the fixed-iteration ghost poll with RenderState::layout_complete(), which awaits the outstanding_layouts counter until every submitted layout action (including the LayoutTemporaryBlock from expand_diffs) has been processed, deterministically waiting for the background layout hop. - Enable warp_editor/test-util in warp_tui dev-dependencies so layout_complete()/outstanding_layouts are available. Co-Authored-By: Oz --- crates/warp_tui/Cargo.toml | 4 + .../warp_tui/src/tui_file_edits_view_tests.rs | 75 +++++++++++++------ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/crates/warp_tui/Cargo.toml b/crates/warp_tui/Cargo.toml index 40f4171c5dc..fdcbdad37c7 100644 --- a/crates/warp_tui/Cargo.toml +++ b/crates/warp_tui/Cargo.toml @@ -72,6 +72,10 @@ tempfile.workspace = true # `test-util` exposes `Appearance::mock()`, used to register the `Appearance` # singleton that the editor-backed TUI input view depends on in tests. warp_core = { workspace = true, features = ["test-util"] } +# `test-util` exposes `RenderState::layout_complete()`, which the diff-pipeline +# test awaits to deterministically wait for the async layout channel (polled on +# a background thread) to deliver ghost blocks. +warp_editor = { workspace = true, features = ["test-util"] } async-channel.workspace = true # Process-level worker-dispatch regression test. command.workspace = true diff --git a/crates/warp_tui/src/tui_file_edits_view_tests.rs b/crates/warp_tui/src/tui_file_edits_view_tests.rs index d847a7de99f..9c7538d8d9b 100644 --- a/crates/warp_tui/src/tui_file_edits_view_tests.rs +++ b/crates/warp_tui/src/tui_file_edits_view_tests.rs @@ -79,10 +79,28 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { let (tx, rx) = oneshot::channel(); app.update(|ctx| { let mut tx = Some(tx); - ctx.subscribe_to_model(&editor, move |_, event, _| { + ctx.subscribe_to_model(&editor, move |editor, event, app| { if matches!(event, CodeEditorModelEvent::DiffUpdated) { - if let Some(tx) = tx.take() { - let _ = tx.send(()); + // `reset_content` seeds the diff base and triggers a + // base==new diff whose spawned task races the abort issued + // by the subsequent `apply_diffs`. If that stale task + // completes first it emits a spurious `DiffUpdated` for an + // *empty* diff, so synchronizing on the first event would + // release the test before the real, post-edit diff lands. + // Only signal once the diff actually reflects a change — + // the post-edit diff is the only one that populates + // `added_or_changed_lines`. + let diff_has_changes = editor + .as_ref(app) + .diff() + .as_ref(app) + .added_or_changed_lines() + .count() + > 0; + if diff_has_changes { + if let Some(tx) = tx.take() { + let _ = tx.send(()); + } } } }); @@ -103,26 +121,37 @@ fn diff_pipeline_computes_added_lines_and_ghost_blocks() { editor.update(&mut app, |editor, ctx| editor.expand_diffs(ctx)); - // Ghost blocks land via the render state's async layout channel; poll - // until the spawned handler has stored them. - let mut ghosts = Vec::new(); - for _ in 0..100 { - ghosts = app.read(|app| { - editor - .as_ref(app) - .render_state() - .as_ref(app) - .char_cell() - .expect("TUI editor renders in char-cell mode") - .display_lattice(&[]) - .ghosts() - .to_vec() - }); - if !ghosts.is_empty() { - break; - } - futures_lite::future::yield_now().await; - } + // Ghost blocks land via the render state's async layout channel: + // `add_temporary_blocks` submits a `LayoutTemporaryBlock` action that a + // *background* task (spawned by `spawn_stream_local`) polls and forwards + // to a foreground task which stores the blocks on the char-cell state. + // `yield_now` only drives the foreground executor, so a fixed-iteration + // poll can't deterministically wait for that background hop (the original + // cause of the flake). `layout_complete()` awaits the + // `outstanding_layouts` counter until every submitted action — including + // the `LayoutTemporaryBlock` from `expand_diffs` — has been processed, + // deterministically waiting for the ghosts to land regardless of how the + // background thread is scheduled. + let layout_complete = app.read(|app| { + editor + .as_ref(app) + .render_state() + .as_ref(app) + .layout_complete() + }); + layout_complete.await; + + let ghosts = app.read(|app| { + editor + .as_ref(app) + .render_state() + .as_ref(app) + .char_cell() + .expect("TUI editor renders in char-cell mode") + .display_lattice(&[]) + .ghosts() + .to_vec() + }); assert_eq!(ghosts.len(), 1); assert_eq!(ghosts[0].content, "old\n");