Skip to content

Try fixing flaky tests #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
as-cii merged 7 commits into from
May 19, 2021
Merged

Try fixing flaky tests #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
830fc38
Increase `condition` timeout on CI and remove `condition_with_duration`
as-cii May 19, 2021
65cf9b7
Remove `condition_with_duration` for `ModelHandle`
as-cii May 19, 2021
1cf5413
Increase timeout to 2s for fsevents tests
as-cii May 19, 2021
074dc55
Temporary: ensure CI env variable is being set
as-cii May 19, 2021
d7f5587
Revert "Temporary: ensure CI env variable is being set"
as-cii May 19, 2021
64d361f
Don't use `condition` to wait for `spawn_search` to complete
as-cii May 19, 2021
1517ccf
Always flush historical events in `fsevent` tests
as-cii May 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions fsevent/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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]
Expand All @@ -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);
}
}
28 changes: 10 additions & 18 deletions gpui/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand Down
16 changes: 4 additions & 12 deletions zed/src/editor/buffer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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()));
Expand Down Expand Up @@ -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, _| {
Expand Down Expand Up @@ -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;
}

Expand Down
55 changes: 34 additions & 21 deletions zed/src/file_finder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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),
Expand Down Expand Up @@ -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)?
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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");
Expand All @@ -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]
Expand All @@ -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);
Expand Down
14 changes: 4 additions & 10 deletions zed/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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)));
}
Expand Down Expand Up @@ -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));
Expand Down
12 changes: 4 additions & 8 deletions zed/src/worktree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down