Merge pull request #69 from zed-industries/fix-flaky-test

Antonio Scandurra created

Try fixing flaky tests

Change summary

fsevent/src/lib.rs           | 33 ++++++++++++----------
gpui/src/app.rs              | 28 ++++++------------
zed/src/editor/buffer/mod.rs | 16 ++--------
zed/src/file_finder.rs       | 55 +++++++++++++++++++++++--------------
zed/src/workspace.rs         | 14 ++------
zed/src/worktree.rs          | 12 ++-----
6 files changed, 74 insertions(+), 84 deletions(-)

Detailed changes

fsevent/src/lib.rs 🔗

@@ -307,22 +307,20 @@ mod tests {
             for i in 0..10 {
                 fs::write(path.join(format!("existing-file-{}", i)), "").unwrap();
             }
+            flush_historical_events();
 
             let (tx, rx) = mpsc::channel();
             let (stream, handle) = EventStream::new(&[&path], Duration::from_millis(50));
             thread::spawn(move || stream.run(move |events| tx.send(events.to_vec()).is_ok()));
 
-            // Flush any historical events.
-            rx.recv_timeout(Duration::from_millis(500)).ok();
-
             fs::write(path.join("new-file"), "").unwrap();
-            let events = rx.recv_timeout(Duration::from_millis(500)).unwrap();
+            let events = rx.recv_timeout(Duration::from_secs(2)).unwrap();
             let event = events.last().unwrap();
             assert_eq!(event.path, path.join("new-file"));
             assert!(event.flags.contains(StreamFlags::ITEM_CREATED));
 
             fs::remove_file(path.join("existing-file-5")).unwrap();
-            let events = rx.recv_timeout(Duration::from_millis(500)).unwrap();
+            let events = rx.recv_timeout(Duration::from_secs(2)).unwrap();
             let event = events.last().unwrap();
             assert_eq!(event.path, path.join("existing-file-5"));
             assert!(event.flags.contains(StreamFlags::ITEM_REMOVED));
@@ -338,6 +336,7 @@ mod tests {
             for i in 0..10 {
                 fs::write(path.join(format!("existing-file-{}", i)), "").unwrap();
             }
+            flush_historical_events();
 
             let (tx, rx) = mpsc::channel();
             let (stream, handle) = EventStream::new(&[&path], Duration::from_millis(50));
@@ -350,13 +349,13 @@ mod tests {
             });
 
             fs::write(path.join("new-file"), "").unwrap();
-            let events = rx.recv_timeout(Duration::from_millis(800)).unwrap();
+            let events = rx.recv_timeout(Duration::from_secs(2)).unwrap();
             let event = events.last().unwrap();
             assert_eq!(event.path, path.join("new-file"));
             assert!(event.flags.contains(StreamFlags::ITEM_CREATED));
 
             fs::remove_file(path.join("existing-file-5")).unwrap();
-            let events = rx.recv_timeout(Duration::from_millis(500)).unwrap();
+            let events = rx.recv_timeout(Duration::from_secs(2)).unwrap();
             let event = events.last().unwrap();
             assert_eq!(event.path, path.join("existing-file-5"));
             assert!(event.flags.contains(StreamFlags::ITEM_REMOVED));
@@ -368,6 +367,7 @@ mod tests {
     fn test_event_stream_shutdown_by_dropping_handle() {
         let dir = TempDir::new("test-event-stream").unwrap();
         let path = dir.path().canonicalize().unwrap();
+        flush_historical_events();
 
         let (tx, rx) = mpsc::channel();
         let (stream, handle) = EventStream::new(&[&path], Duration::from_millis(50));
@@ -383,17 +383,11 @@ mod tests {
         });
 
         fs::write(path.join("new-file"), "").unwrap();
-        assert_eq!(
-            rx.recv_timeout(Duration::from_millis(500)).unwrap(),
-            "running"
-        );
+        assert_eq!(rx.recv_timeout(Duration::from_secs(2)).unwrap(), "running");
 
         // Dropping the handle causes `EventStream::run` to return.
         drop(handle);
-        assert_eq!(
-            rx.recv_timeout(Duration::from_millis(500)).unwrap(),
-            "stopped"
-        );
+        assert_eq!(rx.recv_timeout(Duration::from_secs(2)).unwrap(), "stopped");
     }
 
     #[test]
@@ -407,4 +401,13 @@ mod tests {
         // This returns immediately because the handle was already dropped.
         stream.run(|_| true);
     }
+
+    fn flush_historical_events() {
+        let duration = if std::env::var("CI").is_ok() {
+            Duration::from_secs(2)
+        } else {
+            Duration::from_millis(500)
+        };
+        thread::sleep(duration);
+    }
 }

