From 71128d2ee6b55824c393f2b2b97f2504a86963c0 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 2 Aug 2022 16:52:37 +0200 Subject: [PATCH 1/3] Compute diffs based on characters rather than lines Previously, a change on a given line would cause that whole line to be replaced. In turn, this caused anchors on that line to go to the start of that line because they would lie inside of a deleted region after applying the diff. By switching to a character-wise diff, we perform smaller edits to the buffer which stabilizes anchor positions. --- crates/language/src/buffer.rs | 2 +- crates/language/src/tests.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index e6b0d48820b0db06de9cf96c4c887dbaff561607..353f3182c0b591cd9abea960ab4c534a05056dfc 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -1006,7 +1006,7 @@ impl Buffer { let old_text = old_text.to_string(); let line_ending = LineEnding::detect(&new_text); LineEnding::normalize(&mut new_text); - let changes = TextDiff::from_lines(old_text.as_str(), new_text.as_str()) + let changes = TextDiff::from_chars(old_text.as_str(), new_text.as_str()) .iter_all_changes() .map(|c| (c.tag(), c.value().len())) .collect::>(); diff --git a/crates/language/src/tests.rs b/crates/language/src/tests.rs index 937ff069305cabe280d4d5de5949ea5423181054..572f2b0ba88d31a62b26ab6660a4d6e203e94089 100644 --- a/crates/language/src/tests.rs +++ b/crates/language/src/tests.rs @@ -183,20 +183,23 @@ fn test_edit_events(cx: &mut gpui::MutableAppContext) { async fn test_apply_diff(cx: &mut gpui::TestAppContext) { let text = "a\nbb\nccc\ndddd\neeeee\nffffff\n"; let buffer = cx.add_model(|cx| Buffer::new(0, text, cx)); + let anchor = buffer.read_with(cx, |buffer, _| buffer.anchor_before(Point::new(3, 3))); let text = "a\nccc\ndddd\nffffff\n"; let diff = buffer.read_with(cx, |b, cx| b.diff(text.into(), cx)).await; buffer.update(cx, |buffer, cx| { buffer.apply_diff(diff, cx).unwrap(); + assert_eq!(buffer.text(), text); + assert_eq!(anchor.to_point(&buffer), Point::new(2, 3)); }); - cx.read(|cx| assert_eq!(buffer.read(cx).text(), text)); let text = "a\n1\n\nccc\ndd2dd\nffffff\n"; let diff = buffer.read_with(cx, |b, cx| b.diff(text.into(), cx)).await; buffer.update(cx, |buffer, cx| { buffer.apply_diff(diff, cx).unwrap(); + assert_eq!(buffer.text(), text); + assert_eq!(anchor.to_point(&buffer), Point::new(4, 4)); }); - cx.read(|cx| assert_eq!(buffer.read(cx).text(), text)); } #[gpui::test] From fc141001344237fa9d3f5b6fca2e181a1653e3b4 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 2 Aug 2022 18:48:17 +0200 Subject: [PATCH 2/3] Fix tests --- crates/project/src/project_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 4c5e9ef8e1b95994dbe9550b745ef6fb091d79b4..3dc4e9359f7dbb87ae5455a1a4ae5f6911ddcefd 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -2498,7 +2498,7 @@ async fn test_buffer_file_changes_on_disk(cx: &mut gpui::TestAppContext) { .collect::>(); assert_eq!( anchor_positions, - [Point::new(1, 1), Point::new(3, 1), Point::new(4, 0)] + [Point::new(1, 1), Point::new(3, 1), Point::new(3, 5)] ); }); From 42db566ff6add0c303bb18f816346d4aea675f63 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 2 Aug 2022 19:09:34 +0200 Subject: [PATCH 3/3] Remove terminal integration test Creating a full-fledged terminal is flaky and causes tests to either hang or outright panic. The only test that requires creating a terminal was `test_terminal` but we think the value added by that test is not worth the flakiness, so we're removing it. Co-Authored-By: Mikayla Maki --- crates/terminal/src/terminal.rs | 22 ++---- .../src/tests/terminal_test_context.rs | 75 +------------------ 2 files changed, 10 insertions(+), 87 deletions(-) diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index 028a8f23b03fb0c50ad2851a7bc14067f6396a71..40e53070eef9fdf77d862e6a3ab691e18af79508 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -389,23 +389,13 @@ mod tests { mod terminal_test_context; - ///Basic integration test, can we get the terminal to show up, execute a command, - //and produce noticable output? - #[gpui::test(retries = 5)] - async fn test_terminal(cx: &mut TestAppContext) { - let mut cx = TerminalTestContext::new(cx, true); - - cx.execute_and_wait("expr 3 + 4", |content, _cx| content.contains("7")) - .await; - } - ///Working directory calculation tests ///No Worktrees in project -> home_dir() #[gpui::test] async fn no_worktree(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; //Test cx.cx.read(|cx| { @@ -428,7 +418,7 @@ mod tests { async fn no_active_entry_worktree_is_file(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; cx.create_file_wt(project.clone(), "/root.txt").await; @@ -451,7 +441,7 @@ mod tests { #[gpui::test] async fn no_active_entry_worktree_is_dir(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; let (_wt, _entry) = cx.create_folder_wt(project.clone(), "/root/").await; @@ -474,7 +464,7 @@ mod tests { #[gpui::test] async fn active_entry_worktree_is_file(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; let (_wt, _entry) = cx.create_folder_wt(project.clone(), "/root1/").await; let (wt2, entry2) = cx.create_file_wt(project.clone(), "/root2.txt").await; @@ -498,7 +488,7 @@ mod tests { #[gpui::test] async fn active_entry_worktree_is_dir(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; let (_wt, _entry) = cx.create_folder_wt(project.clone(), "/root1/").await; let (wt2, entry2) = cx.create_folder_wt(project.clone(), "/root2/").await; @@ -522,7 +512,7 @@ mod tests { #[gpui::test] async fn active_entry_worktree_is_file_int(cx: &mut TestAppContext) { //Setup variables - let mut cx = TerminalTestContext::new(cx, true); + let mut cx = TerminalTestContext::new(cx); let (project, workspace) = cx.blank_workspace().await; let (_wt, _entry) = cx.create_folder_wt(project.clone(), "/root1/").await; let (wt2, entry2) = cx.create_file_wt(project.clone(), "/root2.txt").await; diff --git a/crates/terminal/src/tests/terminal_test_context.rs b/crates/terminal/src/tests/terminal_test_context.rs index e78939224b3c5c8011c121cf9f2a529830e770fa..d1f6422a2dd099b0328acc4707c6bd83b11b40a7 100644 --- a/crates/terminal/src/tests/terminal_test_context.rs +++ b/crates/terminal/src/tests/terminal_test_context.rs @@ -1,72 +1,16 @@ -use std::{path::Path, time::Duration}; - -use gpui::{ - geometry::vector::vec2f, AppContext, ModelHandle, ReadModelWith, TestAppContext, ViewHandle, -}; -use itertools::Itertools; +use gpui::{ModelHandle, TestAppContext, ViewHandle}; use project::{Entry, Project, ProjectPath, Worktree}; +use std::{path::Path, time::Duration}; use workspace::{AppState, Workspace}; -use crate::{ - connected_el::TermDimensions, - model::{Terminal, TerminalBuilder}, - DEBUG_CELL_WIDTH, DEBUG_LINE_HEIGHT, DEBUG_TERMINAL_HEIGHT, DEBUG_TERMINAL_WIDTH, -}; - pub struct TerminalTestContext<'a> { pub cx: &'a mut TestAppContext, - pub connection: Option>, } impl<'a> TerminalTestContext<'a> { - pub fn new(cx: &'a mut TestAppContext, term: bool) -> Self { + pub fn new(cx: &'a mut TestAppContext) -> Self { cx.set_condition_duration(Some(Duration::from_secs(5))); - - let size_info = TermDimensions::new( - DEBUG_CELL_WIDTH, - DEBUG_LINE_HEIGHT, - vec2f(DEBUG_TERMINAL_WIDTH, DEBUG_TERMINAL_HEIGHT), - ); - - let connection = term.then(|| { - cx.add_model(|cx| { - TerminalBuilder::new(None, None, None, size_info) - .unwrap() - .subscribe(cx) - }) - }); - - TerminalTestContext { cx, connection } - } - - pub async fn execute_and_wait(&mut self, command: &str, f: F) -> String - where - F: Fn(String, &AppContext) -> bool, - { - let connection = self.connection.take().unwrap(); - - let command = command.to_string(); - connection.update(self.cx, |connection, _| { - connection.write_to_pty(command); - connection.write_to_pty("\r".to_string()); - }); - - connection - .condition(self.cx, |conn, cx| { - let content = Self::grid_as_str(conn); - f(content, cx) - }) - .await; - - let res = self - .cx - .read_model_with(&connection, &mut |conn, _: &AppContext| { - Self::grid_as_str(conn) - }); - - self.connection = Some(connection); - - res + TerminalTestContext { cx } } ///Creates a worktree with 1 file: /root.txt @@ -139,17 +83,6 @@ impl<'a> TerminalTestContext<'a> { project.update(cx, |project, cx| project.set_active_path(Some(p), cx)); }); } - - fn grid_as_str(connection: &Terminal) -> String { - connection.render_lock(None, |content, _| { - let lines = content.display_iter.group_by(|i| i.point.line.0); - lines - .into_iter() - .map(|(_, line)| line.map(|i| i.c).collect::()) - .collect::>() - .join("\n") - }) - } } impl<'a> Drop for TerminalTestContext<'a> {