From 1f4741cd5f5866454ae663f1c10b447d38d0bdfb Mon Sep 17 00:00:00 2001 From: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com> Date: Thu, 22 Jan 2026 10:22:04 +0100 Subject: [PATCH] worktree: Use public-api-only tests (#47152) - **settings: Do not leak type serialization into downstream crates** - **worktree: Separate integration tests into separate target** Release Notes: - N/A --- Cargo.lock | 1 + crates/settings/src/settings_store.rs | 15 ++- crates/worktree/Cargo.toml | 10 +- crates/worktree/src/worktree.rs | 47 ++++--- crates/worktree/src/worktree_settings.rs | 123 ------------------ .../integration/main.rs} | 28 ++-- .../tests/integration/worktree_settings.rs | 121 +++++++++++++++++ 7 files changed, 177 insertions(+), 168 deletions(-) rename crates/worktree/{src/worktree_tests.rs => tests/integration/main.rs} (99%) create mode 100644 crates/worktree/tests/integration/worktree_settings.rs diff --git a/Cargo.lock b/Cargo.lock index 1aa884a43d358b3b9d485ea5f745d72fa9fa034f..aace3b782bcc2cbf04a102dbb1fa64155df5abd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -20437,6 +20437,7 @@ dependencies = [ "text", "tracing", "util", + "worktree", "zlog", "ztracing", ] diff --git a/crates/settings/src/settings_store.rs b/crates/settings/src/settings_store.rs index 6c715177862b921fd6f1d2bced83d09332128c62..fbf1feff4de49916239ecb380e7451cd38de494c 100644 --- a/crates/settings/src/settings_store.rs +++ b/crates/settings/src/settings_store.rs @@ -437,12 +437,15 @@ impl SettingsStore { ) { let mut content = self.user_settings.clone().unwrap_or_default().content; update(&mut content); - let new_text = serde_json::to_string(&UserSettingsContent { - content, - ..Default::default() - }) - .unwrap(); - _ = self.set_user_settings(&new_text, cx); + fn trail(this: &mut SettingsStore, content: Box, cx: &mut App) { + let new_text = serde_json::to_string(&UserSettingsContent { + content, + ..Default::default() + }) + .unwrap(); + _ = this.set_user_settings(&new_text, cx); + } + trail(self, content, cx); } pub async fn load_settings(fs: &Arc) -> Result { diff --git a/crates/worktree/Cargo.toml b/crates/worktree/Cargo.toml index 3d902b22af65454c381b2cc56a544b0fc8791e24..788333b5e801f2a0bb22558945d2f142b50ef0a5 100644 --- a/crates/worktree/Cargo.toml +++ b/crates/worktree/Cargo.toml @@ -8,6 +8,12 @@ license = "GPL-3.0-or-later" [lib] path = "src/worktree.rs" doctest = false +test = false + +[[test]] +name = "integration" +required-features = ["test-support"] +path = "tests/integration/main.rs" [lints] workspace = true @@ -17,6 +23,7 @@ test-support = [ "gpui/test-support", "http_client/test-support", "language/test-support", + "pretty_assertions", "settings/test-support", "text/test-support", "util/test-support", @@ -40,6 +47,7 @@ log.workspace = true parking_lot.workspace = true paths.workspace = true postage.workspace = true +pretty_assertions = { workspace = true, optional = true } rpc = { workspace = true, features = ["gpui"] } serde.workspace = true serde_json.workspace = true @@ -59,11 +67,11 @@ git2.workspace = true gpui = { workspace = true, features = ["test-support"] } http_client.workspace = true paths = { workspace = true, features = ["test-support"] } -pretty_assertions.workspace = true rand.workspace = true rpc = { workspace = true, features = ["test-support"] } settings = { workspace = true, features = ["test-support"] } util = { workspace = true, features = ["test-support"] } +worktree = { workspace = true, features = ["test-support"] } zlog.workspace = true [package.metadata.cargo-machete] diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index eefd01f1760d6da50dae84fbff24adfe1ac88bfa..2560beb8099e5d3b9569111edeea449dde5a160b 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -1,7 +1,5 @@ mod ignore; mod worktree_settings; -#[cfg(test)] -mod worktree_tests; use ::ignore::gitignore::{Gitignore, GitignoreBuilder}; use anyhow::{Context as _, Result, anyhow}; @@ -714,7 +712,7 @@ impl Worktree { } } - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] pub fn has_update_observer(&self) -> bool { match self { Worktree::Local(this) => this.update_observer.is_some(), @@ -1416,7 +1414,7 @@ impl LocalWorktree { lowest_ancestor.unwrap_or_else(|| RelPath::empty().into()) } - fn create_entry( + pub fn create_entry( &self, path: Arc, is_dir: bool, @@ -1477,7 +1475,7 @@ impl LocalWorktree { }) } - fn write_file( + pub fn write_file( &self, path: Arc, text: Rope, @@ -1593,7 +1591,7 @@ impl LocalWorktree { }) } - fn delete_entry( + pub fn delete_entry( &self, entry_id: ProjectEntryId, trash: bool, @@ -1816,7 +1814,7 @@ impl LocalWorktree { }) } - fn observe_updates(&mut self, project_id: u64, cx: &Context, callback: F) + pub fn observe_updates(&mut self, project_id: u64, cx: &Context, callback: F) where F: 'static + Send + Fn(proto::UpdateWorktree) -> Fut, Fut: 'static + Send + Future, @@ -1891,6 +1889,13 @@ impl LocalWorktree { self.snapshot.update_abs_path(new_path, root_name); self.restart_background_scanners(cx); } + #[cfg(feature = "test-support")] + pub fn repositories(&self) -> Vec> { + self.git_repositories + .values() + .map(|entry| entry.work_directory_abs_path.clone()) + .collect::>() + } } impl RemoteWorktree { @@ -2279,7 +2284,7 @@ impl Snapshot { } } - fn apply_remote_update( + pub fn apply_remote_update( &mut self, update: proto::UpdateWorktree, always_included_paths: &PathMatcher, @@ -2680,14 +2685,14 @@ impl LocalSnapshot { ignore_stack } - #[cfg(test)] - fn expanded_entries(&self) -> impl Iterator { + #[cfg(feature = "test-support")] + pub fn expanded_entries(&self) -> impl Iterator { self.entries_by_path .cursor::<()>(()) .filter(|entry| entry.kind == EntryKind::Dir && (entry.is_external || entry.is_ignored)) } - #[cfg(test)] + #[cfg(feature = "test-support")] pub fn check_invariants(&self, git_state: bool) { use pretty_assertions::assert_eq; @@ -2767,7 +2772,7 @@ impl LocalSnapshot { } } - #[cfg(test)] + #[cfg(feature = "test-support")] pub fn entries_without_ids(&self, include_ignored: bool) -> Vec<(&RelPath, u64, bool)> { let mut paths = Vec::new(); for entry in self.entries_by_path.cursor::<()>(()) { @@ -2875,7 +2880,7 @@ impl BackgroundScannerState { .await; } - #[cfg(test)] + #[cfg(feature = "test-support")] self.snapshot.check_invariants(false); entry @@ -2943,7 +2948,7 @@ impl BackgroundScannerState { self.changed_paths.insert(ix, parent_path.clone()); } - #[cfg(test)] + #[cfg(feature = "test-support")] self.snapshot.check_invariants(false); } @@ -2999,7 +3004,7 @@ impl BackgroundScannerState { .git_repositories .retain(|id, _| removed_ids.binary_search(id).is_err()); - #[cfg(test)] + #[cfg(feature = "test-support")] self.snapshot.check_invariants(false); } @@ -5130,7 +5135,7 @@ impl BackgroundScanner { return futures::future::pending().await; } - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] if self.fs.is_fake() { return self.executor.simulate_random_delay().await; } @@ -5361,13 +5366,13 @@ struct UpdateIgnoreStatusJob { } pub trait WorktreeModelHandle { - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] fn flush_fs_events<'a>( &self, cx: &'a mut gpui::TestAppContext, ) -> futures::future::LocalBoxFuture<'a, ()>; - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] fn flush_fs_events_in_root_git_repository<'a>( &self, cx: &'a mut gpui::TestAppContext, @@ -5381,7 +5386,7 @@ impl WorktreeModelHandle for Entity { // // This function mutates the worktree's directory and waits for those mutations to be picked up, // to ensure that all redundant FS events have already been processed. - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] fn flush_fs_events<'a>( &self, cx: &'a mut gpui::TestAppContext, @@ -5451,7 +5456,7 @@ impl WorktreeModelHandle for Entity { // worktree and thus its FS events might go through a different path. // In order to flush those, we need to create artificial events in the .git folder and wait // for the repository to be reloaded. - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] fn flush_fs_events_in_root_git_repository<'a>( &self, cx: &'a mut gpui::TestAppContext, @@ -5874,7 +5879,7 @@ impl ProjectEntryId { } } -#[cfg(any(test, feature = "test-support"))] +#[cfg(feature = "test-support")] impl CreatedEntry { pub fn into_included(self) -> Option { match self { diff --git a/crates/worktree/src/worktree_settings.rs b/crates/worktree/src/worktree_settings.rs index 05e17e0c8f270fe53d5c7534b3b74f1e8a2149f5..79b482482942b4322fef869e04b4ea18943a7f8a 100644 --- a/crates/worktree/src/worktree_settings.rs +++ b/crates/worktree/src/worktree_settings.rs @@ -106,126 +106,3 @@ fn path_matchers(mut values: Vec, context: &'static str) -> anyhow::Resu PathMatcher::new(values, PathStyle::local()) .with_context(|| format!("Failed to parse globs from {}", context)) } - -#[cfg(test)] -mod tests { - use super::*; - use std::path::Path; - - fn make_settings_with_read_only(patterns: &[&str]) -> WorktreeSettings { - WorktreeSettings { - project_name: None, - prevent_sharing_in_public_channels: false, - file_scan_exclusions: PathMatcher::default(), - file_scan_inclusions: PathMatcher::default(), - parent_dir_scan_inclusions: PathMatcher::default(), - private_files: PathMatcher::default(), - hidden_files: PathMatcher::default(), - read_only_files: PathMatcher::new( - patterns.iter().map(|s| s.to_string()), - PathStyle::local(), - ) - .unwrap(), - } - } - - #[test] - fn test_is_path_read_only_with_glob_patterns() { - let settings = make_settings_with_read_only(&["**/generated/**", "**/*.gen.rs"]); - - let generated_file = - RelPath::new(Path::new("src/generated/schema.rs"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&generated_file), - "Files in generated directory should be read-only" - ); - - let gen_rs_file = RelPath::new(Path::new("src/types.gen.rs"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&gen_rs_file), - "Files with .gen.rs extension should be read-only" - ); - - let regular_file = RelPath::new(Path::new("src/main.rs"), PathStyle::local()).unwrap(); - assert!( - !settings.is_path_read_only(®ular_file), - "Regular files should not be read-only" - ); - - let similar_name = RelPath::new(Path::new("src/generator.rs"), PathStyle::local()).unwrap(); - assert!( - !settings.is_path_read_only(&similar_name), - "Files with 'generator' in name but not in generated dir should not be read-only" - ); - } - - #[test] - fn test_is_path_read_only_with_specific_paths() { - let settings = make_settings_with_read_only(&["vendor/**", "node_modules/**"]); - - let vendor_file = - RelPath::new(Path::new("vendor/lib/package.js"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&vendor_file), - "Files in vendor directory should be read-only" - ); - - let node_modules_file = RelPath::new( - Path::new("node_modules/lodash/index.js"), - PathStyle::local(), - ) - .unwrap(); - assert!( - settings.is_path_read_only(&node_modules_file), - "Files in node_modules should be read-only" - ); - - let src_file = RelPath::new(Path::new("src/app.js"), PathStyle::local()).unwrap(); - assert!( - !settings.is_path_read_only(&src_file), - "Files in src should not be read-only" - ); - } - - #[test] - fn test_is_path_read_only_empty_patterns() { - let settings = make_settings_with_read_only(&[]); - - let any_file = RelPath::new(Path::new("src/main.rs"), PathStyle::local()).unwrap(); - assert!( - !settings.is_path_read_only(&any_file), - "No files should be read-only when patterns are empty" - ); - } - - #[test] - fn test_is_path_read_only_with_extension_pattern() { - let settings = make_settings_with_read_only(&["**/*.lock", "**/*.min.js"]); - - let lock_file = RelPath::new(Path::new("Cargo.lock"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&lock_file), - "Lock files should be read-only" - ); - - let nested_lock = - RelPath::new(Path::new("packages/app/yarn.lock"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&nested_lock), - "Nested lock files should be read-only" - ); - - let minified_js = - RelPath::new(Path::new("dist/bundle.min.js"), PathStyle::local()).unwrap(); - assert!( - settings.is_path_read_only(&minified_js), - "Minified JS files should be read-only" - ); - - let regular_js = RelPath::new(Path::new("src/app.js"), PathStyle::local()).unwrap(); - assert!( - !settings.is_path_read_only(®ular_js), - "Regular JS files should not be read-only" - ); - } -} diff --git a/crates/worktree/src/worktree_tests.rs b/crates/worktree/tests/integration/main.rs similarity index 99% rename from crates/worktree/src/worktree_tests.rs rename to crates/worktree/tests/integration/main.rs index e8c7180b47d890097004682668ee3c2c235fc01f..3782e36272648cfed0af35dc92638d04ab56d204 100644 --- a/crates/worktree/src/worktree_tests.rs +++ b/crates/worktree/tests/integration/main.rs @@ -1,4 +1,5 @@ -use crate::{Entry, EntryKind, Event, PathChange, Worktree, WorktreeModelHandle}; +mod worktree_settings; + use anyhow::Result; use encoding_rs; use fs::{FakeFs, Fs, RealFs, RemoveOptions}; @@ -8,6 +9,7 @@ use parking_lot::Mutex; use postage::stream::Stream; use pretty_assertions::assert_eq; use rand::prelude::*; +use worktree::{Entry, EntryKind, Event, PathChange, Worktree, WorktreeModelHandle}; use serde_json::json; use settings::SettingsStore; @@ -2022,7 +2024,7 @@ fn randomly_mutate_worktree( match rng.random_range(0_u32..100) { 0..=33 if entry.path.as_ref() != RelPath::empty() => { - log::info!("deleting entry {:?} ({})", entry.path, entry.id.0); + log::info!("deleting entry {:?} ({})", entry.path, entry.id.to_usize()); worktree.delete_entry(entry.id, false, cx).unwrap() } _ => { @@ -2040,7 +2042,11 @@ fn randomly_mutate_worktree( Ok(()) }) } else { - log::info!("overwriting file {:?} ({})", &entry.path, entry.id.0); + log::info!( + "overwriting file {:?} ({})", + &entry.path, + entry.id.to_usize() + ); let task = worktree.write_file( entry.path.clone(), "".into(), @@ -2282,13 +2288,7 @@ async fn test_repository_above_root(executor: BackgroundExecutor, cx: &mut TestA .await; cx.run_until_parked(); let repos = worktree.update(cx, |worktree, _| { - worktree - .as_local() - .unwrap() - .git_repositories - .values() - .map(|entry| entry.work_directory_abs_path.clone()) - .collect::>() + worktree.as_local().unwrap().repositories() }); pretty_assertions::assert_eq!(repos, [Path::new(path!("/root")).into()]); @@ -2301,13 +2301,7 @@ async fn test_repository_above_root(executor: BackgroundExecutor, cx: &mut TestA cx.run_until_parked(); let repos = worktree.update(cx, |worktree, _| { - worktree - .as_local() - .unwrap() - .git_repositories - .values() - .map(|entry| entry.work_directory_abs_path.clone()) - .collect::>() + worktree.as_local().unwrap().repositories() }); pretty_assertions::assert_eq!(repos, [Path::new(path!("/root")).into()]); } diff --git a/crates/worktree/tests/integration/worktree_settings.rs b/crates/worktree/tests/integration/worktree_settings.rs new file mode 100644 index 0000000000000000000000000000000000000000..213d888e9f99980d28be62717ff3d2885ebdfe57 --- /dev/null +++ b/crates/worktree/tests/integration/worktree_settings.rs @@ -0,0 +1,121 @@ +use std::path::Path; +use util::{ + paths::{PathMatcher, PathStyle}, + rel_path::RelPath, +}; +use worktree::*; + +fn make_settings_with_read_only(patterns: &[&str]) -> WorktreeSettings { + WorktreeSettings { + project_name: None, + prevent_sharing_in_public_channels: false, + file_scan_exclusions: PathMatcher::default(), + file_scan_inclusions: PathMatcher::default(), + parent_dir_scan_inclusions: PathMatcher::default(), + private_files: PathMatcher::default(), + hidden_files: PathMatcher::default(), + read_only_files: PathMatcher::new( + patterns.iter().map(|s| s.to_string()), + PathStyle::local(), + ) + .unwrap(), + } +} + +#[test] +fn test_is_path_read_only_with_glob_patterns() { + let settings = make_settings_with_read_only(&["**/generated/**", "**/*.gen.rs"]); + + let generated_file = + RelPath::new(Path::new("src/generated/schema.rs"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&generated_file), + "Files in generated directory should be read-only" + ); + + let gen_rs_file = RelPath::new(Path::new("src/types.gen.rs"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&gen_rs_file), + "Files with .gen.rs extension should be read-only" + ); + + let regular_file = RelPath::new(Path::new("src/main.rs"), PathStyle::local()).unwrap(); + assert!( + !settings.is_path_read_only(®ular_file), + "Regular files should not be read-only" + ); + + let similar_name = RelPath::new(Path::new("src/generator.rs"), PathStyle::local()).unwrap(); + assert!( + !settings.is_path_read_only(&similar_name), + "Files with 'generator' in name but not in generated dir should not be read-only" + ); +} + +#[test] +fn test_is_path_read_only_with_specific_paths() { + let settings = make_settings_with_read_only(&["vendor/**", "node_modules/**"]); + + let vendor_file = RelPath::new(Path::new("vendor/lib/package.js"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&vendor_file), + "Files in vendor directory should be read-only" + ); + + let node_modules_file = RelPath::new( + Path::new("node_modules/lodash/index.js"), + PathStyle::local(), + ) + .unwrap(); + assert!( + settings.is_path_read_only(&node_modules_file), + "Files in node_modules should be read-only" + ); + + let src_file = RelPath::new(Path::new("src/app.js"), PathStyle::local()).unwrap(); + assert!( + !settings.is_path_read_only(&src_file), + "Files in src should not be read-only" + ); +} + +#[test] +fn test_is_path_read_only_empty_patterns() { + let settings = make_settings_with_read_only(&[]); + + let any_file = RelPath::new(Path::new("src/main.rs"), PathStyle::local()).unwrap(); + assert!( + !settings.is_path_read_only(&any_file), + "No files should be read-only when patterns are empty" + ); +} + +#[test] +fn test_is_path_read_only_with_extension_pattern() { + let settings = make_settings_with_read_only(&["**/*.lock", "**/*.min.js"]); + + let lock_file = RelPath::new(Path::new("Cargo.lock"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&lock_file), + "Lock files should be read-only" + ); + + let nested_lock = + RelPath::new(Path::new("packages/app/yarn.lock"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&nested_lock), + "Nested lock files should be read-only" + ); + + let minified_js = RelPath::new(Path::new("dist/bundle.min.js"), PathStyle::local()).unwrap(); + assert!( + settings.is_path_read_only(&minified_js), + "Minified JS files should be read-only" + ); + + let regular_js = RelPath::new(Path::new("src/app.js"), PathStyle::local()).unwrap(); + assert!( + !settings.is_path_read_only(®ular_js), + "Regular JS files should not be read-only" + ); +}