gpui/src/app.rs 🔗

@@ -2082,15 +2082,6 @@ impl<T: Entity> ModelHandle<T> {
     pub fn condition(
         &self,
         ctx: &TestAppContext,
-        predicate: impl FnMut(&T, &AppContext) -> bool,
-    ) -> impl Future<Output = ()> {
-        self.condition_with_duration(Duration::from_millis(100), ctx, predicate)
-    }
-
-    pub fn condition_with_duration(
-        &self,
-        duration: Duration,
-        ctx: &TestAppContext,
         mut predicate: impl FnMut(&T, &AppContext) -> bool,
     ) -> impl Future<Output = ()> {
         let (tx, mut rx) = mpsc::channel(1024);
@@ -2113,6 +2104,11 @@ impl<T: Entity> ModelHandle<T> {
 
         let ctx = ctx.weak_self.as_ref().unwrap().upgrade().unwrap();
         let handle = self.downgrade();
+        let duration = if std::env::var("CI").is_ok() {
+            Duration::from_secs(2)
+        } else {
+            Duration::from_millis(500)
+        };
 
         async move {
             timeout(duration, async move {
@@ -2290,15 +2286,6 @@ impl<T: View> ViewHandle<T> {
     pub fn condition(
         &self,
         ctx: &TestAppContext,
-        predicate: impl FnMut(&T, &AppContext) -> bool,
-    ) -> impl Future<Output = ()> {
-        self.condition_with_duration(Duration::from_millis(500), ctx, predicate)
-    }
-
-    pub fn condition_with_duration(
-        &self,
-        duration: Duration,
-        ctx: &TestAppContext,
         mut predicate: impl FnMut(&T, &AppContext) -> bool,
     ) -> impl Future<Output = ()> {
         let (tx, mut rx) = mpsc::channel(1024);
@@ -2322,6 +2309,11 @@ impl<T: View> ViewHandle<T> {
 
         let ctx = ctx.weak_self.as_ref().unwrap().upgrade().unwrap();
         let handle = self.downgrade();
+        let duration = if std::env::var("CI").is_ok() {
+            Duration::from_secs(2)
+        } else {
+            Duration::from_millis(500)
+        };
 
         async move {
             timeout(duration, async move {

zed/src/editor/buffer/mod.rs 🔗

@@ -3011,9 +3011,7 @@ mod tests {
             });
 
             fs::remove_file(dir.path().join("file2")).unwrap();
-            buffer2
-                .condition_with_duration(Duration::from_millis(500), &app, |b, _| b.is_dirty())
-                .await;
+            buffer2.condition(&app, |b, _| b.is_dirty()).await;
             assert_eq!(
                 *events.borrow(),
                 &[Event::Dirtied, Event::FileHandleChanged]
@@ -3038,9 +3036,7 @@ mod tests {
             events.borrow_mut().clear();
             fs::remove_file(dir.path().join("file3")).unwrap();
             buffer3
-                .condition_with_duration(Duration::from_millis(500), &app, |_, _| {
-                    !events.borrow().is_empty()
-                })
+                .condition(&app, |_, _| !events.borrow().is_empty())
                 .await;
             assert_eq!(*events.borrow(), &[Event::FileHandleChanged]);
             app.read(|ctx| assert!(buffer3.read(ctx).is_dirty()));
@@ -3095,9 +3091,7 @@ mod tests {
         // contents are edited according to the diff between the old and new
         // file contents.
         buffer
-            .condition_with_duration(Duration::from_millis(500), &app, |buffer, _| {
-                buffer.text() != initial_contents
-            })
+            .condition(&app, |buffer, _| buffer.text() != initial_contents)
             .await;
 
         buffer.update(&mut app, |buffer, _| {
@@ -3131,9 +3125,7 @@ mod tests {
         // Becaues the buffer is modified, it doesn't reload from disk, but is
         // marked as having a conflict.
         buffer
-            .condition_with_duration(Duration::from_millis(500), &app, |buffer, _| {
-                buffer.has_conflict()
-            })
+            .condition(&app, |buffer, _| buffer.has_conflict())
             .await;
     }
 

zed/src/file_finder.rs 🔗

@@ -11,7 +11,7 @@ use gpui::{
     fonts::{Properties, Weight},
     geometry::vector::vec2f,
     keymap::{self, Binding},
-    AppContext, Axis, Border, Entity, MutableAppContext, View, ViewContext, ViewHandle,
+    AppContext, Axis, Border, Entity, MutableAppContext, Task, View, ViewContext, ViewHandle,
     WeakViewHandle,
 };
 use postage::watch;
@@ -310,7 +310,9 @@ impl FileFinder {
     }
 
     fn workspace_updated(&mut self, _: ViewHandle<Workspace>, ctx: &mut ViewContext<Self>) {
-        self.spawn_search(self.query_buffer.read(ctx).text(ctx.as_ref()), ctx);
+        if let Some(task) = self.spawn_search(self.query_buffer.read(ctx).text(ctx.as_ref()), ctx) {
+            task.detach();
+        }
     }
 
     fn on_query_buffer_event(
@@ -328,7 +330,9 @@ impl FileFinder {
                     self.matches.clear();
                     ctx.notify();
                 } else {
-                    self.spawn_search(query, ctx);
+                    if let Some(task) = self.spawn_search(query, ctx) {
+                        task.detach();
+                    }
                 }
             }
             Blurred => ctx.emit(Event::Dismissed),
@@ -385,7 +389,8 @@ impl FileFinder {
         ctx.emit(Event::Selected(*tree_id, path.clone()));
     }
 
-    fn spawn_search(&mut self, query: String, ctx: &mut ViewContext<Self>) -> Option<()> {
+    #[must_use]
+    fn spawn_search(&mut self, query: String, ctx: &mut ViewContext<Self>) -> Option<Task<()>> {
         let snapshots = self
             .workspace
             .upgrade(&ctx)?
@@ -415,13 +420,10 @@ impl FileFinder {
             (search_id, did_cancel, query, matches)
         });
 
-        ctx.spawn(|this, mut ctx| async move {
+        Some(ctx.spawn(|this, mut ctx| async move {
             let matches = background_task.await;
             this.update(&mut ctx, |this, ctx| this.update_matches(matches, ctx));
-        })
-        .detach();
-
-        Some(())
+        }))
     }
 
     fn update_matches(
@@ -550,15 +552,18 @@ mod tests {
         let (_, finder) = app.add_window(|ctx| FileFinder::new(settings, workspace.clone(), ctx));
 
         let query = "hi".to_string();
-        finder.update(&mut app, |f, ctx| f.spawn_search(query.clone(), ctx));
-        finder.condition(&app, |f, _| f.matches.len() == 5).await;
+        finder
+            .update(&mut app, |f, ctx| f.spawn_search(query.clone(), ctx))
+            .unwrap()
+            .await;
+        finder.read_with(&app, |f, _| assert_eq!(f.matches.len(), 5));
 
         finder.update(&mut app, |finder, ctx| {
             let matches = finder.matches.clone();
 
             // Simulate a search being cancelled after the time limit,
             // returning only a subset of the matches that would have been found.
-            finder.spawn_search(query.clone(), ctx);
+            finder.spawn_search(query.clone(), ctx).unwrap().detach();
             finder.update_matches(
                 (
                     finder.latest_search_id,
@@ -570,7 +575,7 @@ mod tests {
             );
 
             // Simulate another cancellation.
-            finder.spawn_search(query.clone(), ctx);
+            finder.spawn_search(query.clone(), ctx).unwrap().detach();
             finder.update_matches(
                 (
                     finder.latest_search_id,
@@ -605,14 +610,16 @@ mod tests {
 
         // Even though there is only one worktree, that worktree's filename
         // is included in the matching, because the worktree is a single file.
-        finder.update(&mut app, |f, ctx| f.spawn_search("thf".into(), ctx));
-        finder.condition(&app, |f, _| f.matches.len() == 1).await;
-
+        finder
+            .update(&mut app, |f, ctx| f.spawn_search("thf".into(), ctx))
+            .unwrap()
+            .await;
         app.read(|ctx| {
             let finder = finder.read(ctx);
+            assert_eq!(finder.matches.len(), 1);
+
             let (file_name, file_name_positions, full_path, full_path_positions) =
                 finder.labels_for_match(&finder.matches[0], ctx).unwrap();
-
             assert_eq!(file_name, "the-file");
             assert_eq!(file_name_positions, &[0, 1, 4]);
             assert_eq!(full_path, "the-file");
@@ -621,8 +628,11 @@ mod tests {
 
         // Since the worktree root is a file, searching for its name followed by a slash does
         // not match anything.
-        finder.update(&mut app, |f, ctx| f.spawn_search("thf/".into(), ctx));
-        finder.condition(&app, |f, _| f.matches.len() == 0).await;
+        finder
+            .update(&mut app, |f, ctx| f.spawn_search("thf/".into(), ctx))
+            .unwrap()
+            .await;
+        finder.read_with(&app, |f, _| assert_eq!(f.matches.len(), 0));
     }
 
     #[gpui::test]
@@ -649,11 +659,14 @@ mod tests {
         let (_, finder) = app.add_window(|ctx| FileFinder::new(settings, workspace.clone(), ctx));
 
         // Run a search that matches two files with the same relative path.
-        finder.update(&mut app, |f, ctx| f.spawn_search("a.t".into(), ctx));
-        finder.condition(&app, |f, _| f.matches.len() == 2).await;
+        finder
+            .update(&mut app, |f, ctx| f.spawn_search("a.t".into(), ctx))
+            .unwrap()
+            .await;
 
         // Can switch between different matches with the same relative path.
         finder.update(&mut app, |f, ctx| {
+            assert_eq!(f.matches.len(), 2);
             assert_eq!(f.selected_index(), 0);
             f.select_next(&(), ctx);
             assert_eq!(f.selected_index(), 1);

zed/src/workspace.rs 🔗

@@ -759,7 +759,7 @@ mod tests {
     use super::*;
     use crate::{editor::BufferView, settings, test::temp_tree};
     use serde_json::json;
-    use std::{collections::HashSet, fs, time::Duration};
+    use std::{collections::HashSet, fs};
     use tempdir::TempDir;
 
     #[gpui::test]
@@ -1031,18 +1031,14 @@ mod tests {
         app.update(|ctx| editor.update(ctx, |editor, ctx| editor.insert(&"x".to_string(), ctx)));
         fs::write(dir.path().join("a.txt"), "changed").unwrap();
         editor
-            .condition_with_duration(Duration::from_millis(500), &app, |editor, ctx| {
-                editor.has_conflict(ctx)
-            })
+            .condition(&app, |editor, ctx| editor.has_conflict(ctx))
             .await;
         app.read(|ctx| assert!(editor.is_dirty(ctx)));
 
         app.update(|ctx| workspace.update(ctx, |w, ctx| w.save_active_item(&(), ctx)));
         app.simulate_prompt_answer(window_id, 0);
         editor
-            .condition_with_duration(Duration::from_millis(500), &app, |editor, ctx| {
-                !editor.is_dirty(ctx)
-            })
+            .condition(&app, |editor, ctx| !editor.is_dirty(ctx))
             .await;
         app.read(|ctx| assert!(!editor.has_conflict(ctx)));
     }
@@ -1099,9 +1095,7 @@ mod tests {
 
         // When the save completes, the buffer's title is updated.
         editor
-            .condition_with_duration(Duration::from_millis(500), &app, |editor, ctx| {
-                !editor.is_dirty(ctx)
-            })
+            .condition(&app, |editor, ctx| !editor.is_dirty(ctx))
             .await;
         app.read(|ctx| {
             assert!(!editor.is_dirty(ctx));

zed/src/worktree.rs 🔗

@@ -1276,16 +1276,12 @@ impl WorktreeHandle for ModelHandle<Worktree> {
         let tree = self.clone();
         async move {
             fs::write(root_path.join(filename), "").unwrap();
-            tree.condition_with_duration(Duration::from_secs(5), &app, |tree, _| {
-                tree.entry_for_path(filename).is_some()
-            })
-            .await;
+            tree.condition(&app, |tree, _| tree.entry_for_path(filename).is_some())
+                .await;
 
             fs::remove_file(root_path.join(filename)).unwrap();
-            tree.condition_with_duration(Duration::from_secs(5), &app, |tree, _| {
-                tree.entry_for_path(filename).is_none()
-            })
-            .await;
+            tree.condition(&app, |tree, _| tree.entry_for_path(filename).is_none())
+                .await;
 
             app.read(|ctx| tree.read(ctx).scan_complete()).await;
         }