From debb694d971cfc398f5dd8d87b7c2bb9e67d682b Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 13 Apr 2023 16:51:11 -0700 Subject: [PATCH 01/17] Always bump scan_id when refreshing an entry The scan_id needs to be bumped even if a scan is already in progress, so that worktree updates can detect that entries have changed. This means that the worktree's completed_scan_id may increase by more than one at the end of a scan. --- crates/project/src/worktree.rs | 130 +++++++++++++++++++-------------- 1 file changed, 75 insertions(+), 55 deletions(-) diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 3459bd7e5d7d4fd062cdeb521c1917eb34af5594..29aec156104e05a76e2ce38fa59503a61bfe7aee 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -95,7 +95,17 @@ pub struct Snapshot { root_char_bag: CharBag, entries_by_path: SumTree, entries_by_id: SumTree, + + /// A number that increases every time the worktree begins scanning + /// a set of paths from the filesystem. This scanning could be caused + /// by some operation performed on the worktree, such as reading or + /// writing a file, or by an event reported by the filesystem. scan_id: usize, + + /// The latest scan id that has completed, and whose preceding scans + /// have all completed. The current `scan_id` could be more than one + /// greater than the `completed_scan_id` if operations are performed + /// on the worktree while it is processing a file-system event. completed_scan_id: usize, } @@ -2168,6 +2178,7 @@ impl BackgroundScanner { } { let mut snapshot = self.snapshot.lock(); + snapshot.scan_id += 1; ignore_stack = snapshot.ignore_stack_for_abs_path(&root_abs_path, true); if ignore_stack.is_all() { if let Some(mut root_entry) = snapshot.root_entry().cloned() { @@ -2189,6 +2200,10 @@ impl BackgroundScanner { .unwrap(); drop(scan_job_tx); self.scan_dirs(true, scan_job_rx).await; + { + let mut snapshot = self.snapshot.lock(); + snapshot.completed_scan_id = snapshot.scan_id; + } self.send_status_update(false, None); // Process any any FS events that occurred while performing the initial scan. @@ -2200,7 +2215,6 @@ impl BackgroundScanner { paths.extend(more_events.into_iter().map(|e| e.path)); } self.process_events(paths).await; - self.send_status_update(false, None); } self.finished_initial_scan = true; @@ -2212,9 +2226,8 @@ impl BackgroundScanner { // these before handling changes reported by the filesystem. request = self.refresh_requests_rx.recv().fuse() => { let Ok((paths, barrier)) = request else { break }; - self.reload_entries_for_paths(paths, None).await; - if !self.send_status_update(false, Some(barrier)) { - break; + if !self.process_refresh_request(paths, barrier).await { + return; } } @@ -2225,15 +2238,17 @@ impl BackgroundScanner { paths.extend(more_events.into_iter().map(|e| e.path)); } self.process_events(paths).await; - self.send_status_update(false, None); } } } } - async fn process_events(&mut self, paths: Vec) { - use futures::FutureExt as _; + async fn process_refresh_request(&self, paths: Vec, barrier: barrier::Sender) -> bool { + self.reload_entries_for_paths(paths, None).await; + self.send_status_update(false, Some(barrier)) + } + async fn process_events(&mut self, paths: Vec) { let (scan_job_tx, scan_job_rx) = channel::unbounded(); if let Some(mut paths) = self .reload_entries_for_paths(paths, Some(scan_job_tx.clone())) @@ -2245,35 +2260,7 @@ impl BackgroundScanner { drop(scan_job_tx); self.scan_dirs(false, scan_job_rx).await; - let (ignore_queue_tx, ignore_queue_rx) = channel::unbounded(); - let snapshot = self.update_ignore_statuses(ignore_queue_tx); - self.executor - .scoped(|scope| { - for _ in 0..self.executor.num_cpus() { - scope.spawn(async { - loop { - select_biased! { - // Process any path refresh requests before moving on to process - // the queue of ignore statuses. - request = self.refresh_requests_rx.recv().fuse() => { - let Ok((paths, barrier)) = request else { break }; - self.reload_entries_for_paths(paths, None).await; - if !self.send_status_update(false, Some(barrier)) { - return; - } - } - - // Recursively process directories whose ignores have changed. - job = ignore_queue_rx.recv().fuse() => { - let Ok(job) = job else { break }; - self.update_ignore_status(job, &snapshot).await; - } - } - } - }); - } - }) - .await; + self.update_ignore_statuses().await; let mut snapshot = self.snapshot.lock(); let mut git_repositories = mem::take(&mut snapshot.git_repositories); @@ -2281,6 +2268,9 @@ impl BackgroundScanner { snapshot.git_repositories = git_repositories; snapshot.removed_entry_ids.clear(); snapshot.completed_scan_id = snapshot.scan_id; + drop(snapshot); + + self.send_status_update(false, None); } async fn scan_dirs( @@ -2313,8 +2303,7 @@ impl BackgroundScanner { // the scan queue, so that user operations are prioritized. request = self.refresh_requests_rx.recv().fuse() => { let Ok((paths, barrier)) = request else { break }; - self.reload_entries_for_paths(paths, None).await; - if !self.send_status_update(false, Some(barrier)) { + if !self.process_refresh_request(paths, barrier).await { return; } } @@ -2521,12 +2510,10 @@ impl BackgroundScanner { .await; let mut snapshot = self.snapshot.lock(); - - if snapshot.completed_scan_id == snapshot.scan_id { - snapshot.scan_id += 1; - if !doing_recursive_update { - snapshot.completed_scan_id = snapshot.scan_id; - } + let is_idle = snapshot.completed_scan_id == snapshot.scan_id; + snapshot.scan_id += 1; + if is_idle && !doing_recursive_update { + snapshot.completed_scan_id = snapshot.scan_id; } // Remove any entries for paths that no longer exist or are being recursively @@ -2596,16 +2583,17 @@ impl BackgroundScanner { Some(event_paths) } - fn update_ignore_statuses( - &self, - ignore_queue_tx: Sender, - ) -> LocalSnapshot { + async fn update_ignore_statuses(&self) { + use futures::FutureExt as _; + let mut snapshot = self.snapshot.lock().clone(); let mut ignores_to_update = Vec::new(); let mut ignores_to_delete = Vec::new(); for (parent_abs_path, (_, scan_id)) in &snapshot.ignores_by_parent_abs_path { if let Ok(parent_path) = parent_abs_path.strip_prefix(&snapshot.abs_path) { - if *scan_id == snapshot.scan_id && snapshot.entry_for_path(parent_path).is_some() { + if *scan_id > snapshot.completed_scan_id + && snapshot.entry_for_path(parent_path).is_some() + { ignores_to_update.push(parent_abs_path.clone()); } @@ -2624,6 +2612,7 @@ impl BackgroundScanner { .remove(&parent_abs_path); } + let (ignore_queue_tx, ignore_queue_rx) = channel::unbounded(); ignores_to_update.sort_unstable(); let mut ignores_to_update = ignores_to_update.into_iter().peekable(); while let Some(parent_abs_path) = ignores_to_update.next() { @@ -2642,8 +2631,34 @@ impl BackgroundScanner { })) .unwrap(); } + drop(ignore_queue_tx); + + self.executor + .scoped(|scope| { + for _ in 0..self.executor.num_cpus() { + scope.spawn(async { + loop { + select_biased! { + // Process any path refresh requests before moving on to process + // the queue of ignore statuses. + request = self.refresh_requests_rx.recv().fuse() => { + let Ok((paths, barrier)) = request else { break }; + if !self.process_refresh_request(paths, barrier).await { + return; + } + } - snapshot + // Recursively process directories whose ignores have changed. + job = ignore_queue_rx.recv().fuse() => { + let Ok(job) = job else { break }; + self.update_ignore_status(job, &snapshot).await; + } + } + } + }); + } + }) + .await; } async fn update_ignore_status(&self, job: UpdateIgnoreStatusJob, snapshot: &LocalSnapshot) { @@ -3054,12 +3069,11 @@ mod tests { use fs::repository::FakeGitRepository; use fs::{FakeFs, RealFs}; use gpui::{executor::Deterministic, TestAppContext}; + use pretty_assertions::assert_eq; use rand::prelude::*; use serde_json::json; use std::{env, fmt::Write}; - use util::http::FakeHttpClient; - - use util::test::temp_tree; + use util::{http::FakeHttpClient, test::temp_tree}; #[gpui::test] async fn test_traversal(cx: &mut TestAppContext) { @@ -3461,7 +3475,7 @@ mod tests { } #[gpui::test(iterations = 30)] - async fn test_create_directory(cx: &mut TestAppContext) { + async fn test_create_directory_during_initial_scan(cx: &mut TestAppContext) { let client = cx.read(|cx| Client::new(FakeHttpClient::with_404_response(), cx)); let fs = FakeFs::new(cx.background()); @@ -3486,6 +3500,8 @@ mod tests { .await .unwrap(); + let mut snapshot1 = tree.update(cx, |tree, _| tree.as_local().unwrap().snapshot()); + let entry = tree .update(cx, |tree, cx| { tree.as_local_mut() @@ -3497,10 +3513,14 @@ mod tests { assert!(entry.is_dir()); cx.foreground().run_until_parked(); - tree.read_with(cx, |tree, _| { assert_eq!(tree.entry_for_path("a/e").unwrap().kind, EntryKind::Dir); }); + + let snapshot2 = tree.update(cx, |tree, _| tree.as_local().unwrap().snapshot()); + let update = snapshot2.build_update(&snapshot1, 0, 0, true); + snapshot1.apply_remote_update(update).unwrap(); + assert_eq!(snapshot1.to_vec(true), snapshot2.to_vec(true),); } #[gpui::test(iterations = 100)] From bb1cfd51b83e396f99a9680bbac8284571c32a27 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 13 Apr 2023 22:34:03 -0700 Subject: [PATCH 02/17] Add randomized test for mutating worktree during initial scan --- crates/fs/src/fs.rs | 57 ++++++------ crates/project/src/worktree.rs | 165 +++++++++++++++++++++++++++++++-- 2 files changed, 188 insertions(+), 34 deletions(-) diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index c53c20c774bfd087dc914ecfeb9e596754298e54..d856b71e398c58940623773adf79334215e71807 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -523,31 +523,7 @@ impl FakeFs { } pub async fn insert_file(&self, path: impl AsRef, content: String) { - let mut state = self.state.lock(); - let path = path.as_ref(); - let inode = state.next_inode; - let mtime = state.next_mtime; - state.next_inode += 1; - state.next_mtime += Duration::from_nanos(1); - let file = Arc::new(Mutex::new(FakeFsEntry::File { - inode, - mtime, - content, - })); - state - .write_path(path, move |entry| { - match entry { - btree_map::Entry::Vacant(e) => { - e.insert(file); - } - btree_map::Entry::Occupied(mut e) => { - *e.get_mut() = file; - } - } - Ok(()) - }) - .unwrap(); - state.emit_event(&[path]); + self.write_file_internal(path, content).unwrap() } pub async fn insert_symlink(&self, path: impl AsRef, target: PathBuf) { @@ -569,6 +545,33 @@ impl FakeFs { state.emit_event(&[path]); } + fn write_file_internal(&self, path: impl AsRef, content: String) -> Result<()> { + let mut state = self.state.lock(); + let path = path.as_ref(); + let inode = state.next_inode; + let mtime = state.next_mtime; + state.next_inode += 1; + state.next_mtime += Duration::from_nanos(1); + let file = Arc::new(Mutex::new(FakeFsEntry::File { + inode, + mtime, + content, + })); + state.write_path(path, move |entry| { + match entry { + btree_map::Entry::Vacant(e) => { + e.insert(file); + } + btree_map::Entry::Occupied(mut e) => { + *e.get_mut() = file; + } + } + Ok(()) + })?; + state.emit_event(&[path]); + Ok(()) + } + pub async fn pause_events(&self) { self.state.lock().events_paused = true; } @@ -952,7 +955,7 @@ impl Fs for FakeFs { async fn atomic_write(&self, path: PathBuf, data: String) -> Result<()> { self.simulate_random_delay().await; let path = normalize_path(path.as_path()); - self.insert_file(path, data.to_string()).await; + self.write_file_internal(path, data.to_string())?; Ok(()) } @@ -961,7 +964,7 @@ impl Fs for FakeFs { self.simulate_random_delay().await; let path = normalize_path(path); let content = chunks(text, line_ending).collect(); - self.insert_file(path, content).await; + self.write_file_internal(path, content)?; Ok(()) } diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 29aec156104e05a76e2ce38fa59503a61bfe7aee..7a826740f14072b0d221bc465013b770058ffbe0 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -3523,6 +3523,83 @@ mod tests { assert_eq!(snapshot1.to_vec(true), snapshot2.to_vec(true),); } + #[gpui::test(iterations = 100)] + async fn test_random_worktree_operations_during_initial_scan( + cx: &mut TestAppContext, + mut rng: StdRng, + ) { + let operations = env::var("OPERATIONS") + .map(|o| o.parse().unwrap()) + .unwrap_or(5); + let initial_entries = env::var("INITIAL_ENTRIES") + .map(|o| o.parse().unwrap()) + .unwrap_or(20); + + let root_dir = Path::new("/test"); + let fs = FakeFs::new(cx.background()) as Arc; + fs.as_fake().insert_tree(root_dir, json!({})).await; + for _ in 0..initial_entries { + randomly_mutate_fs(&fs, root_dir, 1.0, &mut rng).await; + } + log::info!("generated initial tree"); + + let client = cx.read(|cx| Client::new(FakeHttpClient::with_404_response(), cx)); + let worktree = Worktree::local( + client.clone(), + root_dir, + true, + fs.clone(), + Default::default(), + &mut cx.to_async(), + ) + .await + .unwrap(); + + let mut snapshot = worktree.update(cx, |tree, _| tree.as_local().unwrap().snapshot()); + + for _ in 0..operations { + worktree + .update(cx, |worktree, cx| { + randomly_mutate_worktree(worktree, &mut rng, cx) + }) + .await + .log_err(); + worktree.read_with(cx, |tree, _| { + tree.as_local().unwrap().snapshot.check_invariants() + }); + + if rng.gen_bool(0.6) { + let new_snapshot = + worktree.read_with(cx, |tree, _| tree.as_local().unwrap().snapshot()); + let update = new_snapshot.build_update(&snapshot, 0, 0, true); + snapshot.apply_remote_update(update.clone()).unwrap(); + assert_eq!( + snapshot.to_vec(true), + new_snapshot.to_vec(true), + "incorrect snapshot after update {:?}", + update + ); + } + } + + worktree + .update(cx, |tree, _| tree.as_local_mut().unwrap().scan_complete()) + .await; + worktree.read_with(cx, |tree, _| { + tree.as_local().unwrap().snapshot.check_invariants() + }); + + let new_snapshot = worktree.read_with(cx, |tree, _| tree.as_local().unwrap().snapshot()); + let update = new_snapshot.build_update(&snapshot, 0, 0, true); + snapshot.apply_remote_update(update.clone()).unwrap(); + assert_eq!( + snapshot.to_vec(true), + new_snapshot.to_vec(true), + "incorrect snapshot after update {:?}", + update + ); + } + #[gpui::test(iterations = 100)] async fn test_random_worktree_changes(cx: &mut TestAppContext, mut rng: StdRng) { let operations = env::var("OPERATIONS") @@ -3536,18 +3613,17 @@ mod tests { let fs = FakeFs::new(cx.background()) as Arc; fs.as_fake().insert_tree(root_dir, json!({})).await; for _ in 0..initial_entries { - randomly_mutate_tree(&fs, root_dir, 1.0, &mut rng).await; + randomly_mutate_fs(&fs, root_dir, 1.0, &mut rng).await; } log::info!("generated initial tree"); - let next_entry_id = Arc::new(AtomicUsize::default()); let client = cx.read(|cx| Client::new(FakeHttpClient::with_404_response(), cx)); let worktree = Worktree::local( client.clone(), root_dir, true, fs.clone(), - next_entry_id.clone(), + Default::default(), &mut cx.to_async(), ) .await @@ -3603,14 +3679,14 @@ mod tests { let mut snapshots = Vec::new(); let mut mutations_len = operations; while mutations_len > 1 { - randomly_mutate_tree(&fs, root_dir, 1.0, &mut rng).await; + randomly_mutate_fs(&fs, root_dir, 1.0, &mut rng).await; let buffered_event_count = fs.as_fake().buffered_event_count().await; if buffered_event_count > 0 && rng.gen_bool(0.3) { let len = rng.gen_range(0..=buffered_event_count); log::info!("flushing {} events", len); fs.as_fake().flush_events(len).await; } else { - randomly_mutate_tree(&fs, root_dir, 0.6, &mut rng).await; + randomly_mutate_fs(&fs, root_dir, 0.6, &mut rng).await; mutations_len -= 1; } @@ -3635,7 +3711,7 @@ mod tests { root_dir, true, fs.clone(), - next_entry_id, + Default::default(), &mut cx.to_async(), ) .await @@ -3679,7 +3755,67 @@ mod tests { } } - async fn randomly_mutate_tree( + fn randomly_mutate_worktree( + worktree: &mut Worktree, + rng: &mut impl Rng, + cx: &mut ModelContext, + ) -> Task> { + let worktree = worktree.as_local_mut().unwrap(); + let snapshot = worktree.snapshot(); + let entry = snapshot.entries(false).choose(rng).unwrap(); + + match rng.gen_range(0_u32..100) { + 0..=33 if entry.path.as_ref() != Path::new("") => { + log::info!("deleting entry {:?} ({})", entry.path, entry.id.0); + worktree.delete_entry(entry.id, cx).unwrap() + } + ..=66 if entry.path.as_ref() != Path::new("") => { + let other_entry = snapshot.entries(false).choose(rng).unwrap(); + let new_parent_path = if other_entry.is_dir() { + other_entry.path.clone() + } else { + other_entry.path.parent().unwrap().into() + }; + let mut new_path = new_parent_path.join(gen_name(rng)); + if new_path.starts_with(&entry.path) { + new_path = gen_name(rng).into(); + } + + log::info!( + "renaming entry {:?} ({}) to {:?}", + entry.path, + entry.id.0, + new_path + ); + let task = worktree.rename_entry(entry.id, new_path, cx).unwrap(); + cx.foreground().spawn(async move { + task.await?; + Ok(()) + }) + } + _ => { + let task = if entry.is_dir() { + let child_path = entry.path.join(gen_name(rng)); + let is_dir = rng.gen_bool(0.3); + log::info!( + "creating {} at {:?}", + if is_dir { "dir" } else { "file" }, + child_path, + ); + worktree.create_entry(child_path, is_dir, cx) + } else { + log::info!("overwriting file {:?} ({})", entry.path, entry.id.0); + worktree.write_file(entry.path.clone(), "".into(), Default::default(), cx) + }; + cx.foreground().spawn(async move { + task.await?; + Ok(()) + }) + } + } + } + + async fn randomly_mutate_fs( fs: &Arc, root_path: &Path, insertion_probability: f64, @@ -3847,6 +3983,20 @@ mod tests { impl LocalSnapshot { fn check_invariants(&self) { + assert_eq!( + self.entries_by_path + .cursor::<()>() + .map(|e| (&e.path, e.id)) + .collect::>(), + self.entries_by_id + .cursor::<()>() + .map(|e| (&e.path, e.id)) + .collect::>() + .into_iter() + .collect::>(), + "entries_by_path and entries_by_id are inconsistent" + ); + let mut files = self.files(true, 0); let mut visible_files = self.files(false, 0); for entry in self.entries_by_path.cursor::<()>() { @@ -3857,6 +4007,7 @@ mod tests { } } } + assert!(files.next().is_none()); assert!(visible_files.next().is_none()); From 5ea49b3ae3d37710f2b97a04b7338dbb56283942 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 13 Apr 2023 22:34:34 -0700 Subject: [PATCH 03/17] Fix inconsistent worktree state when renaming entries while scanning --- crates/project/src/worktree.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index 7a826740f14072b0d221bc465013b770058ffbe0..d0cf2faa7eedb0dc76ab277fbf47ba67d3574815 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -1491,7 +1491,12 @@ impl LocalSnapshot { } let scan_id = self.scan_id; - self.entries_by_path.insert_or_replace(entry.clone(), &()); + let removed = self.entries_by_path.insert_or_replace(entry.clone(), &()); + if let Some(removed) = removed { + if removed.id != entry.id { + self.entries_by_id.remove(&removed.id, &()); + } + } self.entries_by_id.insert_or_replace( PathEntry { id: entry.id, From 253411bfd0f7db89acbf18a2a0cbe58107566112 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 14 Apr 2023 11:41:52 -0700 Subject: [PATCH 04/17] Start work randomized test runner GH action workflow --- .github/workflows/randomized_tests.yml | 51 ++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 .github/workflows/randomized_tests.yml diff --git a/.github/workflows/randomized_tests.yml b/.github/workflows/randomized_tests.yml new file mode 100644 index 0000000000000000000000000000000000000000..b9f5bbcbd37ca276fbcc976cbb162590dde22810 --- /dev/null +++ b/.github/workflows/randomized_tests.yml @@ -0,0 +1,51 @@ +name: Randomized Tests + +concurrency: randomized-tests + +on: + push: + branches: + - main + - randomized-tests-runner + schedule: + - cron: '*/15 * * * *' + +env: + CARGO_TERM_COLOR: always + CARGO_INCREMENTAL: 0 + RUST_BACKTRACE: 1 + OPERATIONS: 200 + ITERATIONS: 10000 + +jobs: + tests: + name: Run randomized tests + runs-on: + - self-hosted + - randomized-tests + steps: + - name: Install Rust + run: | + rustup set profile minimal + rustup update stable + + - name: Install Node + uses: actions/setup-node@v2 + with: + node-version: '16' + + - name: Checkout repo + uses: actions/checkout@v2 + with: + clean: false + submodules: 'recursive' + + - name: Select seed + run: | + set -eu + seed=$(od -A n -N 8 -t u8 /dev/urandom | xargs) + echo "seed: ${seed}" + echo "SEED=${seed}" >> $GITHUB_ENV + + - name: Run tests + run: cargo test --release --package collab random From c329546570afb29decbf14e0e74270f52dd9b307 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 14 Apr 2023 14:25:55 -0700 Subject: [PATCH 05/17] Extract randomized test CI process into a script --- .github/workflows/randomized_tests.yml | 13 ++----------- crates/gpui/src/test.rs | 2 +- script/randomized-test-ci | 27 ++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 12 deletions(-) create mode 100755 script/randomized-test-ci diff --git a/.github/workflows/randomized_tests.yml b/.github/workflows/randomized_tests.yml index b9f5bbcbd37ca276fbcc976cbb162590dde22810..3b3d3b21cd9395be5032f54b8b63d3e547c81fa7 100644 --- a/.github/workflows/randomized_tests.yml +++ b/.github/workflows/randomized_tests.yml @@ -14,8 +14,6 @@ env: CARGO_TERM_COLOR: always CARGO_INCREMENTAL: 0 RUST_BACKTRACE: 1 - OPERATIONS: 200 - ITERATIONS: 10000 jobs: tests: @@ -40,12 +38,5 @@ jobs: clean: false submodules: 'recursive' - - name: Select seed - run: | - set -eu - seed=$(od -A n -N 8 -t u8 /dev/urandom | xargs) - echo "seed: ${seed}" - echo "SEED=${seed}" >> $GITHUB_ENV - - - name: Run tests - run: cargo test --release --package collab random + - name: Run randomized tests + run: script/randomized-test-ci diff --git a/crates/gpui/src/test.rs b/crates/gpui/src/test.rs index 372131b06c46a5578e1ea603eff3045d3651738f..cacb8a435a922a2044e263b9567cd19582d1d838 100644 --- a/crates/gpui/src/test.rs +++ b/crates/gpui/src/test.rs @@ -76,7 +76,7 @@ pub fn run_test( let seed = atomic_seed.load(SeqCst); if is_randomized { - dbg!(seed); + eprintln!("seed = {seed}"); } let deterministic = executor::Deterministic::new(seed); diff --git a/script/randomized-test-ci b/script/randomized-test-ci new file mode 100755 index 0000000000000000000000000000000000000000..e8837d42b2b39521095325147f7318b5963fe979 --- /dev/null +++ b/script/randomized-test-ci @@ -0,0 +1,27 @@ +#!/bin/bash + +# Compile the tests first +mkdir -p target +cargo test --release --package collab --no-run +if [[ $? != 0 ]]; then + echo "Build failed" + exit 1 +fi + +set -eu + +LOG_FILE=target/randomized-tests.log +export SAVE_PLAN=target/test-plan.json +export OPERATIONS=200 +export ITERATIONS=10000 +export SEED=$(od -A n -N 8 -t u8 /dev/urandom | xargs) + +cargo test --release --package collab random -- --nocapture 2> >(tee $LOG_FILE) +if [[ $? == 0 ]]; then + echo "Tests passed" + exit 0 +fi + +# If the tests failed, find the failing seed in the logs +failing_seed=$(grep "failing seed" $LOG_FILE | cut -d: -f2 | xargs) +echo "Tests failed. seed: $failing_seed" From 5c3da91e15b052d64f786c0c28d796d13e9f2689 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 14 Apr 2023 15:36:55 -0700 Subject: [PATCH 06/17] Report randomized test failures to zed.dev, to create issues in linear --- .github/workflows/randomized_tests.yml | 4 ++- script/randomized-test-ci | 35 ++++++++++++++++++++------ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/.github/workflows/randomized_tests.yml b/.github/workflows/randomized_tests.yml index 3b3d3b21cd9395be5032f54b8b63d3e547c81fa7..f180313ed349d3e704fa27126a162bd8253c2a9d 100644 --- a/.github/workflows/randomized_tests.yml +++ b/.github/workflows/randomized_tests.yml @@ -8,12 +8,14 @@ on: - main - randomized-tests-runner schedule: - - cron: '*/15 * * * *' + - cron: '0 * * * *' env: CARGO_TERM_COLOR: always CARGO_INCREMENTAL: 0 RUST_BACKTRACE: 1 + ZED_SERVER_URL: https://zed.dev + ZED_CLIENT_SECRET_TOKEN: ${{ secrets.ZED_CLIENT_SECRET_TOKEN }} jobs: tests: diff --git a/script/randomized-test-ci b/script/randomized-test-ci index e8837d42b2b39521095325147f7318b5963fe979..556bc34b6b7f21cb5ec0570a27acc55e085a36c1 100755 --- a/script/randomized-test-ci +++ b/script/randomized-test-ci @@ -1,27 +1,48 @@ #!/bin/bash +set -u + +: $ZED_SERVER_URL +: $ZED_CLIENT_SECRET_TOKEN + # Compile the tests first mkdir -p target -cargo test --release --package collab --no-run +cargo test --release --lib --package collab --no-run if [[ $? != 0 ]]; then echo "Build failed" exit 1 fi -set -eu - LOG_FILE=target/randomized-tests.log export SAVE_PLAN=target/test-plan.json export OPERATIONS=200 -export ITERATIONS=10000 +export ITERATIONS=100000 export SEED=$(od -A n -N 8 -t u8 /dev/urandom | xargs) -cargo test --release --package collab random -- --nocapture 2> >(tee $LOG_FILE) +echo "Starting seed: ${SEED}" + +cargo test --release --lib --package collab random 2>&1 > $LOG_FILE if [[ $? == 0 ]]; then echo "Tests passed" exit 0 fi # If the tests failed, find the failing seed in the logs -failing_seed=$(grep "failing seed" $LOG_FILE | cut -d: -f2 | xargs) -echo "Tests failed. seed: $failing_seed" +commit=$(git rev-parse HEAD) +failing_seed=$(grep "failing seed" $LOG_FILE | tail -n1 | cut -d: -f2 | xargs) +failing_plan=$(cat $SAVE_PLAN) +request="{ + \"seed\": \"${failing_seed}\", + \"commit\": \"${commit}\", + \"token\": \"${ZED_CLIENT_SECRET_TOKEN}\", + \"plan\": ${failing_plan} +}" + +echo "Reporting test failure." +echo $request + +curl \ + -X POST \ + -H "Content-Type: application/json" \ + -d "${request}" \ + "${ZED_SERVER_URL}/api/randomized_test_failure" From 3569c617848c92612a7d1803637530aa79a983a3 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 14 Apr 2023 17:47:00 -0700 Subject: [PATCH 07/17] Minimize randomized test failures before reporting issues --- script/randomized-test-ci | 6 +- script/randomized-test-minimize | 104 ++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) create mode 100755 script/randomized-test-minimize diff --git a/script/randomized-test-ci b/script/randomized-test-ci index 556bc34b6b7f21cb5ec0570a27acc55e085a36c1..3241bbcca06fa52fc0614226674e07cf915a7883 100755 --- a/script/randomized-test-ci +++ b/script/randomized-test-ci @@ -14,6 +14,7 @@ if [[ $? != 0 ]]; then fi LOG_FILE=target/randomized-tests.log +MIN_PLAN=target/test-plan.min.json export SAVE_PLAN=target/test-plan.json export OPERATIONS=200 export ITERATIONS=100000 @@ -27,10 +28,11 @@ if [[ $? == 0 ]]; then exit 0 fi +failing_seed=$(script/randomized-test-minimize $SAVE_PLAN $MIN_PLAN) + # If the tests failed, find the failing seed in the logs commit=$(git rev-parse HEAD) -failing_seed=$(grep "failing seed" $LOG_FILE | tail -n1 | cut -d: -f2 | xargs) -failing_plan=$(cat $SAVE_PLAN) +failing_plan=$(cat $MIN_PLAN) request="{ \"seed\": \"${failing_seed}\", \"commit\": \"${commit}\", diff --git a/script/randomized-test-minimize b/script/randomized-test-minimize new file mode 100755 index 0000000000000000000000000000000000000000..fe2686b69b8fa66e107a8e7f356590abdc4323c3 --- /dev/null +++ b/script/randomized-test-minimize @@ -0,0 +1,104 @@ +#!/usr/bin/env node --redirect-warnings=/dev/null + +const fs = require('fs') +const path = require('path') +const {spawnSync} = require('child_process') + +if (process.argv.length < 4) { + process.stderr.write("usage: script/randomized-test-minimize [start-index]\n") + process.exit(1) +} + +const inputPlanPath = process.argv[2] +const outputPlanPath = process.argv[3] +const startIndex = parseInt(process.argv[4]) || 0 + +const tempPlanPath = inputPlanPath + '.try' + +const FAILING_SEED_REGEX = /failing seed: (\d+)/ig + +fs.copyFileSync(inputPlanPath, outputPlanPath) +let testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) + +process.stderr.write("minimizing failing test plan...\n") +for (let ix = startIndex; ix < testPlan.length; ix++) { + // Skip 'MutateClients' entries, since they themselves are not single operations. + if (testPlan[ix].MutateClients) { + continue + } + + // Remove a row from the test plan + const newTestPlan = testPlan.slice() + newTestPlan.splice(ix, 1) + fs.writeFileSync(tempPlanPath, serializeTestPlan(newTestPlan), 'utf8'); + + process.stderr.write(`${ix}/${testPlan.length}: ${JSON.stringify(testPlan[ix])}`) + + const failingSeed = runTestsForPlan(tempPlanPath, 500) + + // If the test failed, keep the test plan with the removed row. Reload the test + // plan from the JSON file, since the test itself will remove any operations + // which are no longer valid before saving the test plan. + if (failingSeed != null) { + process.stderr.write(` - remove. failing seed: ${failingSeed}.\n`) + fs.copyFileSync(tempPlanPath, outputPlanPath) + testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) + ix-- + } else { + process.stderr.write(` - keep.\n`) + } +} + +fs.unlinkSync(tempPlanPath) + +// Re-run the final minimized plan to get the correct failing seed. +// This is a workaround for the fact that the execution order can +// slightly change when replaying a test plan after it has been +// saved and loaded. +const failingSeed = runTestsForPlan(outputPlanPath, 5000) + +process.stderr.write(`final test plan: ${outputPlanPath}\n`) +process.stderr.write(`final seed: ${failingSeed}\n`) +console.log(failingSeed) + +function runTestsForPlan(path, iterations) { + const {status, stdout, stderr} = spawnSync( + 'cargo', + [ + 'test', + '--release', + '--lib', + '--package', 'collab', + 'random_collaboration' + ], + { + stdio: 'pipe', + encoding: 'utf8', + env: { + ...process.env, + 'SEED': 0, + 'LOAD_PLAN': path, + 'SAVE_PLAN': path, + 'ITERATIONS': String(iterations), + } + } + ); + + if (status !== 0) { + FAILING_SEED_REGEX.lastIndex = 0 + const match = FAILING_SEED_REGEX.exec(stdout) + if (!match) { + process.stderr.write("test failed, but no failing seed found:\n") + process.stderr.write(stdout) + process.stderr.write('\n') + process.exit(1) + } + return match[1] + } else { + return null + } +} + +function serializeTestPlan(plan) { + return "[\n" + plan.map(row => JSON.stringify(row)).join(",\n") + "\n]\n" +} From 61f4f8aaeb32699d3015346400347ea56e17b124 Mon Sep 17 00:00:00 2001 From: Julia Date: Mon, 17 Apr 2023 13:10:40 -0400 Subject: [PATCH 08/17] Unconditionally display followers in full color --- crates/collab_ui/src/collab_titlebar_item.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/collab_ui/src/collab_titlebar_item.rs b/crates/collab_ui/src/collab_titlebar_item.rs index a505df316e0ede594e0a41d7da6f44e891d83e31..5e992ae559afcf5ad1d9dd338c2e8396b4e992f5 100644 --- a/crates/collab_ui/src/collab_titlebar_item.rs +++ b/crates/collab_ui/src/collab_titlebar_item.rs @@ -710,11 +710,9 @@ impl CollabTitlebarItem { } })?; - let location = remote_participant.map(|p| p.location); - Some(Self::render_face( avatar.clone(), - Self::location_style(workspace, location, follower_style, cx), + follower_style, background_color, )) })) From 1f284408a97496b4dd4f87cc6df4e26ae237ffba Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 17 Apr 2023 19:15:07 +0200 Subject: [PATCH 09/17] Send buffer operations in batches to reduce latency Co-Authored-By: Max Brunsfeld --- crates/project/src/project.rs | 56 ++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 9192c7a411de149b7d7daa715ccd4eb00cb73f70..2deada6a5cb83faa21fdd0b69fe643cdb5bab779 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -1733,13 +1733,19 @@ impl Project { async fn send_buffer_messages( this: WeakModelHandle, - mut rx: UnboundedReceiver, + rx: UnboundedReceiver, mut cx: AsyncAppContext, - ) { + ) -> Option<()> { + const MAX_BATCH_SIZE: usize = 128; + let mut needs_resync_with_host = false; - while let Some(change) = rx.next().await { - if let Some(this) = this.upgrade(&mut cx) { - let is_local = this.read_with(&cx, |this, _| this.is_local()); + let mut operations_by_buffer_id = HashMap::default(); + let mut changes = rx.ready_chunks(MAX_BATCH_SIZE); + while let Some(changes) = changes.next().await { + let this = this.upgrade(&mut cx)?; + let is_local = this.read_with(&cx, |this, _| this.is_local()); + + for change in changes { match change { BufferMessage::Operation { buffer_id, @@ -1748,21 +1754,14 @@ impl Project { if needs_resync_with_host { continue; } - let request = this.read_with(&cx, |this, _| { - let project_id = this.remote_id()?; - Some(this.client.request(proto::UpdateBuffer { - buffer_id, - project_id, - operations: vec![operation], - })) - }); - if let Some(request) = request { - if request.await.is_err() && !is_local { - needs_resync_with_host = true; - } - } + + operations_by_buffer_id + .entry(buffer_id) + .or_insert(Vec::new()) + .push(operation); } BufferMessage::Resync => { + operations_by_buffer_id.clear(); if this .update(&mut cx, |this, cx| this.synchronize_remote_buffers(cx)) .await @@ -1772,10 +1771,27 @@ impl Project { } } } - } else { - break; + } + + for (buffer_id, operations) in operations_by_buffer_id.drain() { + let request = this.read_with(&cx, |this, _| { + let project_id = this.remote_id()?; + Some(this.client.request(proto::UpdateBuffer { + buffer_id, + project_id, + operations, + })) + }); + if let Some(request) = request { + if request.await.is_err() && !is_local { + needs_resync_with_host = true; + break; + } + } } } + + None } fn on_buffer_event( From 5d73e646d8b4c049931f36b8c7f89c61d945fb18 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 17 Apr 2023 10:23:57 -0700 Subject: [PATCH 10/17] Delete pull_request_template.md --- .github/pull_request_template.md | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md deleted file mode 100644 index 6826566e717682008315d4acceb3057a18f322fe..0000000000000000000000000000000000000000 --- a/.github/pull_request_template.md +++ /dev/null @@ -1,9 +0,0 @@ -## Description of feature or change - -## Link to related issues from zed or community - -## Before Merging - -- [ ] Does this have tests or have existing tests been updated to cover this change? -- [ ] Have you added the necessary settings to configure this feature? -- [ ] Has documentation been created or updated (including above changes to settings)? From 14ef0edd7ffc4fe1b51e754682457816c99ae585 Mon Sep 17 00:00:00 2001 From: Julia Date: Mon, 17 Apr 2023 14:13:28 -0400 Subject: [PATCH 11/17] Don't have contacts popover affect appearance of Share/Unshare button --- crates/collab_ui/src/collab_titlebar_item.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/collab_ui/src/collab_titlebar_item.rs b/crates/collab_ui/src/collab_titlebar_item.rs index 5e992ae559afcf5ad1d9dd338c2e8396b4e992f5..b0b734f9cfda59871130fb5a2ced1b2700651902 100644 --- a/crates/collab_ui/src/collab_titlebar_item.rs +++ b/crates/collab_ui/src/collab_titlebar_item.rs @@ -459,9 +459,7 @@ impl CollabTitlebarItem { .with_child( MouseEventHandler::::new(0, cx, |state, _| { //TODO: Ensure this button has consistant width for both text variations - let style = titlebar - .share_button - .style_for(state, self.contacts_popover.is_some()); + let style = titlebar.share_button.style_for(state, false); Label::new(label, style.text.clone()) .contained() .with_style(style.container) From 837866f9622069a8c3196bbfaa45000ab43f25df Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 17 Apr 2023 15:34:23 -0700 Subject: [PATCH 12/17] Consolidate logic for running randomized tests in scripts --- script/randomized-test-ci | 113 ++++++++++++---------- script/randomized-test-minimize | 166 +++++++++++++++++++------------- 2 files changed, 160 insertions(+), 119 deletions(-) diff --git a/script/randomized-test-ci b/script/randomized-test-ci index 3241bbcca06fa52fc0614226674e07cf915a7883..31fdea6aee8973f3c954ecfa93014cb808ed90ff 100755 --- a/script/randomized-test-ci +++ b/script/randomized-test-ci @@ -1,50 +1,63 @@ -#!/bin/bash - -set -u - -: $ZED_SERVER_URL -: $ZED_CLIENT_SECRET_TOKEN - -# Compile the tests first -mkdir -p target -cargo test --release --lib --package collab --no-run -if [[ $? != 0 ]]; then - echo "Build failed" - exit 1 -fi - -LOG_FILE=target/randomized-tests.log -MIN_PLAN=target/test-plan.min.json -export SAVE_PLAN=target/test-plan.json -export OPERATIONS=200 -export ITERATIONS=100000 -export SEED=$(od -A n -N 8 -t u8 /dev/urandom | xargs) - -echo "Starting seed: ${SEED}" - -cargo test --release --lib --package collab random 2>&1 > $LOG_FILE -if [[ $? == 0 ]]; then - echo "Tests passed" - exit 0 -fi - -failing_seed=$(script/randomized-test-minimize $SAVE_PLAN $MIN_PLAN) - -# If the tests failed, find the failing seed in the logs -commit=$(git rev-parse HEAD) -failing_plan=$(cat $MIN_PLAN) -request="{ - \"seed\": \"${failing_seed}\", - \"commit\": \"${commit}\", - \"token\": \"${ZED_CLIENT_SECRET_TOKEN}\", - \"plan\": ${failing_plan} -}" - -echo "Reporting test failure." -echo $request - -curl \ - -X POST \ - -H "Content-Type: application/json" \ - -d "${request}" \ - "${ZED_SERVER_URL}/api/randomized_test_failure" +#!/usr/bin/env node --redirect-warnings=/dev/null + +const fs = require('fs') +const {randomBytes} = require('crypto') +const {execFileSync} = require('child_process') +const {minimizeTestPlan, buildTests, runTests} = require('./randomized-test-minimize'); + +const {ZED_SERVER_URL, ZED_CLIENT_SECRET_TOKEN} = process.env +if (!ZED_SERVER_URL) throw new Error('Missing env var `ZED_SERVER_URL`') +if (!ZED_CLIENT_SECRET_TOKEN) throw new Error('Missing env var `ZED_CLIENT_SECRET_TOKEN`') + +main() + +async function main() { + buildTests() + + const seed = randomU64(); + const commit = execFileSync( + 'git', + ['rev-parse', 'HEAD'], + {encoding: 'utf8'} + ).trim() + + console.log("commit:", commit) + console.log("starting seed:", seed) + + const planPath = 'target/test-plan.json' + const minPlanPath = 'target/test-plan.min.json' + const failingSeed = runTests({ + SEED: seed, + SAVE_PLAN: planPath, + ITERATIONS: 50000, + OPERATIONS: 200, + }) + + if (!failingSeed) { + console.log("tests passed") + return + } + + console.log("found failure at seed", failingSeed) + const minimizedSeed = minimizeTestPlan(planPath, minPlanPath) + const minimizedPlan = JSON.parse(fs.readFileSync(minPlanPath, 'utf8')) + + const url = `${ZED_SERVER_URL}/api/randomized_test_failure` + const body = { + seed: minimizedSeed, + token: ZED_CLIENT_SECRET_TOKEN, + plan: minimizedPlan, + commit: commit, + } + await fetch(url, { + method: 'POST', + headers: {"Content-Type": "application/json"}, + body: JSON.stringify(body) + }) +} + +function randomU64() { + const bytes = randomBytes(8) + const hexString = bytes.reduce(((string, byte) => string + byte.toString(16)), '') + return BigInt('0x' + hexString).toString(10) +} diff --git a/script/randomized-test-minimize b/script/randomized-test-minimize index fe2686b69b8fa66e107a8e7f356590abdc4323c3..ce0b7203b4be127430ee4d8e24aad2a9fb32f4d9 100755 --- a/script/randomized-test-minimize +++ b/script/randomized-test-minimize @@ -4,85 +4,109 @@ const fs = require('fs') const path = require('path') const {spawnSync} = require('child_process') -if (process.argv.length < 4) { - process.stderr.write("usage: script/randomized-test-minimize [start-index]\n") - process.exit(1) -} - -const inputPlanPath = process.argv[2] -const outputPlanPath = process.argv[3] -const startIndex = parseInt(process.argv[4]) || 0 - -const tempPlanPath = inputPlanPath + '.try' - const FAILING_SEED_REGEX = /failing seed: (\d+)/ig - -fs.copyFileSync(inputPlanPath, outputPlanPath) -let testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) - -process.stderr.write("minimizing failing test plan...\n") -for (let ix = startIndex; ix < testPlan.length; ix++) { - // Skip 'MutateClients' entries, since they themselves are not single operations. - if (testPlan[ix].MutateClients) { - continue +const CARGO_TEST_ARGS = [ + '--release', + '--lib', + '--package', 'collab', + 'random_collaboration', +] + +if (require.main === module) { + if (process.argv.length < 4) { + process.stderr.write("usage: script/randomized-test-minimize [start-index]\n") + process.exit(1) } - // Remove a row from the test plan - const newTestPlan = testPlan.slice() - newTestPlan.splice(ix, 1) - fs.writeFileSync(tempPlanPath, serializeTestPlan(newTestPlan), 'utf8'); + minimizeTestPlan( + process.argv[2], + process.argv[3], + parseInt(process.argv[4]) || 0 + ); +} + +function minimizeTestPlan( + inputPlanPath, + outputPlanPath, + startIndex = 0 +) { + const tempPlanPath = inputPlanPath + '.try' + + fs.copyFileSync(inputPlanPath, outputPlanPath) + let testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) + + process.stderr.write("minimizing failing test plan...\n") + for (let ix = startIndex; ix < testPlan.length; ix++) { + // Skip 'MutateClients' entries, since they themselves are not single operations. + if (testPlan[ix].MutateClients) { + continue + } - process.stderr.write(`${ix}/${testPlan.length}: ${JSON.stringify(testPlan[ix])}`) + // Remove a row from the test plan + const newTestPlan = testPlan.slice() + newTestPlan.splice(ix, 1) + fs.writeFileSync(tempPlanPath, serializeTestPlan(newTestPlan), 'utf8'); + + process.stderr.write(`${ix}/${testPlan.length}: ${JSON.stringify(testPlan[ix])}`) + const failingSeed = runTests({ + SEED: '0', + LOAD_PLAN: tempPlanPath, + SAVE_PLAN: tempPlanPath, + ITERATIONS: '500' + }) + + // If the test failed, keep the test plan with the removed row. Reload the test + // plan from the JSON file, since the test itself will remove any operations + // which are no longer valid before saving the test plan. + if (failingSeed != null) { + process.stderr.write(` - remove. failing seed: ${failingSeed}.\n`) + fs.copyFileSync(tempPlanPath, outputPlanPath) + testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) + ix-- + } else { + process.stderr.write(` - keep.\n`) + } + } - const failingSeed = runTestsForPlan(tempPlanPath, 500) + fs.unlinkSync(tempPlanPath) + + // Re-run the final minimized plan to get the correct failing seed. + // This is a workaround for the fact that the execution order can + // slightly change when replaying a test plan after it has been + // saved and loaded. + const failingSeed = runTests({ + SEED: '0', + ITERATIONS: '5000', + LOAD_PLAN: outputPlanPath, + }) + + process.stderr.write(`final test plan: ${outputPlanPath}\n`) + process.stderr.write(`final seed: ${failingSeed}\n`) + return failingSeed +} - // If the test failed, keep the test plan with the removed row. Reload the test - // plan from the JSON file, since the test itself will remove any operations - // which are no longer valid before saving the test plan. - if (failingSeed != null) { - process.stderr.write(` - remove. failing seed: ${failingSeed}.\n`) - fs.copyFileSync(tempPlanPath, outputPlanPath) - testPlan = JSON.parse(fs.readFileSync(outputPlanPath, 'utf8')) - ix-- - } else { - process.stderr.write(` - keep.\n`) +function buildTests() { + const {status} = spawnSync('cargo', ['test', '--no-run', ...CARGO_TEST_ARGS], { + stdio: 'inherit', + encoding: 'utf8', + env: { + ...process.env, + } + }); + if (status !== 0) { + throw new Error('build failed') } } -fs.unlinkSync(tempPlanPath) - -// Re-run the final minimized plan to get the correct failing seed. -// This is a workaround for the fact that the execution order can -// slightly change when replaying a test plan after it has been -// saved and loaded. -const failingSeed = runTestsForPlan(outputPlanPath, 5000) - -process.stderr.write(`final test plan: ${outputPlanPath}\n`) -process.stderr.write(`final seed: ${failingSeed}\n`) -console.log(failingSeed) - -function runTestsForPlan(path, iterations) { - const {status, stdout, stderr} = spawnSync( - 'cargo', - [ - 'test', - '--release', - '--lib', - '--package', 'collab', - 'random_collaboration' - ], - { - stdio: 'pipe', - encoding: 'utf8', - env: { - ...process.env, - 'SEED': 0, - 'LOAD_PLAN': path, - 'SAVE_PLAN': path, - 'ITERATIONS': String(iterations), - } +function runTests(env) { + const {status, stdout} = spawnSync('cargo', ['test', ...CARGO_TEST_ARGS], { + stdio: 'pipe', + encoding: 'utf8', + env: { + ...process.env, + ...env, } - ); + }); if (status !== 0) { FAILING_SEED_REGEX.lastIndex = 0 @@ -102,3 +126,7 @@ function runTestsForPlan(path, iterations) { function serializeTestPlan(plan) { return "[\n" + plan.map(row => JSON.stringify(row)).join(",\n") + "\n]\n" } + +exports.buildTests = buildTests +exports.runTests = runTests +exports.minimizeTestPlan = minimizeTestPlan From 485c56e3bd9c2a5e2c86756088366aa59d5f63e4 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 17 Apr 2023 15:43:12 -0700 Subject: [PATCH 13/17] Don't run randomized tests on pushes to main --- .github/workflows/randomized_tests.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/randomized_tests.yml b/.github/workflows/randomized_tests.yml index f180313ed349d3e704fa27126a162bd8253c2a9d..c3579ad41ea41d951eb7597d8620eeef7fda97af 100644 --- a/.github/workflows/randomized_tests.yml +++ b/.github/workflows/randomized_tests.yml @@ -5,7 +5,6 @@ concurrency: randomized-tests on: push: branches: - - main - randomized-tests-runner schedule: - cron: '0 * * * *' From 695973d1171a99fc7716e2e75523095ffd34a631 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 17 Apr 2023 16:22:30 -0700 Subject: [PATCH 14/17] Use Node 18 on CI, for fetch API --- .github/workflows/ci.yml | 4 ++-- .github/workflows/randomized_tests.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a32f25fbe9bf0b96585b5e4242daff92216a34a2..2b7cb97efadd938bb1875f57515e4e75e0f52990 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -54,7 +54,7 @@ jobs: - name: Install Node uses: actions/setup-node@v2 with: - node-version: '16' + node-version: '18' - name: Checkout repo uses: actions/checkout@v2 @@ -102,7 +102,7 @@ jobs: - name: Install Node uses: actions/setup-node@v2 with: - node-version: '16' + node-version: '18' - name: Checkout repo uses: actions/checkout@v2 diff --git a/.github/workflows/randomized_tests.yml b/.github/workflows/randomized_tests.yml index c3579ad41ea41d951eb7597d8620eeef7fda97af..d963ac46e062c5272908c4bcde9b2455ada91dbf 100644 --- a/.github/workflows/randomized_tests.yml +++ b/.github/workflows/randomized_tests.yml @@ -31,7 +31,7 @@ jobs: - name: Install Node uses: actions/setup-node@v2 with: - node-version: '16' + node-version: '18' - name: Checkout repo uses: actions/checkout@v2 From 8f25b98e6f5cd276e40786ae27efdde6cef27cb8 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 17 Apr 2023 16:35:54 -0700 Subject: [PATCH 15/17] Print the final minimized test plan before sending it to zed.dev --- script/randomized-test-ci | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/script/randomized-test-ci b/script/randomized-test-ci index 31fdea6aee8973f3c954ecfa93014cb808ed90ff..7816ebfbbf22b336814f18b95175d73dc561ef36 100755 --- a/script/randomized-test-ci +++ b/script/randomized-test-ci @@ -40,13 +40,15 @@ async function main() { console.log("found failure at seed", failingSeed) const minimizedSeed = minimizeTestPlan(planPath, minPlanPath) - const minimizedPlan = JSON.parse(fs.readFileSync(minPlanPath, 'utf8')) + const minimizedPlan = fs.readFileSync(minPlanPath, 'utf8') + + console.log("minimized plan:\n", minimizedPlan) const url = `${ZED_SERVER_URL}/api/randomized_test_failure` const body = { seed: minimizedSeed, token: ZED_CLIENT_SECRET_TOKEN, - plan: minimizedPlan, + plan: JSON.parse(minimizedPlan), commit: commit, } await fetch(url, { From 75d6b6360f02143b2040c6e382c4a4e0318c5872 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 18 Apr 2023 10:24:20 +0200 Subject: [PATCH 16/17] Add failing test to demonstrate Copilot not showing enough suggestions --- crates/editor/src/editor_tests.rs | 41 +++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 74fad79fe83c50adda83708f88c3d68f2d2daec0..9cf3c4a81e517ed28657046fa4fe4b9be0554eeb 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -5897,13 +5897,12 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC ) .await; + // When inserting, ensure autocompletion is favored over Copilot suggestions. cx.set_state(indoc! {" oneˇ two three "}); - - // When inserting, ensure autocompletion is favored over Copilot suggestions. cx.simulate_keystroke("."); let _ = handle_completion_request( &mut cx, @@ -5917,8 +5916,8 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC handle_copilot_completion_request( &copilot_lsp, vec![copilot::request::Completion { - text: "copilot1".into(), - range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 5)), + text: "one.copilot1".into(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 4)), ..Default::default() }], vec![], @@ -5940,13 +5939,45 @@ async fn test_copilot(deterministic: Arc, cx: &mut gpui::TestAppC assert_eq!(editor.display_text(cx), "one.completion_a\ntwo\nthree\n"); }); + // Ensure Copilot suggestions are shown right away if no autocompletion is available. cx.set_state(indoc! {" oneˇ two three "}); + cx.simulate_keystroke("."); + let _ = handle_completion_request( + &mut cx, + indoc! {" + one.|<> + two + three + "}, + vec![], + ); + handle_copilot_completion_request( + &copilot_lsp, + vec![copilot::request::Completion { + text: "one.copilot1".into(), + range: lsp::Range::new(lsp::Position::new(0, 0), lsp::Position::new(0, 4)), + ..Default::default() + }], + vec![], + ); + deterministic.advance_clock(COPILOT_DEBOUNCE_TIMEOUT); + cx.update_editor(|editor, cx| { + assert!(!editor.context_menu_visible()); + assert!(editor.has_active_copilot_suggestion(cx)); + assert_eq!(editor.display_text(cx), "one.copilot1\ntwo\nthree\n"); + assert_eq!(editor.text(cx), "one.\ntwo\nthree\n"); + }); - // When inserting, ensure autocompletion is favored over Copilot suggestions. + // Reset editor, and ensure autocompletion is still favored over Copilot suggestions. + cx.set_state(indoc! {" + oneˇ + two + three + "}); cx.simulate_keystroke("."); let _ = handle_completion_request( &mut cx, From d26d0ac56fda9afd4899524dcfb5cb72e6d8e734 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 18 Apr 2023 10:34:18 +0200 Subject: [PATCH 17/17] Clean up completion tasks, even if they fail or return no results This fixes a bug where leaving the completion task in `completion_tasks` could cause the Copilot suggestion to not be shown due to the LSP not successfully return a completion. --- crates/editor/src/editor.rs | 93 +++++++++++++++++++++---------------- 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 6649e0a699f0dc976e08a2fda84cfd8d6f1cafbd..b3f3723bc1adb7d1fa828207e9a46b4517cc3b35 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -20,7 +20,7 @@ mod editor_tests; pub mod test; use aho_corasick::AhoCorasick; -use anyhow::Result; +use anyhow::{anyhow, Result}; use blink_manager::BlinkManager; use clock::ReplicaId; use collections::{BTreeMap, Bound, HashMap, HashSet, VecDeque}; @@ -2352,53 +2352,66 @@ impl Editor { let id = post_inc(&mut self.next_completion_id); let task = cx.spawn_weak(|this, mut cx| { async move { - let completions = completions.await?; - if completions.is_empty() { - return Ok(()); - } - - let mut menu = CompletionsMenu { - id, - initial_position: position, - match_candidates: completions - .iter() - .enumerate() - .map(|(id, completion)| { - StringMatchCandidate::new( - id, - completion.label.text[completion.label.filter_range.clone()].into(), - ) - }) - .collect(), - buffer, - completions: completions.into(), - matches: Vec::new().into(), - selected_item: 0, - list: Default::default(), + let menu = if let Some(completions) = completions.await.log_err() { + let mut menu = CompletionsMenu { + id, + initial_position: position, + match_candidates: completions + .iter() + .enumerate() + .map(|(id, completion)| { + StringMatchCandidate::new( + id, + completion.label.text[completion.label.filter_range.clone()] + .into(), + ) + }) + .collect(), + buffer, + completions: completions.into(), + matches: Vec::new().into(), + selected_item: 0, + list: Default::default(), + }; + menu.filter(query.as_deref(), cx.background()).await; + if menu.matches.is_empty() { + None + } else { + Some(menu) + } + } else { + None }; - menu.filter(query.as_deref(), cx.background()).await; + let this = this + .upgrade(&cx) + .ok_or_else(|| anyhow!("editor was dropped"))?; + this.update(&mut cx, |this, cx| { + this.completion_tasks.retain(|(task_id, _)| *task_id > id); - if let Some(this) = this.upgrade(&cx) { - this.update(&mut cx, |this, cx| { - match this.context_menu.as_ref() { - None => {} - Some(ContextMenu::Completions(prev_menu)) => { - if prev_menu.id > menu.id { - return; - } + match this.context_menu.as_ref() { + None => {} + Some(ContextMenu::Completions(prev_menu)) => { + if prev_menu.id > id { + return; } - _ => return, } + _ => return, + } - this.completion_tasks.retain(|(id, _)| *id > menu.id); - if this.focused && !menu.matches.is_empty() { - this.show_context_menu(ContextMenu::Completions(menu), cx); - } else if this.hide_context_menu(cx).is_none() { + if this.focused && menu.is_some() { + let menu = menu.unwrap(); + this.show_context_menu(ContextMenu::Completions(menu), cx); + } else if this.completion_tasks.is_empty() { + // If there are no more completion tasks and the last menu was + // empty, we should hide it. If it was already hidden, we should + // also show the copilot suggestion when available. + if this.hide_context_menu(cx).is_none() { this.update_visible_copilot_suggestion(cx); } - }); - } + } + }); + Ok::<_, anyhow::Error>(()) } .log_err()