From e1fe31508c471eeb7c30916abd780b3930f7e42b Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Sun, 27 Aug 2023 10:16:09 +0100 Subject: [PATCH] fix(lsp/testing): use full ancestry to compute static id of step (#20297) Fixes https://github.com/denoland/vscode_deno/issues/656. Test steps were ID'd by a checksum of `[origin, level, step_name]` which is incorrect. Now it's `[origin, ...ancestor_names, step_name]`. --- cli/lsp/testing/collectors.rs | 120 ++++++++++++++++------------ cli/lsp/testing/definitions.rs | 33 +++++--- cli/lsp/testing/execution.rs | 142 +++++++++++++++++++-------------- cli/tools/test/mod.rs | 10 --- 4 files changed, 171 insertions(+), 134 deletions(-) diff --git a/cli/lsp/testing/collectors.rs b/cli/lsp/testing/collectors.rs index 8361ee1248..6976cffb95 100644 --- a/cli/lsp/testing/collectors.rs +++ b/cli/lsp/testing/collectors.rs @@ -15,7 +15,6 @@ use std::collections::HashSet; /// Parse an arrow expression for any test steps and return them. fn arrow_to_steps( parent: &str, - level: usize, arrow_expr: &ast::ArrowExpr, ) -> Vec { if let Some((maybe_test_context, maybe_step_var)) = @@ -23,7 +22,6 @@ fn arrow_to_steps( { let mut collector = TestStepCollector::new( parent.to_string(), - level, maybe_test_context, maybe_step_var, ); @@ -35,17 +33,12 @@ fn arrow_to_steps( } /// Parse a function for any test steps and return them. -fn fn_to_steps( - parent: &str, - level: usize, - function: &ast::Function, -) -> Vec { +fn fn_to_steps(parent: &str, function: &ast::Function) -> Vec { if let Some((maybe_test_context, maybe_step_var)) = parse_test_context_param(function.params.get(0).map(|p| &p.pat)) { let mut collector = TestStepCollector::new( parent.to_string(), - level, maybe_test_context, maybe_step_var, ); @@ -130,7 +123,6 @@ fn parse_test_context_param( fn check_call_expr( parent: &str, node: &ast::CallExpr, - level: usize, fns: Option<&HashMap>, text_info: Option<&SourceTextInfo>, ) -> Option<(String, Vec)> { @@ -164,10 +156,10 @@ fn check_call_expr( }, "fn" => match key_value_prop.value.as_ref() { ast::Expr::Arrow(arrow_expr) => { - steps = arrow_to_steps(parent, level, arrow_expr); + steps = arrow_to_steps(parent, arrow_expr); } ast::Expr::Fn(fn_expr) => { - steps = fn_to_steps(parent, level, &fn_expr.function); + steps = fn_to_steps(parent, &fn_expr.function); } _ => (), }, @@ -176,7 +168,7 @@ fn check_call_expr( } } ast::Prop::Method(method_prop) => { - steps = fn_to_steps(parent, level, &method_prop.function); + steps = fn_to_steps(parent, &method_prop.function); } _ => (), } @@ -187,7 +179,7 @@ fn check_call_expr( ast::Expr::Fn(fn_expr) => { if let Some(ast::Ident { sym, .. }) = fn_expr.ident.as_ref() { let name = sym.to_string(); - let steps = fn_to_steps(parent, level, &fn_expr.function); + let steps = fn_to_steps(parent, &fn_expr.function); Some((name, steps)) } else { None @@ -198,10 +190,10 @@ fn check_call_expr( let mut steps = vec![]; match node.args.get(1).map(|es| es.expr.as_ref()) { Some(ast::Expr::Fn(fn_expr)) => { - steps = fn_to_steps(parent, level, &fn_expr.function); + steps = fn_to_steps(parent, &fn_expr.function); } Some(ast::Expr::Arrow(arrow_expr)) => { - steps = arrow_to_steps(parent, level, arrow_expr); + steps = arrow_to_steps(parent, arrow_expr); } _ => (), } @@ -212,10 +204,10 @@ fn check_call_expr( let mut steps = vec![]; match node.args.get(1).map(|es| es.expr.as_ref()) { Some(ast::Expr::Fn(fn_expr)) => { - steps = fn_to_steps(parent, level, &fn_expr.function); + steps = fn_to_steps(parent, &fn_expr.function); } Some(ast::Expr::Arrow(arrow_expr)) => { - steps = arrow_to_steps(parent, level, arrow_expr); + steps = arrow_to_steps(parent, arrow_expr); } _ => (), } @@ -230,7 +222,7 @@ fn check_call_expr( fns.and_then(|fns| { fns .get(&name) - .map(|fn_expr| (name, fn_to_steps(parent, level, fn_expr))) + .map(|fn_expr| (name, fn_to_steps(parent, fn_expr))) }) } _ => { @@ -255,7 +247,6 @@ fn check_call_expr( /// branch contains any testing steps. struct TestStepCollector { steps: Vec, - level: usize, parent: String, maybe_test_context: Option, vars: HashSet, @@ -264,7 +255,6 @@ struct TestStepCollector { impl TestStepCollector { fn new( parent: String, - level: usize, maybe_test_context: Option, maybe_step_var: Option, ) -> Self { @@ -274,7 +264,6 @@ impl TestStepCollector { } Self { steps: Vec::default(), - level, parent, maybe_test_context, vars, @@ -287,19 +276,13 @@ impl TestStepCollector { range: SourceRange, steps: Vec, ) { - let step = TestDefinition::new_step( - name.as_ref().to_string(), - range, - self.parent.clone(), - self.level, - steps, - ); + let step = + TestDefinition::new_step(name.as_ref().to_string(), range, steps); self.steps.push(step); } fn check_call_expr(&mut self, node: &ast::CallExpr, range: SourceRange) { - if let Some((name, steps)) = - check_call_expr(&self.parent, node, self.level + 1, None, None) + if let Some((name, steps)) = check_call_expr(&self.parent, node, None, None) { self.add_step(name, range, steps); } @@ -436,7 +419,6 @@ impl TestCollector { if let Some((name, steps)) = check_call_expr( self.specifier.as_str(), node, - 1, Some(&self.fns), Some(&self.text_info), ) { @@ -579,7 +561,6 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(12, 16), steps: vec![], @@ -600,7 +581,6 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(12, 16), steps: vec![], @@ -631,21 +611,18 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(12, 16), steps: vec![TestDefinition { id: - "b3b2daad49e5c3095fe26aba0a840131f3d8f32e105e95507f5fc5118642b059" + "704d24083fd4a3e1bd204faa20827dc594334812245e5d45dda222b3edc60a0c" .to_string(), - level: 1, name: "step".to_string(), range: new_range(81, 85), steps: vec![TestDefinition { id: - "abf356f59139b77574089615f896a6f501c010985d95b8a93abeb0069ccb2201" + "0d006a4ec0abaa9cc1d18256b1ccd2677a4c882ff5cb807123890f7528ab1e8d" .to_string(), - level: 2, name: "sub step".to_string(), range: new_range(128, 132), steps: vec![], @@ -678,21 +655,18 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(12, 16), steps: vec![TestDefinition { id: - "b3b2daad49e5c3095fe26aba0a840131f3d8f32e105e95507f5fc5118642b059" + "704d24083fd4a3e1bd204faa20827dc594334812245e5d45dda222b3edc60a0c" .to_string(), - level: 1, name: "step".to_string(), range: new_range(81, 85), steps: vec![TestDefinition { id: - "abf356f59139b77574089615f896a6f501c010985d95b8a93abeb0069ccb2201" + "0d006a4ec0abaa9cc1d18256b1ccd2677a4c882ff5cb807123890f7528ab1e8d" .to_string(), - level: 2, name: "sub step".to_string(), range: new_range(128, 132), steps: vec![], @@ -716,7 +690,6 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(36, 40), steps: vec![], @@ -739,14 +712,12 @@ pub mod tests { vec![TestDefinition { id: "86b4c821900e38fc89f24bceb0e45193608ab3f9d2a6019c7b6a5aceff5d7df2" .to_string(), - level: 0, name: "useFnName".to_string(), range: new_range(12, 16), steps: vec![TestDefinition { id: - "b3b2daad49e5c3095fe26aba0a840131f3d8f32e105e95507f5fc5118642b059" + "dac8a169b8f8c6babf11122557ea545de2733bfafed594d044b22bc6863a0856" .to_string(), - level: 1, name: "step".to_string(), range: new_range(71, 72), steps: vec![], @@ -769,7 +740,6 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(34, 35), steps: vec![], @@ -791,7 +761,6 @@ pub mod tests { vec![TestDefinition { id: "4ebb361c93f76a0f1bac300638675609f1cf481e6f3b9006c3c98604b3a184e9" .to_string(), - level: 0, name: "test".to_string(), range: new_range(45, 49), steps: vec![], @@ -812,7 +781,6 @@ pub mod tests { vec![TestDefinition { id: "e0f6a73647b763f82176c98a019e54200b799a32007f9859fb782aaa9e308568" .to_string(), - level: 0, name: "someFunction".to_string(), range: new_range(12, 16), steps: vec![] @@ -834,7 +802,6 @@ pub mod tests { vec![TestDefinition { id: "e0f6a73647b763f82176c98a019e54200b799a32007f9859fb782aaa9e308568" .to_string(), - level: 0, name: "someFunction".to_string(), range: new_range(51, 55), steps: vec![] @@ -856,11 +823,60 @@ pub mod tests { vec![TestDefinition { id: "6d05d6dc35548b86a1e70acaf24a5bc2dd35db686b35b685ad5931d201b4a918" .to_string(), - level: 0, name: "Test 3:7".to_string(), range: new_range(79, 83), steps: vec![] }] ); } + + // Regression test for https://github.com/denoland/vscode_deno/issues/656. + #[test] + fn test_test_collector_nested_steps_same_name_and_level() { + let res = collect( + r#" + Deno.test("1", async (t) => { + await t.step("step 1", async (t) => { + await t.step("nested step", () => {}); + }); + await t.step("step 2", async (t) => { + await t.step("nested step", () => {}); + }); + }); + "#, + ); + + assert_eq!( + res, + vec![TestDefinition { + id: "3799fc549a32532145ffc8532b0cd943e025bbc19a02e2cde9be94f87bceb829".to_string(), + name: "1".to_string(), + range: new_range(12, 16), + steps: vec![ + TestDefinition { + id: "e714fc695c0895327bf7148a934c3303ad515af029a14906be46f80340c6d7e3".to_string(), + name: "step 1".to_string(), + range: new_range(53, 57), + steps: vec![TestDefinition { + id: "d874949e18dfc297e15c52ff13f13b4e6ae911ec1818b2c761e3313bc018a3ab".to_string(), + name: "nested step".to_string(), + range: new_range(101, 105), + steps: vec![], + }], + }, + TestDefinition { + id: "ec6b03d3dd3dde78d2d11ed981d3386083aeca701510cc049189d74bd79f8587".to_string(), + name: "step 2".to_string(), + range: new_range(160, 164), + steps: vec![TestDefinition { + id: "96729f1f1608e50160b0bf11946719384b4021fd1d26b14eff7765034b3d2684".to_string(), + name: "nested step".to_string(), + range: new_range(208, 212), + steps: vec![], + }], + }, + ] + }] + ); + } } diff --git a/cli/lsp/testing/definitions.rs b/cli/lsp/testing/definitions.rs index 1022fc43a2..6992c995de 100644 --- a/cli/lsp/testing/definitions.rs +++ b/cli/lsp/testing/definitions.rs @@ -15,7 +15,6 @@ use tower_lsp::lsp_types as lsp; #[derive(Debug, Clone, PartialEq)] pub struct TestDefinition { pub id: String, - pub level: usize, pub name: String, pub range: SourceRange, pub steps: Vec, @@ -26,33 +25,41 @@ impl TestDefinition { specifier: &ModuleSpecifier, name: String, range: SourceRange, - steps: Vec, + mut steps: Vec, ) -> Self { - let id = checksum::gen(&[specifier.as_str().as_bytes(), name.as_bytes()]); + let mut id_components = Vec::with_capacity(7); + id_components.push(specifier.as_str().as_bytes()); + id_components.push(name.as_bytes()); + let id = checksum::gen(&id_components); + Self::fix_ids(&mut steps, &mut id_components); Self { id, - level: 0, name, range, steps, } } + fn fix_ids<'a>( + steps: &'a mut Vec, + id_components: &mut Vec<&'a [u8]>, + ) { + for step in steps { + id_components.push(step.name.as_bytes()); + step.id = checksum::gen(id_components); + Self::fix_ids(&mut step.steps, id_components); + id_components.pop(); + } + } + pub fn new_step( name: String, range: SourceRange, - parent: String, - level: usize, steps: Vec, ) -> Self { - let id = checksum::gen(&[ - parent.as_bytes(), - &level.to_be_bytes(), - name.as_bytes(), - ]); Self { - id, - level, + // ID will be fixed later when the entire ancestry is available. + id: "".to_string(), name, range, steps, diff --git a/cli/lsp/testing/execution.rs b/cli/lsp/testing/execution.rs index 4a1e703222..54c5e69143 100644 --- a/cli/lsp/testing/execution.rs +++ b/cli/lsp/testing/execution.rs @@ -478,48 +478,96 @@ impl TestRun { } #[derive(Debug, PartialEq)] -enum TestOrTestStepDescription { +enum LspTestDescription { TestDescription(test::TestDescription), - TestStepDescription(test::TestStepDescription), + /// `(desc, static_id, root_static_id)` + TestStepDescription(test::TestStepDescription, String, String), } -impl TestOrTestStepDescription { +impl LspTestDescription { fn origin(&self) -> &str { match self { - TestOrTestStepDescription::TestDescription(d) => d.origin.as_str(), - TestOrTestStepDescription::TestStepDescription(d) => d.origin.as_str(), + LspTestDescription::TestDescription(d) => d.origin.as_str(), + LspTestDescription::TestStepDescription(d, _, _) => d.origin.as_str(), } } + + fn location(&self) -> &test::TestLocation { + match self { + LspTestDescription::TestDescription(d) => &d.location, + LspTestDescription::TestStepDescription(d, _, _) => &d.location, + } + } + + fn parent_id(&self) -> Option { + match self { + LspTestDescription::TestDescription(_) => None, + LspTestDescription::TestStepDescription(d, _, _) => Some(d.parent_id), + } + } + + fn from_step_description( + desc: &test::TestStepDescription, + tests: &IndexMap, + ) -> Self { + let mut id_components = Vec::with_capacity(7); + let mut root_static_id = None; + let mut step_desc = Some(desc); + while let Some(desc) = step_desc { + id_components.push(desc.name.as_bytes()); + let parent_desc = tests.get(&desc.parent_id).unwrap(); + step_desc = match parent_desc { + LspTestDescription::TestDescription(d) => { + id_components.push(d.name.as_bytes()); + root_static_id = Some(d.static_id()); + None + } + LspTestDescription::TestStepDescription(d, _, _) => Some(d), + }; + } + id_components.push(desc.origin.as_bytes()); + id_components.reverse(); + let static_id = checksum::gen(&id_components); + LspTestDescription::TestStepDescription( + desc.clone(), + static_id, + root_static_id.unwrap(), + ) + } } -impl From<&test::TestDescription> for TestOrTestStepDescription { +impl From<&test::TestDescription> for LspTestDescription { fn from(desc: &test::TestDescription) -> Self { Self::TestDescription(desc.clone()) } } -impl From<&test::TestStepDescription> for TestOrTestStepDescription { - fn from(desc: &test::TestStepDescription) -> Self { - Self::TestStepDescription(desc.clone()) - } -} - -impl From<&TestOrTestStepDescription> for lsp_custom::TestIdentifier { - fn from(desc: &TestOrTestStepDescription) -> lsp_custom::TestIdentifier { +impl From<&LspTestDescription> for lsp_custom::TestIdentifier { + fn from(desc: &LspTestDescription) -> lsp_custom::TestIdentifier { match desc { - TestOrTestStepDescription::TestDescription(test_desc) => test_desc.into(), - TestOrTestStepDescription::TestStepDescription(test_step_desc) => { - test_step_desc.into() + LspTestDescription::TestDescription(d) => d.into(), + LspTestDescription::TestStepDescription(d, static_id, root_static_id) => { + let uri = ModuleSpecifier::parse(&d.origin).unwrap(); + Self { + text_document: lsp::TextDocumentIdentifier { uri }, + id: Some(root_static_id.clone()), + step_id: Some(static_id.clone()), + } } } } } -impl From<&TestOrTestStepDescription> for lsp_custom::TestData { - fn from(desc: &TestOrTestStepDescription) -> Self { +impl From<&LspTestDescription> for lsp_custom::TestData { + fn from(desc: &LspTestDescription) -> Self { match desc { - TestOrTestStepDescription::TestDescription(desc) => desc.into(), - TestOrTestStepDescription::TestStepDescription(desc) => desc.into(), + LspTestDescription::TestDescription(d) => d.into(), + LspTestDescription::TestStepDescription(d, static_id, _) => Self { + id: static_id.clone(), + label: d.name.clone(), + steps: Default::default(), + range: None, + }, } } } @@ -546,37 +594,12 @@ impl From<&test::TestDescription> for lsp_custom::TestIdentifier { } } -impl From<&test::TestStepDescription> for lsp_custom::TestData { - fn from(desc: &test::TestStepDescription) -> Self { - Self { - id: desc.static_id(), - label: desc.name.clone(), - steps: Default::default(), - range: None, - } - } -} - -impl From<&test::TestStepDescription> for lsp_custom::TestIdentifier { - fn from(desc: &test::TestStepDescription) -> Self { - let uri = ModuleSpecifier::parse(&desc.origin).unwrap(); - Self { - text_document: lsp::TextDocumentIdentifier { uri }, - id: Some(checksum::gen(&[ - desc.origin.as_bytes(), - desc.root_name.as_bytes(), - ])), - step_id: Some(desc.static_id()), - } - } -} - struct LspTestReporter { client: Client, id: u32, maybe_root_uri: Option, files: Arc>>, - tests: IndexMap, + tests: IndexMap, current_test: Option, } @@ -713,26 +736,27 @@ impl LspTestReporter { } fn report_step_register(&mut self, desc: &test::TestStepDescription) { - self.tests.insert(desc.id, desc.into()); + self.tests.insert( + desc.id, + LspTestDescription::from_step_description(desc, &self.tests), + ); + let desc = self.tests.get(&desc.id).unwrap(); let mut files = self.files.lock(); let tds = files - .entry(ModuleSpecifier::parse(&desc.location.file_name).unwrap()) + .entry(ModuleSpecifier::parse(&desc.location().file_name).unwrap()) .or_default(); let data: lsp_custom::TestData = desc.into(); if tds.inject(data.clone()) { - let mut step_desc = Some(desc); let mut data = data; - while let Some(desc) = step_desc { - let parent_desc = self.tests.get(&desc.parent_id).unwrap(); + let mut current_desc = desc; + while let Some(parent_id) = current_desc.parent_id() { + let parent_desc = self.tests.get(&parent_id).unwrap(); let mut parent_data: lsp_custom::TestData = parent_desc.into(); parent_data.steps = vec![data]; data = parent_data; - step_desc = match parent_desc { - TestOrTestStepDescription::TestDescription(_) => None, - TestOrTestStepDescription::TestStepDescription(d) => Some(d), - } + current_desc = parent_desc; } - let specifier = ModuleSpecifier::parse(&desc.origin).unwrap(); + let specifier = ModuleSpecifier::parse(desc.origin()).unwrap(); let label = if let Some(root) = &self.maybe_root_uri { specifier.as_str().replace(root.as_str(), "") } else { @@ -758,6 +782,7 @@ impl LspTestReporter { if self.current_test == Some(desc.parent_id) { self.current_test = Some(desc.id); } + let desc = &LspTestDescription::from_step_description(desc, &self.tests); let test: lsp_custom::TestIdentifier = desc.into(); self.progress(lsp_custom::TestRunProgressMessage::Started { test }); } @@ -771,6 +796,7 @@ impl LspTestReporter { if self.current_test == Some(desc.id) { self.current_test = Some(desc.parent_id); } + let desc = &LspTestDescription::from_step_description(desc, &self.tests); match result { test::TestStepResult::Ok => { self.progress(lsp_custom::TestRunProgressMessage::Passed { @@ -848,7 +874,6 @@ mod tests { let test_def_a = TestDefinition { id: "0b7c6bf3cd617018d33a1bf982a08fe088c5bb54fcd5eb9e802e7c137ec1af94" .to_string(), - level: 0, name: "test a".to_string(), range: new_range(420, 424), steps: vec![], @@ -856,7 +881,6 @@ mod tests { let test_def_b = TestDefinition { id: "69d9fe87f64f5b66cb8b631d4fd2064e8224b8715a049be54276c42189ff8f9f" .to_string(), - level: 0, name: "test b".to_string(), range: new_range(480, 481), steps: vec![], diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs index 9ad205e2e7..2a0938e96e 100644 --- a/cli/tools/test/mod.rs +++ b/cli/tools/test/mod.rs @@ -297,16 +297,6 @@ pub struct TestStepDescription { pub root_name: String, } -impl TestStepDescription { - pub fn static_id(&self) -> String { - checksum::gen(&[ - self.location.file_name.as_bytes(), - &self.level.to_be_bytes(), - self.name.as_bytes(), - ]) - } -} - #[allow(clippy::derive_partial_eq_without_eq)] #[derive(Debug, Clone, PartialEq, Deserialize)] #[serde(rename_all = "camelCase")]