visual-test-foreground-executor.md

  1# Controllable Foreground Executor for Visual Tests
  2
  3## Status: COMPLETED ✓
  4
  5The implementation is complete. See the summary at the end of this document.
  6
  7## Problem Statement
  8
  9The visual test framework (`VisualTestAppContext`) cannot properly test UI interactions that rely on foreground async tasks, such as tooltips, hover states with delays, and other deferred UI updates. This is because:
 10
 111. **The visual test framework uses the real macOS platform** for actual rendering (needed for screenshot capture)
 122. **But it doesn't drive the macOS run loop**, so foreground tasks spawned via `window.spawn()` never execute
 133. **`run_until_parked()` only drives the background executor**, not the foreground executor
 14
 15### Specific Example: Tooltip Testing
 16
 17When trying to capture a screenshot of a tooltip:
 18
 191. We call `window.simulate_mouse_move(position, cx)` to hover over a button
 202. The button's tooltip handler in `register_tooltip_mouse_handlers` (in `div.rs`) spawns a delayed task:
 21   ```rust
 22   let delayed_show_task = window.spawn(cx, {
 23       async move |cx| {
 24           cx.background_executor().timer(TOOLTIP_SHOW_DELAY).await;  // 500ms
 25           cx.update(|window, cx| {
 26               // Set active_tooltip state
 27               window.refresh();
 28           }).ok();
 29       }
 30   });
 31   ```
 323. We wait with `cx.background_executor().timer(700ms).await`
 334. We take a screenshot - **but the tooltip never appears**
 34
 35The reason: `window.spawn()` schedules work on the **foreground executor**, but our test only drives the **background executor**. The tooltip task is sitting in the foreground queue, never being processed.
 36
 37## Architecture Overview
 38
 39### Current Executor Setup
 40
 41```
 42┌─────────────────────────────────────────────────────────────┐
 43│                     VisualTestAppContext                     │
 44├─────────────────────────────────────────────────────────────┤
 45│  platform: Rc<dyn Platform>  ←── MacPlatform (real)         │
 46│  background_executor: BackgroundExecutor                     │
 47│  foreground_executor: ForegroundExecutor                     │
 48└─────────────────────────────────────────────────────────────┘
 49 50 51┌─────────────────────────────────────────────────────────────┐
 52│                       MacPlatform                            │
 53├─────────────────────────────────────────────────────────────┤
 54│  dispatcher: Arc<MacDispatcher>                              │
 55│    - Uses real macOS run loop (CFRunLoop)                   │
 56│    - as_test() returns None                                  │
 57└─────────────────────────────────────────────────────────────┘
 58```
 59
 60### Test Platform (for comparison)
 61
 62```
 63┌─────────────────────────────────────────────────────────────┐
 64│                      TestAppContext                          │
 65├─────────────────────────────────────────────────────────────┤
 66│  platform: TestPlatform                                      │
 67│  dispatcher: TestDispatcher                                  │
 68│    - Controllable task queue                                │
 69│    - as_test() returns Some(self)                           │
 70│    - run_until_parked() drives both bg and fg tasks         │
 71└─────────────────────────────────────────────────────────────┘
 72```
 73
 74## Key Files to Investigate
 75
 76### 1. Platform Dispatcher Trait
 77**File:** `crates/gpui/src/platform.rs`
 78
 79```rust
 80pub trait PlatformDispatcher: Send + Sync {
 81    fn is_main_thread(&self) -> bool;
 82    fn dispatch(&self, runnable: Runnable, label: Option<TaskLabel>);
 83    fn dispatch_on_main_thread(&self, runnable: Runnable);
 84    fn dispatch_after(&self, duration: Duration, runnable: Runnable);
 85    fn park(&self, timeout: Option<Duration>) -> bool;
 86    fn unparker(&self) -> Unparker;
 87
 88    #[cfg(any(test, feature = "test-support"))]
 89    fn as_test(&self) -> Option<&TestDispatcher> {
 90        None  // Default implementation - real platforms return None
 91    }
 92}
 93```
 94
 95### 2. Test Dispatcher Implementation
 96**File:** `crates/gpui/src/platform/test/dispatcher.rs`
 97
 98This is the reference implementation for a controllable dispatcher:
 99
