From 2e516261fe26061782fb2eec255267f4b1d14e80 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 28 Feb 2024 11:04:01 +0200 Subject: [PATCH] Add tests on inventory task sorting --- crates/project/src/task_inventory.rs | 257 ++++++++++++++++++++-- crates/project_panel/src/project_panel.rs | 8 +- crates/task/src/lib.rs | 4 +- crates/task/src/oneshot_source.rs | 10 +- crates/task/src/static_source.rs | 10 +- crates/util/src/util.rs | 10 +- 6 files changed, 267 insertions(+), 32 deletions(-) diff --git a/crates/project/src/task_inventory.rs b/crates/project/src/task_inventory.rs index f21bad88f0..a4ccd010fb 100644 --- a/crates/project/src/task_inventory.rs +++ b/crates/project/src/task_inventory.rs @@ -5,8 +5,8 @@ use std::{any::TypeId, path::Path, sync::Arc}; use collections::{HashMap, VecDeque}; use gpui::{AppContext, Context, Model, ModelContext, Subscription}; use itertools::Itertools; -use task::{Source, Task, TaskId}; -use util::post_inc; +use task::{Task, TaskId, TaskSource}; +use util::{post_inc, NumericPrefixWithSuffix}; /// Inventory tracks available tasks for a given project. pub struct Inventory { @@ -15,7 +15,7 @@ pub struct Inventory { } struct SourceInInventory { - source: Model>, + source: Model>, _subscription: Subscription, type_id: TypeId, } @@ -29,7 +29,7 @@ impl Inventory { } /// Registers a new tasks source, that would be fetched for available tasks. - pub fn add_source(&mut self, source: Model>, cx: &mut ModelContext) { + pub fn add_source(&mut self, source: Model>, cx: &mut ModelContext) { let _subscription = cx.observe(&source, |_, _, cx| { cx.notify(); }); @@ -43,7 +43,7 @@ impl Inventory { cx.notify(); } - pub fn source(&self) -> Option>> { + pub fn source(&self) -> Option>> { let target_type_id = std::any::TypeId::of::(); self.sources.iter().find_map( |SourceInInventory { @@ -77,6 +77,8 @@ impl Inventory { } else { HashMap::default() }; + let not_used_score = post_inc(&mut lru_score); + self.sources .iter() .flat_map(|source| { @@ -89,16 +91,20 @@ impl Inventory { tasks_by_usage .get(&task.id()) .copied() - .unwrap_or_else(|| post_inc(&mut lru_score)) + .unwrap_or(not_used_score) } else { - post_inc(&mut lru_score) + not_used_score }; (task, usages) }) .sorted_unstable_by(|(task_a, usages_a), (task_b, usages_b)| { - usages_a - .cmp(usages_b) - .then(task_a.name().cmp(task_b.name())) + usages_a.cmp(usages_b).then({ + NumericPrefixWithSuffix::from_numeric_prefixed_str(task_a.name()) + .cmp(&NumericPrefixWithSuffix::from_numeric_prefixed_str( + task_b.name(), + )) + .then(task_a.name().cmp(task_b.name())) + }) }) .map(|(task, _)| task) .collect() @@ -114,6 +120,7 @@ impl Inventory { }) } + /// Registers task "usage" as being scheduled – to be used for LRU sorting when listing all tasks. pub fn task_scheduled(&mut self, id: TaskId) { self.last_scheduled_tasks.push_back(id); if self.last_scheduled_tasks.len() > 5_000 { @@ -124,10 +131,234 @@ impl Inventory { #[cfg(test)] mod tests { + use std::path::PathBuf; + + use gpui::TestAppContext; + use super::*; - #[test] - fn todo_kb() { - todo!("TODO kb LRU tests") + #[gpui::test] + fn test_task_list_sorting(cx: &mut TestAppContext) { + let inventory = cx.update(Inventory::new); + let initial_tasks = list_task_names(&inventory, None, true, cx); + assert!( + initial_tasks.is_empty(), + "No tasks expected for empty inventory, but got {initial_tasks:?}" + ); + let initial_tasks = list_task_names(&inventory, None, false, cx); + assert!( + initial_tasks.is_empty(), + "No tasks expected for empty inventory, but got {initial_tasks:?}" + ); + + inventory.update(cx, |inventory, cx| { + inventory.add_source(TestSource::new(vec!["3_task".to_string()], cx), cx); + }); + inventory.update(cx, |inventory, cx| { + inventory.add_source( + TestSource::new( + vec![ + "1_task".to_string(), + "2_task".to_string(), + "1_a_task".to_string(), + ], + cx, + ), + cx, + ); + }); + + let expected_initial_state = [ + "1_a_task".to_string(), + "1_task".to_string(), + "2_task".to_string(), + "3_task".to_string(), + ]; + assert_eq!( + list_task_names(&inventory, None, false, cx), + &expected_initial_state, + "Task list without lru sorting, should be sorted alphanumerically" + ); + assert_eq!( + list_task_names(&inventory, None, true, cx), + &expected_initial_state, + "Tasks with equal amount of usages should be sorted alphanumerically" + ); + + register_task_used(&inventory, "2_task", cx); + assert_eq!( + list_task_names(&inventory, None, false, cx), + &expected_initial_state, + "Task list without lru sorting, should be sorted alphanumerically" + ); + assert_eq!( + list_task_names(&inventory, None, true, cx), + vec![ + "2_task".to_string(), + "1_a_task".to_string(), + "1_task".to_string(), + "3_task".to_string() + ], + ); + + register_task_used(&inventory, "1_task", cx); + register_task_used(&inventory, "1_task", cx); + register_task_used(&inventory, "1_task", cx); + register_task_used(&inventory, "3_task", cx); + assert_eq!( + list_task_names(&inventory, None, false, cx), + &expected_initial_state, + "Task list without lru sorting, should be sorted alphanumerically" + ); + assert_eq!( + list_task_names(&inventory, None, true, cx), + vec![ + "3_task".to_string(), + "1_task".to_string(), + "2_task".to_string(), + "1_a_task".to_string(), + ], + ); + + inventory.update(cx, |inventory, cx| { + inventory.add_source( + TestSource::new(vec!["10_hello".to_string(), "11_hello".to_string()], cx), + cx, + ); + }); + let expected_updated_state = [ + "1_a_task".to_string(), + "1_task".to_string(), + "2_task".to_string(), + "3_task".to_string(), + "10_hello".to_string(), + "11_hello".to_string(), + ]; + assert_eq!( + list_task_names(&inventory, None, false, cx), + &expected_updated_state, + "Task list without lru sorting, should be sorted alphanumerically" + ); + assert_eq!( + list_task_names(&inventory, None, true, cx), + vec![ + "3_task".to_string(), + "1_task".to_string(), + "2_task".to_string(), + "1_a_task".to_string(), + "10_hello".to_string(), + "11_hello".to_string(), + ], + ); + + register_task_used(&inventory, "11_hello", cx); + assert_eq!( + list_task_names(&inventory, None, false, cx), + &expected_updated_state, + "Task list without lru sorting, should be sorted alphanumerically" + ); + assert_eq!( + list_task_names(&inventory, None, true, cx), + vec![ + "11_hello".to_string(), + "3_task".to_string(), + "1_task".to_string(), + "2_task".to_string(), + "1_a_task".to_string(), + "10_hello".to_string(), + ], + ); + } + + #[derive(Debug, Clone, PartialEq, Eq)] + struct TestTask { + id: TaskId, + name: String, + } + + impl Task for TestTask { + fn id(&self) -> &TaskId { + &self.id + } + + fn name(&self) -> &str { + &self.name + } + + fn cwd(&self) -> Option<&Path> { + None + } + + fn exec(&self, _cwd: Option) -> Option { + None + } + } + + struct TestSource { + tasks: Vec, + } + + impl TestSource { + fn new( + task_names: impl IntoIterator, + cx: &mut AppContext, + ) -> Model> { + cx.new_model(|_| { + Box::new(Self { + tasks: task_names + .into_iter() + .enumerate() + .map(|(i, name)| TestTask { + id: TaskId(format!("task_{i}_{name}")), + name, + }) + .collect(), + }) as Box + }) + } + } + + impl TaskSource for TestSource { + fn tasks_for_path( + &mut self, + _path: Option<&Path>, + _cx: &mut ModelContext>, + ) -> Vec> { + self.tasks + .clone() + .into_iter() + .map(|task| Arc::new(task) as Arc) + .collect() + } + + fn as_any(&mut self) -> &mut dyn std::any::Any { + self + } + } + + fn list_task_names( + inventory: &Model, + path: Option<&Path>, + lru: bool, + cx: &mut TestAppContext, + ) -> Vec { + inventory.update(cx, |inventory, cx| { + inventory + .list_tasks(path, lru, cx) + .into_iter() + .map(|task| task.name().to_string()) + .collect() + }) + } + + fn register_task_used(inventory: &Model, task_name: &str, cx: &mut TestAppContext) { + inventory.update(cx, |inventory, cx| { + let task = inventory + .list_tasks(None, false, cx) + .into_iter() + .find(|task| task.name() == task_name) + .unwrap_or_else(|| panic!("Failed to find task with name {task_name}")); + inventory.task_scheduled(task.id().clone()); + }); } } diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index b9a5422227..daeff4844f 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/crates/project_panel/src/project_panel.rs @@ -1182,11 +1182,15 @@ impl ProjectPanel { let num_and_remainder_a = Path::new(component_a.as_os_str()) .file_stem() .and_then(|s| s.to_str()) - .and_then(NumericPrefixWithSuffix::from_str)?; + .and_then( + NumericPrefixWithSuffix::from_numeric_prefixed_str, + )?; let num_and_remainder_b = Path::new(component_b.as_os_str()) .file_stem() .and_then(|s| s.to_str()) - .and_then(NumericPrefixWithSuffix::from_str)?; + .and_then( + NumericPrefixWithSuffix::from_numeric_prefixed_str, + )?; num_and_remainder_a.partial_cmp(&num_and_remainder_b) }); diff --git a/crates/task/src/lib.rs b/crates/task/src/lib.rs index b008c7e836..84d7bd4934 100644 --- a/crates/task/src/lib.rs +++ b/crates/task/src/lib.rs @@ -56,13 +56,13 @@ pub trait Task { /// /// Implementations of this trait could be e.g. [`StaticSource`] that parses tasks from a .json files and provides process templates to be spawned; /// another one could be a language server providing lenses with tests or build server listing all targets for a given project. -pub trait Source: Any { +pub trait TaskSource: Any { /// A way to erase the type of the source, processing and storing them generically. fn as_any(&mut self) -> &mut dyn Any; /// Collects all tasks available for scheduling, for the path given. fn tasks_for_path( &mut self, path: Option<&Path>, - cx: &mut ModelContext>, + cx: &mut ModelContext>, ) -> Vec>; } diff --git a/crates/task/src/oneshot_source.rs b/crates/task/src/oneshot_source.rs index 0c874a4d3e..829c2e2f3e 100644 --- a/crates/task/src/oneshot_source.rs +++ b/crates/task/src/oneshot_source.rs @@ -2,7 +2,7 @@ use std::sync::Arc; -use crate::{Source, SpawnInTerminal, Task, TaskId}; +use crate::{SpawnInTerminal, Task, TaskId, TaskSource}; use gpui::{AppContext, Context, Model}; /// A storage and source of tasks generated out of user command prompt inputs. @@ -54,8 +54,8 @@ impl Task for OneshotTask { impl OneshotSource { /// Initializes the oneshot source, preparing to store user prompts. - pub fn new(cx: &mut AppContext) -> Model> { - cx.new_model(|_| Box::new(Self { tasks: Vec::new() }) as Box) + pub fn new(cx: &mut AppContext) -> Model> { + cx.new_model(|_| Box::new(Self { tasks: Vec::new() }) as Box) } /// Spawns a certain task based on the user prompt. @@ -66,7 +66,7 @@ impl OneshotSource { } } -impl Source for OneshotSource { +impl TaskSource for OneshotSource { fn as_any(&mut self) -> &mut dyn std::any::Any { self } @@ -74,7 +74,7 @@ impl Source for OneshotSource { fn tasks_for_path( &mut self, _path: Option<&std::path::Path>, - _cx: &mut gpui::ModelContext>, + _cx: &mut gpui::ModelContext>, ) -> Vec> { self.tasks.clone() } diff --git a/crates/task/src/static_source.rs b/crates/task/src/static_source.rs index 2f7f7b4d74..7e2d59ad08 100644 --- a/crates/task/src/static_source.rs +++ b/crates/task/src/static_source.rs @@ -12,7 +12,7 @@ use schemars::{gen::SchemaSettings, JsonSchema}; use serde::{Deserialize, Serialize}; use util::ResultExt; -use crate::{Source, SpawnInTerminal, Task, TaskId}; +use crate::{SpawnInTerminal, Task, TaskId, TaskSource}; use futures::channel::mpsc::UnboundedReceiver; /// A single config file entry with the deserialized task definition. @@ -152,12 +152,12 @@ impl StaticSource { pub fn new( tasks_file_tracker: UnboundedReceiver, cx: &mut AppContext, - ) -> Model> { + ) -> Model> { let definitions = TrackedFile::new(DefinitionProvider::default(), tasks_file_tracker, cx); cx.new_model(|cx| { let _subscription = cx.observe( &definitions, - |source: &mut Box<(dyn Source + 'static)>, new_definitions, cx| { + |source: &mut Box<(dyn TaskSource + 'static)>, new_definitions, cx| { if let Some(static_source) = source.as_any().downcast_mut::() { static_source.tasks = new_definitions .read(cx) @@ -181,11 +181,11 @@ impl StaticSource { } } -impl Source for StaticSource { +impl TaskSource for StaticSource { fn tasks_for_path( &mut self, _: Option<&Path>, - _: &mut ModelContext>, + _: &mut ModelContext>, ) -> Vec> { self.tasks .clone() diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index ae3ab3ee5a..4bc939dece 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -497,9 +497,9 @@ impl RangeExt for RangeInclusive { pub struct NumericPrefixWithSuffix<'a>(i32, &'a str); impl<'a> NumericPrefixWithSuffix<'a> { - pub fn from_str(str: &'a str) -> Option { + pub fn from_numeric_prefixed_str(str: &'a str) -> Option { let mut chars = str.chars(); - let prefix: String = chars.by_ref().take_while(|c| c.is_digit(10)).collect(); + let prefix: String = chars.by_ref().take_while(|c| c.is_ascii_digit()).collect(); let remainder = chars.as_str(); match prefix.parse::() { @@ -514,7 +514,7 @@ impl Ord for NumericPrefixWithSuffix<'_> { let NumericPrefixWithSuffix(num_a, remainder_a) = self; let NumericPrefixWithSuffix(num_b, remainder_b) = other; num_a - .cmp(&num_b) + .cmp(num_b) .then_with(|| UniCase::new(remainder_a).cmp(&UniCase::new(remainder_b))) } } @@ -569,7 +569,7 @@ mod tests { fn test_numeric_prefix_with_suffix() { let mut sorted = vec!["1-abc", "10", "11def", "2", "21-abc"]; sorted.sort_by_key(|s| { - NumericPrefixWithSuffix::from_str(s).unwrap_or_else(|| { + NumericPrefixWithSuffix::from_numeric_prefixed_str(s).unwrap_or_else(|| { panic!("Cannot convert string `{s}` into NumericPrefixWithSuffix") }) }); @@ -577,7 +577,7 @@ mod tests { for numeric_prefix_less in ["numeric_prefix_less", "aaa", "~™£"] { assert_eq!( - NumericPrefixWithSuffix::from_str(numeric_prefix_less), + NumericPrefixWithSuffix::from_numeric_prefixed_str(numeric_prefix_less), None, "String without numeric prefix `{numeric_prefix_less}` should not be converted into NumericPrefixWithSuffix" )