Fix visual test runner panics and add visual tests to macOS CI

Richard Feldman created 1 month ago

Three pre-existing issues in the visual test runner were never caught
because visual tests aren't run in CI. Fix all three and add visual
tests to the macOS CI job so they stay healthy going forward.

1. Re-entrant read panic in sidebar creation (3 places): Sidebar::new
   reads the MultiWorkspace entity, so creating it inside a
   multi_workspace.update() closure causes a double-borrow panic.
   Fix by using update_window with root_view.downcast() instead.

2. Tooltip entity leak in breakpoint_hover test: The tooltip system has
   an Rc cycle (ActiveTooltip::Visible holds a check_visible_and_update
   closure that captures the Rc<RefCell<Option<ActiveTooltip>>>). This
   cycle is normally broken when the tooltip is dismissed, but the test
   closed the window while the tooltip was still visible. Fix by
   simulating a click before cleanup to dismiss the tooltip.

3. Visual tests not in CI: Add a cargo_run_visual_tests step to the
   macOS test job via xtask so these tests run on every PR.

Change summary

.github/workflows/release.yml                  |  2 ++
.github/workflows/run_tests.yml                |  2 ++
crates/zed/src/visual_test_runner.rs           | 18 ++++++++++--------
tooling/xtask/src/tasks/workflows/run_tests.rs |  3 +++
tooling/xtask/src/tasks/workflows/steps.rs     |  4 ++++
5 files changed, 21 insertions(+), 8 deletions(-)

Detailed changes

.github/workflows/release.yml

@@ -43,6 +43,8 @@ jobs:
         SCCACHE_BUCKET: sccache-zed
     - name: steps::cargo_nextest
       run: cargo nextest run --workspace --no-fail-fast --no-tests=warn
+    - name: steps::cargo_run_visual_tests
+      run: cargo run -p zed --bin zed_visual_test_runner --features visual-tests
     - name: steps::show_sccache_stats
       run: sccache --show-stats || true
     - name: steps::cleanup_cargo_config

.github/workflows/run_tests.yml

@@ -440,6 +440,8 @@ jobs:
         SCCACHE_BUCKET: sccache-zed
     - name: steps::cargo_nextest
       run: cargo nextest run --workspace --no-fail-fast --no-tests=warn${{ needs.orchestrate.outputs.changed_packages && format(' -E "{0}"', needs.orchestrate.outputs.changed_packages) || '' }}
+    - name: steps::cargo_run_visual_tests
+      run: cargo run -p zed --bin zed_visual_test_runner --features visual-tests
     - name: steps::show_sccache_stats
       run: sccache --show-stats || true
     - name: steps::cleanup_cargo_config

crates/zed/src/visual_test_runner.rs

@@ -2646,10 +2646,11 @@ fn run_multi_workspace_sidebar_visual_tests(
 
     cx.run_until_parked();
 
-    // Create the sidebar and register it on the MultiWorkspace
-    let sidebar = multi_workspace_window
-        .update(cx, |_multi_workspace, window, cx| {
-            let multi_workspace_handle = cx.entity();
+    // Create the sidebar outside the MultiWorkspace update to avoid a
+    // re-entrant read panic (Sidebar::new reads the MultiWorkspace).
+    let sidebar = cx
+        .update_window(multi_workspace_window.into(), |root_view, window, cx| {
+            let multi_workspace_handle: Entity<MultiWorkspace> = root_view.downcast().unwrap();
             cx.new(|cx| sidebar::Sidebar::new(multi_workspace_handle, window, cx))
         })
         .context("Failed to create sidebar")?;
@@ -3178,10 +3179,11 @@ edition = "2021"
 
     cx.run_until_parked();
 
-    // Create and register the workspace sidebar
-    let sidebar = workspace_window
-        .update(cx, |_multi_workspace, window, cx| {
-            let multi_workspace_handle = cx.entity();
+    // Create the sidebar outside the MultiWorkspace update to avoid a
+    // re-entrant read panic (Sidebar::new reads the MultiWorkspace).
+    let sidebar = cx
+        .update_window(workspace_window.into(), |root_view, window, cx| {
+            let multi_workspace_handle: Entity<MultiWorkspace> = root_view.downcast().unwrap();
             cx.new(|cx| sidebar::Sidebar::new(multi_workspace_handle, window, cx))
         })
         .context("Failed to create sidebar")?;

tooling/xtask/src/tasks/workflows/run_tests.rs

@@ -596,6 +596,9 @@ fn run_platform_tests_impl(platform: Platform, filter_packages: bool) -> NamedJo
             .when(!filter_packages, |job| {
                 job.add_step(steps::cargo_nextest(platform))
             })
+            .when(platform == Platform::Mac, |job| {
+                job.add_step(steps::cargo_run_visual_tests())
+            })
             .add_step(steps::show_sccache_stats(platform))
             .add_step(steps::cleanup_cargo_config(platform)),
     }

tooling/xtask/src/tasks/workflows/steps.rs

@@ -254,6 +254,10 @@ pub fn setup_sccache(platform: Platform) -> Step<Run> {
         .add_env(("SCCACHE_BUCKET", SCCACHE_R2_BUCKET))
 }
 
+pub fn cargo_run_visual_tests() -> Step<Run> {
+    named::bash("cargo run -p zed --bin zed_visual_test_runner --features visual-tests")
+}
+
 pub fn show_sccache_stats(platform: Platform) -> Step<Run> {
     match platform {
         // Use $env:RUSTC_WRAPPER (absolute path) because GITHUB_PATH changes