From 63b625236d2942ccbe881b93e31b4efa52fce763 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Fri, 8 Aug 2025 13:20:38 +0200 Subject: [PATCH] More tests --- Cargo.lock | 1 + crates/agent2/Cargo.toml | 1 + crates/agent2/src/tools/edit_file_tool.rs | 285 +++++++------------ crates/assistant_tools/src/edit_file_tool.rs | 85 ------ 4 files changed, 111 insertions(+), 261 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9ba60f44f2..65827d428b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -179,6 +179,7 @@ dependencies = [ "language_model", "language_models", "log", + "lsp", "paths", "pretty_assertions", "project", diff --git a/crates/agent2/Cargo.toml b/crates/agent2/Cargo.toml index 862f6a96b1..3e19895a31 100644 --- a/crates/agent2/Cargo.toml +++ b/crates/agent2/Cargo.toml @@ -56,6 +56,7 @@ gpui = { workspace = true, "features" = ["test-support"] } gpui_tokio.workspace = true language = { workspace = true, "features" = ["test-support"] } language_model = { workspace = true, "features" = ["test-support"] } +lsp = { workspace = true, "features" = ["test-support"] } project = { workspace = true, "features" = ["test-support"] } reqwest_client.workspace = true settings = { workspace = true, "features" = ["test-support"] } diff --git a/crates/agent2/src/tools/edit_file_tool.rs b/crates/agent2/src/tools/edit_file_tool.rs index 4c4365519d..8c96c248d5 100644 --- a/crates/agent2/src/tools/edit_file_tool.rs +++ b/crates/agent2/src/tools/edit_file_tool.rs @@ -195,7 +195,7 @@ impl AgentTool for EditFileTool { let project = self.thread.read(cx).project().clone(); let project_path = match resolve_path(&input, project.clone(), cx) { Ok(path) => path, - Err(err) => return Task::ready(Err(anyhow!(err))).into(), + Err(err) => return Task::ready(Err(anyhow!(err))), }; let request = self.thread.update(cx, |thread, cx| { @@ -473,189 +473,97 @@ mod tests { ); } - // #[gpui::test] - // async fn test_resolve_path_for_creating_file(cx: &mut TestAppContext) { - // let mode = &EditFileMode::Create; + #[gpui::test] + async fn test_resolve_path_for_creating_file(cx: &mut TestAppContext) { + let mode = &EditFileMode::Create; - // let result = test_resolve_path(mode, "root/new.txt", cx); - // assert_resolved_path_eq(result.await, "new.txt"); + let result = test_resolve_path(mode, "root/new.txt", cx); + assert_resolved_path_eq(result.await, "new.txt"); - // let result = test_resolve_path(mode, "new.txt", cx); - // assert_resolved_path_eq(result.await, "new.txt"); + let result = test_resolve_path(mode, "new.txt", cx); + assert_resolved_path_eq(result.await, "new.txt"); - // let result = test_resolve_path(mode, "dir/new.txt", cx); - // assert_resolved_path_eq(result.await, "dir/new.txt"); + let result = test_resolve_path(mode, "dir/new.txt", cx); + assert_resolved_path_eq(result.await, "dir/new.txt"); - // let result = test_resolve_path(mode, "root/dir/subdir/existing.txt", cx); - // assert_eq!( - // result.await.unwrap_err().to_string(), - // "Can't create file: file already exists" - // ); + let result = test_resolve_path(mode, "root/dir/subdir/existing.txt", cx); + assert_eq!( + result.await.unwrap_err().to_string(), + "Can't create file: file already exists" + ); - // let result = test_resolve_path(mode, "root/dir/nonexistent_dir/new.txt", cx); - // assert_eq!( - // result.await.unwrap_err().to_string(), - // "Can't create file: parent directory doesn't exist" - // ); - // } - - // #[gpui::test] - // async fn test_resolve_path_for_editing_file(cx: &mut TestAppContext) { - // let mode = &EditFileMode::Edit; - - // let path_with_root = "root/dir/subdir/existing.txt"; - // let path_without_root = "dir/subdir/existing.txt"; - // let result = test_resolve_path(mode, path_with_root, cx); - // assert_resolved_path_eq(result.await, path_without_root); - - // let result = test_resolve_path(mode, path_without_root, cx); - // assert_resolved_path_eq(result.await, path_without_root); - - // let result = test_resolve_path(mode, "root/nonexistent.txt", cx); - // assert_eq!( - // result.await.unwrap_err().to_string(), - // "Can't edit file: path not found" - // ); - - // let result = test_resolve_path(mode, "root/dir", cx); - // assert_eq!( - // result.await.unwrap_err().to_string(), - // "Can't edit file: path is a directory" - // ); - // } - - // async fn test_resolve_path( - // mode: &EditFileMode, - // path: &str, - // cx: &mut TestAppContext, - // ) -> anyhow::Result { - // init_test(cx); - - // let fs = project::FakeFs::new(cx.executor()); - // fs.insert_tree( - // "/root", - // json!({ - // "dir": { - // "subdir": { - // "existing.txt": "hello" - // } - // } - // }), - // ) - // .await; - // let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; - - // let input = EditFileToolInput { - // display_description: "Some edit".into(), - // path: path.into(), - // mode: mode.clone(), - // }; - - // let result = cx.update(|cx| resolve_path(&input, project, cx)); - // result - // } - - // fn assert_resolved_path_eq(path: anyhow::Result, expected: &str) { - // let actual = path - // .expect("Should return valid path") - // .path - // .to_str() - // .unwrap() - // .replace("\\", "/"); // Naive Windows paths normalization - // assert_eq!(actual, expected); - // } - - // #[test] - // fn still_streaming_ui_text_with_path() { - // let input = json!({ - // "path": "src/main.rs", - // "display_description": "", - // "old_string": "old code", - // "new_string": "new code" - // }); - - // assert_eq!(EditFileTool.still_streaming_ui_text(&input), "src/main.rs"); - // } - - // #[test] - // fn still_streaming_ui_text_with_description() { - // let input = json!({ - // "path": "", - // "display_description": "Fix error handling", - // "old_string": "old code", - // "new_string": "new code" - // }); - - // assert_eq!( - // EditFileTool.still_streaming_ui_text(&input), - // "Fix error handling", - // ); - // } - - // #[test] - // fn still_streaming_ui_text_with_path_and_description() { - // let input = json!({ - // "path": "src/main.rs", - // "display_description": "Fix error handling", - // "old_string": "old code", - // "new_string": "new code" - // }); - - // assert_eq!( - // EditFileTool.still_streaming_ui_text(&input), - // "Fix error handling", - // ); - // } - - // #[test] - // fn still_streaming_ui_text_no_path_or_description() { - // let input = json!({ - // "path": "", - // "display_description": "", - // "old_string": "old code", - // "new_string": "new code" - // }); - - // assert_eq!( - // EditFileTool.still_streaming_ui_text(&input), - // DEFAULT_UI_TEXT, - // ); - // } - - // #[test] - // fn still_streaming_ui_text_with_null() { - // let input = serde_json::Value::Null; - - // assert_eq!( - // EditFileTool.still_streaming_ui_text(&input), - // DEFAULT_UI_TEXT, - // ); - // } - - fn init_test(cx: &mut TestAppContext) { - cx.update(|cx| { - let settings_store = SettingsStore::test(cx); - cx.set_global(settings_store); - language::init(cx); - TelemetrySettings::register(cx); - agent_settings::AgentSettings::register(cx); - Project::init_settings(cx); - }); + let result = test_resolve_path(mode, "root/dir/nonexistent_dir/new.txt", cx); + assert_eq!( + result.await.unwrap_err().to_string(), + "Can't create file: parent directory doesn't exist" + ); } - // fn init_test_with_config(cx: &mut TestAppContext, data_dir: &Path) { - // cx.update(|cx| { - // // Set custom data directory (config will be under data_dir/config) - // paths::set_custom_data_dir(data_dir.to_str().unwrap()); + #[gpui::test] + async fn test_resolve_path_for_editing_file(cx: &mut TestAppContext) { + let mode = &EditFileMode::Edit; - // let settings_store = SettingsStore::test(cx); - // cx.set_global(settings_store); - // language::init(cx); - // TelemetrySettings::register(cx); - // agent_settings::AgentSettings::register(cx); - // Project::init_settings(cx); - // }); - // } + let path_with_root = "root/dir/subdir/existing.txt"; + let path_without_root = "dir/subdir/existing.txt"; + let result = test_resolve_path(mode, path_with_root, cx); + assert_resolved_path_eq(result.await, path_without_root); + + let result = test_resolve_path(mode, path_without_root, cx); + assert_resolved_path_eq(result.await, path_without_root); + + let result = test_resolve_path(mode, "root/nonexistent.txt", cx); + assert_eq!( + result.await.unwrap_err().to_string(), + "Can't edit file: path not found" + ); + + let result = test_resolve_path(mode, "root/dir", cx); + assert_eq!( + result.await.unwrap_err().to_string(), + "Can't edit file: path is a directory" + ); + } + + async fn test_resolve_path( + mode: &EditFileMode, + path: &str, + cx: &mut TestAppContext, + ) -> anyhow::Result { + init_test(cx); + + let fs = project::FakeFs::new(cx.executor()); + fs.insert_tree( + "/root", + json!({ + "dir": { + "subdir": { + "existing.txt": "hello" + } + } + }), + ) + .await; + let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; + + let input = EditFileToolInput { + display_description: "Some edit".into(), + path: path.into(), + mode: mode.clone(), + }; + + let result = cx.update(|cx| resolve_path(&input, project, cx)); + result + } + + fn assert_resolved_path_eq(path: anyhow::Result, expected: &str) { + let actual = path + .expect("Should return valid path") + .path + .to_str() + .unwrap() + .replace("\\", "/"); // Naive Windows paths normalization + assert_eq!(actual, expected); + } // #[gpui::test] // async fn test_format_on_save(cx: &mut TestAppContext) { @@ -1633,4 +1541,29 @@ mod tests { // ); // }); // } + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + language::init(cx); + TelemetrySettings::register(cx); + agent_settings::AgentSettings::register(cx); + Project::init_settings(cx); + }); + } + + fn init_test_with_config(cx: &mut TestAppContext, data_dir: &Path) { + cx.update(|cx| { + // Set custom data directory (config will be under data_dir/config) + paths::set_custom_data_dir(data_dir.to_str().unwrap()); + + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + language::init(cx); + TelemetrySettings::register(cx); + agent_settings::AgentSettings::register(cx); + Project::init_settings(cx); + }); + } } diff --git a/crates/assistant_tools/src/edit_file_tool.rs b/crates/assistant_tools/src/edit_file_tool.rs index dce9f49abd..311521019d 100644 --- a/crates/assistant_tools/src/edit_file_tool.rs +++ b/crates/assistant_tools/src/edit_file_tool.rs @@ -120,8 +120,6 @@ struct PartialInput { display_description: String, } -const DEFAULT_UI_TEXT: &str = "Editing file"; - impl Tool for EditFileTool { fn name(&self) -> String { "edit_file".into() @@ -211,22 +209,6 @@ impl Tool for EditFileTool { } } - fn still_streaming_ui_text(&self, input: &serde_json::Value) -> String { - if let Some(input) = serde_json::from_value::(input.clone()).ok() { - let description = input.display_description.trim(); - if !description.is_empty() { - return description.to_string(); - } - - let path = input.path.trim(); - if !path.is_empty() { - return path.to_string(); - } - } - - DEFAULT_UI_TEXT.to_string() - } - fn run( self: Arc, input: serde_json::Value, @@ -1370,73 +1352,6 @@ mod tests { assert_eq!(actual, expected); } - #[test] - fn still_streaming_ui_text_with_path() { - let input = json!({ - "path": "src/main.rs", - "display_description": "", - "old_string": "old code", - "new_string": "new code" - }); - - assert_eq!(EditFileTool.still_streaming_ui_text(&input), "src/main.rs"); - } - - #[test] - fn still_streaming_ui_text_with_description() { - let input = json!({ - "path": "", - "display_description": "Fix error handling", - "old_string": "old code", - "new_string": "new code" - }); - - assert_eq!( - EditFileTool.still_streaming_ui_text(&input), - "Fix error handling", - ); - } - - #[test] - fn still_streaming_ui_text_with_path_and_description() { - let input = json!({ - "path": "src/main.rs", - "display_description": "Fix error handling", - "old_string": "old code", - "new_string": "new code" - }); - - assert_eq!( - EditFileTool.still_streaming_ui_text(&input), - "Fix error handling", - ); - } - - #[test] - fn still_streaming_ui_text_no_path_or_description() { - let input = json!({ - "path": "", - "display_description": "", - "old_string": "old code", - "new_string": "new code" - }); - - assert_eq!( - EditFileTool.still_streaming_ui_text(&input), - DEFAULT_UI_TEXT, - ); - } - - #[test] - fn still_streaming_ui_text_with_null() { - let input = serde_json::Value::Null; - - assert_eq!( - EditFileTool.still_streaming_ui_text(&input), - DEFAULT_UI_TEXT, - ); - } - fn init_test(cx: &mut TestAppContext) { cx.update(|cx| { let settings_store = SettingsStore::test(cx);