100```rust
101pub struct TestDispatcher {
102    state: Mutex<TestDispatcherState>,
103    // ...
104}
105
106struct TestDispatcherState {
107    random: StdRng,
108    foreground: Vec<TestTask>,     // ← Foreground task queue
109    background: Vec<TestTask>,     // ← Background task queue
110    on_parking: Vec<UnparkCallback>,
111    waiting_hint: Option<String>,
112    block_on_ticks: RangeInclusive<usize>,
113    forbid_parking: bool,
114}
115
116impl TestDispatcher {
117    pub fn run_until_parked(&self) {
118        // Drives BOTH foreground and background tasks
119        while self.poll(true) {}
120    }
121    
122    fn poll(&self, background_only: bool) -> bool {
123        // Randomly selects and runs tasks from both queues
124    }
125}
126```
127
128### 3. Mac Dispatcher (Current Visual Test Dispatcher)
129**File:** `crates/gpui/src/platform/mac/dispatcher.rs`
130
131```rust
132pub(crate) struct MacDispatcher;
133
134impl PlatformDispatcher for MacDispatcher {
135    fn dispatch_on_main_thread(&self, runnable: Runnable) {
136        // Dispatches to actual macOS main thread via CFRunLoop
137        unsafe {
138            dispatch_async_f(
139                dispatch_get_main_queue(),
140                // ...
141            );
142        }
143    }
144    
145    // as_test() returns None (default)
146}
147```
148
149### 4. Visual Test Context Creation
150**File:** `crates/gpui/src/app/visual_test_context.rs`
151
152```rust
153impl VisualTestAppContext {
154    pub fn new() -> Self {
155        let platform = current_platform(false);  // ← Gets MacPlatform
156        let background_executor = platform.background_executor();
157        let foreground_executor = platform.foreground_executor();
158        // ...
159    }
160    
161    pub fn run_until_parked(&self) {
162        self.background_executor.run_until_parked();  // ← Only bg!
163    }
164}
165```
166
167### 5. BackgroundExecutor::run_until_parked
168**File:** `crates/gpui/src/executor.rs`
169
170```rust
171impl BackgroundExecutor {
172    pub fn run_until_parked(&self) {
173        self.dispatcher.as_test().unwrap().run_until_parked()
174        //              ^^^^^^^^^^^^^^^^ 
175        // This panics for MacDispatcher because as_test() returns None!
176    }
177}
178```
179
180Wait - this means `run_until_parked()` should be panicking in visual tests. Let me check if it's actually being called...
181
182Actually, looking at the test output, it seems like it doesn't panic. This needs investigation - perhaps there's a different code path or the method isn't being called.
183
184## Proposed Solution
185
186### Option A: Hybrid Visual Test Platform (Recommended)
187
188Create a new platform type that:
189- Uses **real Mac rendering** (Metal, text system, etc.) for actual pixel output
190- Uses **TestDispatcher** for controllable task execution
191
192```rust
193pub struct VisualTestPlatform {
194    // Real Mac components for rendering
195    mac_text_system: Arc<MacTextSystem>,
196    mac_renderer: MacRenderer,  // Or access via TestWindow with real backing
197    
198    // Test dispatcher for controllable execution
199    dispatcher: Arc<TestDispatcher>,
200}
201```
202
203**Pros:**
204- Full control over task execution timing
205- Deterministic test behavior
206- Can still capture real rendered output
207
208**Cons:**
209- Significant implementation effort
210- Need to carefully separate "rendering" from "event loop" concerns
211- May need to refactor how windows get their renderer
212
213### Option B: Foreground Task Pumping in Visual Tests
214
215Add a mechanism to manually pump foreground tasks in the visual test context:
216
217```rust
218impl VisualTestAppContext {
219    pub fn pump_foreground_tasks(&mut self, duration: Duration) {
220        let deadline = Instant::now() + duration;
221        while Instant::now() < deadline {
222            // Manually poll the foreground task queue
223            if !self.platform.pump_one_foreground_task() {
224                std::thread::sleep(Duration::from_millis(1));
225            }
226        }
227    }
228}
229```
230
231This would require adding a new method to `PlatformDispatcher`:
232
233```rust
234pub trait PlatformDispatcher: Send + Sync {
235    // Existing methods...
236    
237    /// Attempt to run one foreground task if available.
238    /// Returns true if a task was run, false if queue was empty.
239    fn pump_one_foreground_task(&self) -> bool { false }
240}
241```
242
243**Pros:**
244- Smaller change
245- Doesn't require new platform type
246
247**Cons:**
248- Less deterministic (still time-based)
249- May not work well with macOS's dispatch queue semantics
250
251### Option C: Window-Spawned Task Interception
252
253Intercept tasks spawned via `window.spawn()` and redirect them to a controllable queue:
254
255```rust
256impl Window {
257    pub fn spawn<R>(&mut self, cx: &mut App, f: impl Future<Output = R>) -> Task<R> {
258        #[cfg(any(test, feature = "test-support"))]
259        if cx.is_visual_test_mode() {
260            return self.spawn_to_test_queue(f);
261        }
262        
263        // Normal spawn path
264        self.foreground_executor.spawn(f)
265    }
266}
267```
268
269**Pros:**
270- Targeted fix for the specific problem
271- Minimal changes to existing code
272
273**Cons:**
274- Doesn't solve the general problem
275- Test-specific code paths can diverge from production behavior
276
277## Implementation Plan for Option A
278
279### Step 1: Create TestDispatcher with Real Timing Option
280
281Modify `TestDispatcher` to support real-time delays instead of simulated time:
282
283```rust
284pub struct TestDispatcher {
285    state: Mutex<TestDispatcherState>,
286    use_real_time: bool,  // New flag
287}
288
289impl TestDispatcher {
290    pub fn new_with_real_time() -> Self {
291        Self {
292            state: Mutex::new(TestDispatcherState::new()),
293            use_real_time: true,
294        }
295    }
296}
297```
298
299### Step 2: Create VisualTestPlatform
300
301**New file:** `crates/gpui/src/platform/visual_test/mod.rs`
302
303```rust
304pub struct VisualTestPlatform {
305    dispatcher: Arc<TestDispatcher>,
306    text_system: Arc<MacTextSystem>,
307    // Other Mac components needed for rendering
308}
309
310impl Platform for VisualTestPlatform {
311    fn dispatcher(&self) -> Arc<dyn PlatformDispatcher> {
312        self.dispatcher.clone()
313    }
314    
315    fn text_system(&self) -> Arc<dyn PlatformTextSystem> {
316        self.text_system.clone()
317    }
318    
319    fn open_window(...) -> ... {
320        // Create window with real Metal rendering
321        // but using our test dispatcher
322    }
323}
324```
325
326### Step 3: Create VisualTestWindow
327
328The window needs to use real rendering but the test dispatcher:
329
330```rust
331pub struct VisualTestWindow {
332    // Real Metal/rendering components from MacWindow
333    renderer: Renderer,
334    native_view: ...,
335    
336    // But dispatches through TestDispatcher
337    dispatcher: Arc<TestDispatcher>,
338}
339```
340
341### Step 4: Update VisualTestAppContext
342
343```rust
344impl VisualTestAppContext {
345    pub fn new() -> Self {
346        let dispatcher = Arc::new(TestDispatcher::new_with_real_time());
347        let platform = Arc::new(VisualTestPlatform::new(dispatcher.clone()));
348        // ...
349    }
350    
351    pub fn run_until_parked(&self) {
352        // Now this works because we have a TestDispatcher
353        self.dispatcher.run_until_parked();
354    }
355}
356```
357
358### Step 5: Test the Tooltip Capture
359
360```rust
361// In visual_test_runner.rs
362cx.simulate_mouse_move(window, button_position, None, Modifiers::default());
363
364// Wait real time for the tooltip delay
365cx.background_executor()
366    .timer(Duration::from_millis(600))
367    .await;
368
369// Drive all pending tasks including the tooltip show task
370cx.run_until_parked();
371
372// Now the tooltip should be visible
373cx.update_window(window, |_, window, _| window.refresh())?;
374
375// Capture screenshot with tooltip
376let screenshot = capture_screenshot(window, cx)?;
377```
378
379## Testing the Fix
380
381After implementation, these scenarios should work:
382
3831. **Tooltip on hover**: Mouse over button → wait → tooltip appears in screenshot
3842. **Hover styles**: Mouse over element → hover style visible in screenshot  
3853. **Delayed animations**: Any animation triggered by foreground tasks
3864. **Debounced updates**: UI updates that use debouncing/throttling
387
388## Files to Modify
389
3901. `crates/gpui/src/platform.rs` - May need new trait methods
3912. `crates/gpui/src/platform/test/dispatcher.rs` - Add real-time mode
3923. `crates/gpui/src/platform/visual_test/mod.rs` - New file
3934. `crates/gpui/src/platform/visual_test/platform.rs` - New file
3945. `crates/gpui/src/platform/visual_test/window.rs` - New file
3956. `crates/gpui/src/app/visual_test_context.rs` - Use new platform
3967. `crates/gpui/src/platform/mod.rs` - Export new module
3978. `crates/zed/src/visual_test_runner.rs` - Update test code
398
399## Questions to Resolve
400
4011. **Can Metal rendering work without the macOS run loop?** 
402   - Need to investigate if `CAMetalLayer` and friends require the run loop
403   
4042. **How does `render_to_image()` work?**
405   - This is used for screenshot capture - need to ensure it works with test platform
406
4073. **What about system events (keyboard, mouse)?**
408   - Visual tests simulate these - should work with test dispatcher
409
4104. **Thread safety concerns?**
411   - TestDispatcher is designed for single-threaded use
412   - Metal rendering may have threading requirements
413
414## Related Code References
415
416- `Window::spawn` - `crates/gpui/src/window.rs`
417- `register_tooltip_mouse_handlers` - `crates/gpui/src/elements/div.rs:2845`
418- `handle_tooltip_mouse_move` - `crates/gpui/src/elements/div.rs:2873`
419- `TOOLTIP_SHOW_DELAY` - `crates/gpui/src/elements/div.rs:48` (500ms)
420- `TestWindow` - `crates/gpui/src/platform/test/window.rs`
421- `MacWindow` - `crates/gpui/src/platform/mac/window.rs`
422
423## Success Criteria
424
4251. `diff_review_button_tooltip.png` shows the "Add Review" tooltip
4262. Button shows hover border when mouse is over it
4273. Tests remain deterministic (same output every run)
4284. No reliance on wall-clock timing for correctness
429
430---
431
432## Implementation Summary
433
434### Changes Made
435
4361. **Created `VisualTestPlatform`** (`crates/gpui/src/platform/visual_test.rs`)
437   - A hybrid platform that combines real Mac rendering with controllable `TestDispatcher`
438   - Uses `MacPlatform` for window creation, text system, rendering, and display management
439   - Uses `TestDispatcher` for deterministic task scheduling
440   - Implements the `Platform` trait, delegating rendering operations to `MacPlatform`
441   - Passes its own `ForegroundExecutor` (from `TestDispatcher`) to `MacWindow::open()`
442
4432. **Added `renderer_context()` method to `MacPlatform`** (`crates/gpui/src/platform/mac/platform.rs`)
444   - Allows `VisualTestPlatform` to access the renderer context for window creation
445   - Conditionally compiled for test-support
446
4473. **Updated `VisualTestAppContext`** (`crates/gpui/src/app/visual_test_context.rs`)
448   - Now creates and uses `VisualTestPlatform` instead of `current_platform()`
449   - Gets dispatcher and executors from the platform
450   - This ensures `App::spawn()` and `Window::spawn()` use the `TestDispatcher`
451
4524. **Added tests** (`crates/gpui/src/app/visual_test_context.rs`)
453   - `test_foreground_tasks_run_with_run_until_parked` - verifies foreground tasks execute
454   - `test_advance_clock_triggers_delayed_tasks` - verifies timer-based tasks work
455   - `test_window_spawn_uses_test_dispatcher` - verifies window.spawn uses TestDispatcher
456   - All tests are marked `#[ignore]` because they require macOS main thread
457
458### How It Works
459
460The key insight was that `App::new_app()` gets its executors from `platform.foreground_executor()`. Previously:
461
462```
463VisualTestAppContext
464  └── creates TestDispatcher (unused!)
465  └── creates App with MacPlatform
466        └── MacPlatform has MacDispatcher
467        └── App uses MacDispatcher's executors ❌
468```
469
470After the fix:
471
472```
473VisualTestAppContext
474  └── creates VisualTestPlatform
475        └── Has TestDispatcher
476        └── Has MacPlatform (for rendering)
477        └── foreground_executor() returns TestDispatcher's executor ✓
478  └── creates App with VisualTestPlatform
479        └── App uses TestDispatcher's executors ✓
480        └── Window::spawn() uses TestDispatcher ✓
481```
482
483### Running Visual Tests
484
485Visual tests require the macOS main thread. Run them with:
486
487```bash
488cargo test -p gpui visual_test_context -- --ignored --test-threads=1
489cargo test -p zed visual_tests -- --ignored --test-threads=1
490```
491
492### Tooltip Testing
493
494With this fix, tooltip testing now works:
495
496```rust
497// Simulate hovering over a button
498cx.simulate_mouse_move(window, button_position, None, Modifiers::default());
499
500// Advance clock past TOOLTIP_SHOW_DELAY (500ms)
501cx.advance_clock(Duration::from_millis(600));
502
503// The tooltip task spawned via window.spawn() is now executed!
504// Take screenshot - tooltip will be visible
505let screenshot = cx.capture_screenshot(window)?;
506```