From 1444cd9839dcd04f60bb3ba2284be2183cae567d Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Tue, 19 Aug 2025 10:53:10 -0400 Subject: [PATCH] Fix Windows test failures not being detected in CI (#36446) Bug introduced in #35926 Release Notes: - N/A --- .github/actions/run_tests_windows/action.yml | 1 - crates/acp_thread/src/acp_thread.rs | 14 ++- crates/acp_thread/src/mention.rs | 83 ++++++++--------- crates/agent_ui/src/acp/message_editor.rs | 93 +++++++------------- crates/fs/src/fake_git_repo.rs | 6 +- crates/fs/src/fs.rs | 4 +- 6 files changed, 87 insertions(+), 114 deletions(-) diff --git a/.github/actions/run_tests_windows/action.yml b/.github/actions/run_tests_windows/action.yml index e3e3b7142e..0a550c7d32 100644 --- a/.github/actions/run_tests_windows/action.yml +++ b/.github/actions/run_tests_windows/action.yml @@ -56,7 +56,6 @@ runs: $env:COMPlus_CreateDumpDiagnostics = "1" cargo nextest run --workspace --no-fail-fast - continue-on-error: true - name: Analyze crash dumps if: always() diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 7d70727252..1de8110f07 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -2155,7 +2155,7 @@ mod tests { "} ); }); - assert_eq!(fs.files(), vec![Path::new("/test/file-0")]); + assert_eq!(fs.files(), vec![Path::new(path!("/test/file-0"))]); cx.update(|cx| thread.update(cx, |thread, cx| thread.send(vec!["ipsum".into()], cx))) .await @@ -2185,7 +2185,10 @@ mod tests { }); assert_eq!( fs.files(), - vec![Path::new("/test/file-0"), Path::new("/test/file-1")] + vec![ + Path::new(path!("/test/file-0")), + Path::new(path!("/test/file-1")) + ] ); // Checkpoint isn't stored when there are no changes. @@ -2226,7 +2229,10 @@ mod tests { }); assert_eq!( fs.files(), - vec![Path::new("/test/file-0"), Path::new("/test/file-1")] + vec![ + Path::new(path!("/test/file-0")), + Path::new(path!("/test/file-1")) + ] ); // Rewinding the conversation truncates the history and restores the checkpoint. @@ -2254,7 +2260,7 @@ mod tests { "} ); }); - assert_eq!(fs.files(), vec![Path::new("/test/file-0")]); + assert_eq!(fs.files(), vec![Path::new(path!("/test/file-0"))]); } async fn run_until_first_tool_call( diff --git a/crates/acp_thread/src/mention.rs b/crates/acp_thread/src/mention.rs index 350785ec1e..fcf50b0fd7 100644 --- a/crates/acp_thread/src/mention.rs +++ b/crates/acp_thread/src/mention.rs @@ -52,6 +52,7 @@ impl MentionUri { let path = url.path(); match url.scheme() { "file" => { + let path = url.to_file_path().ok().context("Extracting file path")?; if let Some(fragment) = url.fragment() { let range = fragment .strip_prefix("L") @@ -72,23 +73,17 @@ impl MentionUri { if let Some(name) = single_query_param(&url, "symbol")? { Ok(Self::Symbol { name, - path: path.into(), + path, line_range, }) } else { - Ok(Self::Selection { - path: path.into(), - line_range, - }) + Ok(Self::Selection { path, line_range }) } } else { - let abs_path = - PathBuf::from(format!("{}{}", url.host_str().unwrap_or(""), path)); - if input.ends_with("/") { - Ok(Self::Directory { abs_path }) + Ok(Self::Directory { abs_path: path }) } else { - Ok(Self::File { abs_path }) + Ok(Self::File { abs_path: path }) } } } @@ -162,27 +157,17 @@ impl MentionUri { pub fn to_uri(&self) -> Url { match self { MentionUri::File { abs_path } => { - let mut url = Url::parse("file:///").unwrap(); - let path = abs_path.to_string_lossy(); - url.set_path(&path); - url + Url::from_file_path(abs_path).expect("mention path should be absolute") } MentionUri::Directory { abs_path } => { - let mut url = Url::parse("file:///").unwrap(); - let mut path = abs_path.to_string_lossy().to_string(); - if !path.ends_with("/") { - path.push_str("/"); - } - url.set_path(&path); - url + Url::from_directory_path(abs_path).expect("mention path should be absolute") } MentionUri::Symbol { path, name, line_range, } => { - let mut url = Url::parse("file:///").unwrap(); - url.set_path(&path.to_string_lossy()); + let mut url = Url::from_file_path(path).expect("mention path should be absolute"); url.query_pairs_mut().append_pair("symbol", name); url.set_fragment(Some(&format!( "L{}:{}", @@ -192,8 +177,7 @@ impl MentionUri { url } MentionUri::Selection { path, line_range } => { - let mut url = Url::parse("file:///").unwrap(); - url.set_path(&path.to_string_lossy()); + let mut url = Url::from_file_path(path).expect("mention path should be absolute"); url.set_fragment(Some(&format!( "L{}:{}", line_range.start + 1, @@ -266,15 +250,17 @@ pub fn selection_name(path: &Path, line_range: &Range) -> String { #[cfg(test)] mod tests { + use util::{path, uri}; + use super::*; #[test] fn test_parse_file_uri() { - let file_uri = "file:///path/to/file.rs"; + let file_uri = uri!("file:///path/to/file.rs"); let parsed = MentionUri::parse(file_uri).unwrap(); match &parsed { MentionUri::File { abs_path } => { - assert_eq!(abs_path.to_str().unwrap(), "/path/to/file.rs"); + assert_eq!(abs_path.to_str().unwrap(), path!("/path/to/file.rs")); } _ => panic!("Expected File variant"), } @@ -283,11 +269,11 @@ mod tests { #[test] fn test_parse_directory_uri() { - let file_uri = "file:///path/to/dir/"; + let file_uri = uri!("file:///path/to/dir/"); let parsed = MentionUri::parse(file_uri).unwrap(); match &parsed { MentionUri::Directory { abs_path } => { - assert_eq!(abs_path.to_str().unwrap(), "/path/to/dir/"); + assert_eq!(abs_path.to_str().unwrap(), path!("/path/to/dir/")); } _ => panic!("Expected Directory variant"), } @@ -297,22 +283,24 @@ mod tests { #[test] fn test_to_directory_uri_with_slash() { let uri = MentionUri::Directory { - abs_path: PathBuf::from("/path/to/dir/"), + abs_path: PathBuf::from(path!("/path/to/dir/")), }; - assert_eq!(uri.to_uri().to_string(), "file:///path/to/dir/"); + let expected = uri!("file:///path/to/dir/"); + assert_eq!(uri.to_uri().to_string(), expected); } #[test] fn test_to_directory_uri_without_slash() { let uri = MentionUri::Directory { - abs_path: PathBuf::from("/path/to/dir"), + abs_path: PathBuf::from(path!("/path/to/dir")), }; - assert_eq!(uri.to_uri().to_string(), "file:///path/to/dir/"); + let expected = uri!("file:///path/to/dir/"); + assert_eq!(uri.to_uri().to_string(), expected); } #[test] fn test_parse_symbol_uri() { - let symbol_uri = "file:///path/to/file.rs?symbol=MySymbol#L10:20"; + let symbol_uri = uri!("file:///path/to/file.rs?symbol=MySymbol#L10:20"); let parsed = MentionUri::parse(symbol_uri).unwrap(); match &parsed { MentionUri::Symbol { @@ -320,7 +308,7 @@ mod tests { name, line_range, } => { - assert_eq!(path.to_str().unwrap(), "/path/to/file.rs"); + assert_eq!(path.to_str().unwrap(), path!("/path/to/file.rs")); assert_eq!(name, "MySymbol"); assert_eq!(line_range.start, 9); assert_eq!(line_range.end, 19); @@ -332,11 +320,11 @@ mod tests { #[test] fn test_parse_selection_uri() { - let selection_uri = "file:///path/to/file.rs#L5:15"; + let selection_uri = uri!("file:///path/to/file.rs#L5:15"); let parsed = MentionUri::parse(selection_uri).unwrap(); match &parsed { MentionUri::Selection { path, line_range } => { - assert_eq!(path.to_str().unwrap(), "/path/to/file.rs"); + assert_eq!(path.to_str().unwrap(), path!("/path/to/file.rs")); assert_eq!(line_range.start, 4); assert_eq!(line_range.end, 14); } @@ -418,32 +406,35 @@ mod tests { #[test] fn test_invalid_line_range_format() { // Missing L prefix - assert!(MentionUri::parse("file:///path/to/file.rs#10:20").is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#10:20")).is_err()); // Missing colon separator - assert!(MentionUri::parse("file:///path/to/file.rs#L1020").is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L1020")).is_err()); // Invalid numbers - assert!(MentionUri::parse("file:///path/to/file.rs#L10:abc").is_err()); - assert!(MentionUri::parse("file:///path/to/file.rs#Labc:20").is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L10:abc")).is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#Labc:20")).is_err()); } #[test] fn test_invalid_query_parameters() { // Invalid query parameter name - assert!(MentionUri::parse("file:///path/to/file.rs#L10:20?invalid=test").is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L10:20?invalid=test")).is_err()); // Too many query parameters assert!( - MentionUri::parse("file:///path/to/file.rs#L10:20?symbol=test&another=param").is_err() + MentionUri::parse(uri!( + "file:///path/to/file.rs#L10:20?symbol=test&another=param" + )) + .is_err() ); } #[test] fn test_zero_based_line_numbers() { // Test that 0-based line numbers are rejected (should be 1-based) - assert!(MentionUri::parse("file:///path/to/file.rs#L0:10").is_err()); - assert!(MentionUri::parse("file:///path/to/file.rs#L1:0").is_err()); - assert!(MentionUri::parse("file:///path/to/file.rs#L0:0").is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L0:10")).is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L1:0")).is_err()); + assert!(MentionUri::parse(uri!("file:///path/to/file.rs#L0:0")).is_err()); } } diff --git a/crates/agent_ui/src/acp/message_editor.rs b/crates/agent_ui/src/acp/message_editor.rs index 00368d6087..afb1512e5d 100644 --- a/crates/agent_ui/src/acp/message_editor.rs +++ b/crates/agent_ui/src/acp/message_editor.rs @@ -1640,7 +1640,7 @@ mod tests { use serde_json::json; use text::Point; use ui::{App, Context, IntoElement, Render, SharedString, Window}; - use util::path; + use util::{path, uri}; use workspace::{AppState, Item, Workspace}; use crate::acp::{ @@ -1950,13 +1950,12 @@ mod tests { editor.confirm_completion(&editor::actions::ConfirmCompletion::default(), window, cx); }); + let url_one = uri!("file:///dir/a/one.txt"); editor.update(&mut cx, |editor, cx| { - assert_eq!(editor.text(cx), "Lorem [@one.txt](file:///dir/a/one.txt) "); + let text = editor.text(cx); + assert_eq!(text, format!("Lorem [@one.txt]({url_one}) ")); assert!(!editor.has_visible_completions_menu()); - assert_eq!( - fold_ranges(editor, cx), - vec![Point::new(0, 6)..Point::new(0, 39)] - ); + assert_eq!(fold_ranges(editor, cx).len(), 1); }); let contents = message_editor @@ -1977,47 +1976,35 @@ mod tests { contents, [Mention::Text { content: "1".into(), - uri: "file:///dir/a/one.txt".parse().unwrap() + uri: url_one.parse().unwrap() }] ); cx.simulate_input(" "); editor.update(&mut cx, |editor, cx| { - assert_eq!(editor.text(cx), "Lorem [@one.txt](file:///dir/a/one.txt) "); + let text = editor.text(cx); + assert_eq!(text, format!("Lorem [@one.txt]({url_one}) ")); assert!(!editor.has_visible_completions_menu()); - assert_eq!( - fold_ranges(editor, cx), - vec![Point::new(0, 6)..Point::new(0, 39)] - ); + assert_eq!(fold_ranges(editor, cx).len(), 1); }); cx.simulate_input("Ipsum "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - "Lorem [@one.txt](file:///dir/a/one.txt) Ipsum ", - ); + let text = editor.text(cx); + assert_eq!(text, format!("Lorem [@one.txt]({url_one}) Ipsum "),); assert!(!editor.has_visible_completions_menu()); - assert_eq!( - fold_ranges(editor, cx), - vec![Point::new(0, 6)..Point::new(0, 39)] - ); + assert_eq!(fold_ranges(editor, cx).len(), 1); }); cx.simulate_input("@file "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - "Lorem [@one.txt](file:///dir/a/one.txt) Ipsum @file ", - ); + let text = editor.text(cx); + assert_eq!(text, format!("Lorem [@one.txt]({url_one}) Ipsum @file "),); assert!(editor.has_visible_completions_menu()); - assert_eq!( - fold_ranges(editor, cx), - vec![Point::new(0, 6)..Point::new(0, 39)] - ); + assert_eq!(fold_ranges(editor, cx).len(), 1); }); editor.update_in(&mut cx, |editor, window, cx| { @@ -2041,28 +2028,23 @@ mod tests { .collect::>(); assert_eq!(contents.len(), 2); + let url_eight = uri!("file:///dir/b/eight.txt"); pretty_assertions::assert_eq!( contents[1], Mention::Text { content: "8".to_string(), - uri: "file:///dir/b/eight.txt".parse().unwrap(), + uri: url_eight.parse().unwrap(), } ); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - "Lorem [@one.txt](file:///dir/a/one.txt) Ipsum [@eight.txt](file:///dir/b/eight.txt) " - ); - assert!(!editor.has_visible_completions_menu()); - assert_eq!( - fold_ranges(editor, cx), - vec![ - Point::new(0, 6)..Point::new(0, 39), - Point::new(0, 47)..Point::new(0, 84) - ] - ); - }); + assert_eq!( + editor.text(cx), + format!("Lorem [@one.txt]({url_one}) Ipsum [@eight.txt]({url_eight}) ") + ); + assert!(!editor.has_visible_completions_menu()); + assert_eq!(fold_ranges(editor, cx).len(), 2); + }); let plain_text_language = Arc::new(language::Language::new( language::LanguageConfig { @@ -2108,7 +2090,7 @@ mod tests { let fake_language_server = fake_language_servers.next().await.unwrap(); fake_language_server.set_request_handler::( - |_, _| async move { + move |_, _| async move { Ok(Some(lsp::WorkspaceSymbolResponse::Flat(vec![ #[allow(deprecated)] lsp::SymbolInformation { @@ -2132,18 +2114,13 @@ mod tests { cx.simulate_input("@symbol "); editor.update(&mut cx, |editor, cx| { - assert_eq!( - editor.text(cx), - "Lorem [@one.txt](file:///dir/a/one.txt) Ipsum [@eight.txt](file:///dir/b/eight.txt) @symbol " - ); - assert!(editor.has_visible_completions_menu()); - assert_eq!( - current_completion_labels(editor), - &[ - "MySymbol", - ] - ); - }); + assert_eq!( + editor.text(cx), + format!("Lorem [@one.txt]({url_one}) Ipsum [@eight.txt]({url_eight}) @symbol ") + ); + assert!(editor.has_visible_completions_menu()); + assert_eq!(current_completion_labels(editor), &["MySymbol"]); + }); editor.update_in(&mut cx, |editor, window, cx| { editor.confirm_completion(&editor::actions::ConfirmCompletion::default(), window, cx); @@ -2165,9 +2142,7 @@ mod tests { contents[2], Mention::Text { content: "1".into(), - uri: "file:///dir/a/one.txt?symbol=MySymbol#L1:1" - .parse() - .unwrap(), + uri: format!("{url_one}?symbol=MySymbol#L1:1").parse().unwrap(), } ); @@ -2176,7 +2151,7 @@ mod tests { editor.read_with(&cx, |editor, cx| { assert_eq!( editor.text(cx), - "Lorem [@one.txt](file:///dir/a/one.txt) Ipsum [@eight.txt](file:///dir/b/eight.txt) [@MySymbol](file:///dir/a/one.txt?symbol=MySymbol#L1:1) " + format!("Lorem [@one.txt]({url_one}) Ipsum [@eight.txt]({url_eight}) [@MySymbol]({url_one}?symbol=MySymbol#L1:1) ") ); }); } diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index f0936d400a..5b093ac6a0 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -590,9 +590,9 @@ mod tests { assert_eq!( fs.files_with_contents(Path::new("")), [ - (Path::new("/bar/baz").into(), b"qux".into()), - (Path::new("/foo/a").into(), b"lorem".into()), - (Path::new("/foo/b").into(), b"ipsum".into()) + (Path::new(path!("/bar/baz")).into(), b"qux".into()), + (Path::new(path!("/foo/a")).into(), b"lorem".into()), + (Path::new(path!("/foo/b")).into(), b"ipsum".into()) ] ); } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 847e98d6c4..399c0f3e32 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -1101,7 +1101,9 @@ impl FakeFsState { ) -> Option<(&mut FakeFsEntry, PathBuf)> { let canonical_path = self.canonicalize(target, follow_symlink)?; - let mut components = canonical_path.components(); + let mut components = canonical_path + .components() + .skip_while(|component| matches!(component, Component::Prefix(_))); let Some(Component::RootDir) = components.next() else { panic!( "the path {:?} was not canonicalized properly {:?}",