Merge pull request #19005 from duncanawoods/18955---fix-running-tests-for-packages-with-multiple-targets

fix testing packages with multiple targets

Author: HKalbasi

src/tools/rust-analyzer/crates/project-model/src/cargo_workspace.rs

@@ -226,6 +226,7 @@ pub enum TargetKind {
     Example,
     Test,
     Bench,
+    /// Cargo calls this kind `custom-build`
     BuildScript,
     Other,
 }
@@ -254,6 +255,22 @@ impl TargetKind {
     pub fn is_proc_macro(self) -> bool {
         matches!(self, TargetKind::Lib { is_proc_macro: true })
     }
+
+    /// If this is a valid cargo target, returns the name cargo uses in command line arguments
+    /// and output, otherwise None.
+    /// https://docs.rs/cargo_metadata/latest/cargo_metadata/enum.TargetKind.html
+    pub fn as_cargo_target(self) -> Option<&'static str> {
+        match self {
+            TargetKind::Bin => Some("bin"),
+            TargetKind::Lib { is_proc_macro: true } => Some("proc-macro"),
+            TargetKind::Lib { is_proc_macro: false } => Some("lib"),
+            TargetKind::Example => Some("example"),
+            TargetKind::Test => Some("test"),
+            TargetKind::Bench => Some("bench"),
+            TargetKind::BuildScript => Some("custom-build"),
+            TargetKind::Other => None,
+        }
+    }
 }
 
 #[derive(Default, Clone, Debug, PartialEq, Eq)]

src/tools/rust-analyzer/crates/rust-analyzer/src/command.rs

@@ -13,24 +13,33 @@ use crossbeam_channel::Sender;
 use process_wrap::std::{StdChildWrapper, StdCommandWrap};
 use stdx::process::streaming_output;
 
-/// Cargo output is structured as a one JSON per line. This trait abstracts parsing one line of
-/// cargo output into a Rust data type.
-pub(crate) trait ParseFromLine: Sized + Send + 'static {
-    fn from_line(line: &str, error: &mut String) -> Option<Self>;
-    fn from_eof() -> Option<Self>;
+/// Cargo output is structured as one JSON per line. This trait abstracts parsing one line of
+/// cargo output into a Rust data type
+pub(crate) trait CargoParser<T>: Send + 'static {
+    fn from_line(&self, line: &str, error: &mut String) -> Option<T>;
+    fn from_eof(&self) -> Option<T>;
 }
 
 struct CargoActor<T> {
+    parser: Box<dyn CargoParser<T>>,
     sender: Sender<T>,
     stdout: ChildStdout,
     stderr: ChildStderr,
 }
 
-impl<T: ParseFromLine> CargoActor<T> {
-    fn new(sender: Sender<T>, stdout: ChildStdout, stderr: ChildStderr) -> Self {
-        CargoActor { sender, stdout, stderr }
+impl<T: Sized + Send + 'static> CargoActor<T> {
+    fn new(
+        parser: impl CargoParser<T>,
+        sender: Sender<T>,
+        stdout: ChildStdout,
+        stderr: ChildStderr,
+    ) -> Self {
+        let parser = Box::new(parser);
+        CargoActor { parser, sender, stdout, stderr }
     }
+}
 
+impl<T: Sized + Send + 'static> CargoActor<T> {
     fn run(self) -> io::Result<(bool, String)> {
         // We manually read a line at a time, instead of using serde's
         // stream deserializers, because the deserializer cannot recover
@@ -47,7 +56,7 @@ impl<T: ParseFromLine> CargoActor<T> {
         let mut read_at_least_one_stderr_message = false;
         let process_line = |line: &str, error: &mut String| {
             // Try to deserialize a message from Cargo or Rustc.
-            if let Some(t) = T::from_line(line, error) {
+            if let Some(t) = self.parser.from_line(line, error) {
                 self.sender.send(t).unwrap();
                 true
             } else {
@@ -68,7 +77,7 @@ impl<T: ParseFromLine> CargoActor<T> {
                 }
             },
             &mut || {
-                if let Some(t) = T::from_eof() {
+                if let Some(t) = self.parser.from_eof() {
                     self.sender.send(t).unwrap();
                 }
             },
@@ -116,8 +125,12 @@ impl<T> fmt::Debug for CommandHandle<T> {
     }
 }
 
-impl<T: ParseFromLine> CommandHandle<T> {
-    pub(crate) fn spawn(mut command: Command, sender: Sender<T>) -> std::io::Result<Self> {
+impl<T: Sized + Send + 'static> CommandHandle<T> {
+    pub(crate) fn spawn(
+        mut command: Command,
+        parser: impl CargoParser<T>,
+        sender: Sender<T>,
+    ) -> std::io::Result<Self> {
         command.stdout(Stdio::piped()).stderr(Stdio::piped()).stdin(Stdio::null());
 
         let program = command.get_program().into();
@@ -134,7 +147,7 @@ impl<T: ParseFromLine> CommandHandle<T> {
         let stdout = child.0.stdout().take().unwrap();
         let stderr = child.0.stderr().take().unwrap();
 
-        let actor = CargoActor::<T>::new(sender, stdout, stderr);
+        let actor = CargoActor::<T>::new(parser, sender, stdout, stderr);
         let thread = stdx::thread::Builder::new(stdx::thread::ThreadIntent::Worker)
             .name("CommandHandle".to_owned())
             .spawn(move || actor.run())

src/tools/rust-analyzer/crates/rust-analyzer/src/discover.rs

@@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use tracing::{info_span, span::EnteredSpan};
 
-use crate::command::{CommandHandle, ParseFromLine};
+use crate::command::{CargoParser, CommandHandle};
 
 pub(crate) const ARG_PLACEHOLDER: &str = "{arg}";
 
@@ -66,7 +66,7 @@ impl DiscoverCommand {
         cmd.args(args);
 
         Ok(DiscoverHandle {
-            _handle: CommandHandle::spawn(cmd, self.sender.clone())?,
+            _handle: CommandHandle::spawn(cmd, DiscoverProjectParser, self.sender.clone())?,
             span: info_span!("discover_command").entered(),
         })
     }
@@ -115,8 +115,10 @@ impl DiscoverProjectMessage {
     }
 }
 
-impl ParseFromLine for DiscoverProjectMessage {
-    fn from_line(line: &str, _error: &mut String) -> Option<Self> {
+struct DiscoverProjectParser;
+
+impl CargoParser<DiscoverProjectMessage> for DiscoverProjectParser {
+    fn from_line(&self, line: &str, _error: &mut String) -> Option<DiscoverProjectMessage> {
         // can the line even be deserialized as JSON?
         let Ok(data) = serde_json::from_str::<Value>(line) else {
             let err = DiscoverProjectData::Error { error: line.to_owned(), source: None };
@@ -131,7 +133,7 @@ impl ParseFromLine for DiscoverProjectMessage {
         Some(msg)
     }
 
-    fn from_eof() -> Option<Self> {
+    fn from_eof(&self) -> Option<DiscoverProjectMessage> {
         None
     }
 }

src/tools/rust-analyzer/crates/rust-analyzer/src/flycheck.rs

@@ -17,7 +17,7 @@ pub(crate) use cargo_metadata::diagnostic::{
 use toolchain::Tool;
 use triomphe::Arc;
 
-use crate::command::{CommandHandle, ParseFromLine};
+use crate::command::{CargoParser, CommandHandle};
 
 #[derive(Clone, Debug, Default, PartialEq, Eq)]
 pub(crate) enum InvocationStrategy {
@@ -329,7 +329,7 @@ impl FlycheckActor {
 
                     tracing::debug!(?command, "will restart flycheck");
                     let (sender, receiver) = unbounded();
-                    match CommandHandle::spawn(command, sender) {
+                    match CommandHandle::spawn(command, CargoCheckParser, sender) {
                         Ok(command_handle) => {
                             tracing::debug!(command = formatted_command, "did restart flycheck");
                             self.command_handle = Some(command_handle);
@@ -558,8 +558,10 @@ enum CargoCheckMessage {
     Diagnostic { diagnostic: Diagnostic, package_id: Option<Arc<PackageId>> },
 }
 
-impl ParseFromLine for CargoCheckMessage {
-    fn from_line(line: &str, error: &mut String) -> Option<Self> {
+struct CargoCheckParser;
+
+impl CargoParser<CargoCheckMessage> for CargoCheckParser {
+    fn from_line(&self, line: &str, error: &mut String) -> Option<CargoCheckMessage> {
         let mut deserializer = serde_json::Deserializer::from_str(line);
         deserializer.disable_recursion_limit();
         if let Ok(message) = JsonMessage::deserialize(&mut deserializer) {
@@ -588,7 +590,7 @@ impl ParseFromLine for CargoCheckMessage {
         None
     }
 
-    fn from_eof() -> Option<Self> {
+    fn from_eof(&self) -> Option<CargoCheckMessage> {
         None
     }
 }

src/tools/rust-analyzer/crates/rust-analyzer/src/hack_recover_crate_name.rs

@@ -35,7 +35,6 @@ use crate::{
     config::{Config, RustfmtConfig, WorkspaceSymbolConfig},
     diagnostics::convert_diagnostic,
     global_state::{FetchWorkspaceRequest, GlobalState, GlobalStateSnapshot},
-    hack_recover_crate_name,
     line_index::LineEndings,
     lsp::{
         LspError, completion_item_hash,
@@ -195,74 +194,90 @@ pub(crate) fn handle_view_item_tree(
     Ok(res)
 }
 
-// cargo test requires the real package name which might contain hyphens but
-// the test identifier passed to this function is the namespace form where hyphens
-// are replaced with underscores so we have to reverse this and find the real package name
-fn find_package_name(namespace_root: &str, cargo: &CargoWorkspace) -> Option<String> {
+// cargo test requires:
+// - the package name - the root of the test identifier supplied to this handler can be
+//   a package or a target inside a package.
+// - the target name - if the test identifier is a target, it's needed in addition to the
+//   package name to run the right test
+// - real names - the test identifier uses the namespace form where hyphens are replaced with
+//   underscores. cargo test requires the real name.
+// - the target kind e.g. bin or lib
+fn find_test_target(namespace_root: &str, cargo: &CargoWorkspace) -> Option<TestTarget> {
     cargo.packages().find_map(|p| {
         let package_name = &cargo[p].name;
-        if package_name.replace('-', "_") == namespace_root {
-            Some(package_name.clone())
-        } else {
-            None
+        for target in cargo[p].targets.iter() {
+            let target_name = &cargo[*target].name;
+            if target_name.replace('-', "_") == namespace_root {
+                return Some(TestTarget {
+                    package: package_name.clone(),
+                    target: target_name.clone(),
+                    kind: cargo[*target].kind,
+                });
+            }
         }
+        None
     })
 }
 
+fn get_all_targets(cargo: &CargoWorkspace) -> Vec<TestTarget> {
+    cargo
+        .packages()
+        .flat_map(|p| {
+            let package_name = &cargo[p].name;
+            cargo[p].targets.iter().map(|target| {
+                let target_name = &cargo[*target].name;
+                TestTarget {
+                    package: package_name.clone(),
+                    target: target_name.clone(),
+                    kind: cargo[*target].kind,
+                }
+            })
+        })
+        .collect()
+}
+
 pub(crate) fn handle_run_test(
     state: &mut GlobalState,
     params: lsp_ext::RunTestParams,
 ) -> anyhow::Result<()> {
     if let Some(_session) = state.test_run_session.take() {
         state.send_notification::<lsp_ext::EndRunTest>(());
     }
-    // We detect the lowest common ancestor of all included tests, and
-    // run it. We ignore excluded tests for now, the client will handle
-    // it for us.
-    let lca = match params.include {
-        Some(tests) => tests
-            .into_iter()
-            .reduce(|x, y| {
-                let mut common_prefix = "".to_owned();
-                for (xc, yc) in x.chars().zip(y.chars()) {
-                    if xc != yc {
-                        break;
-                    }
-                    common_prefix.push(xc);
-                }
-                common_prefix
-            })
-            .unwrap_or_default(),
-        None => "".to_owned(),
-    };
-    let (namespace_root, test_path) = if lca.is_empty() {
-        (None, None)
-    } else if let Some((namespace_root, path)) = lca.split_once("::") {
-        (Some(namespace_root), Some(path))
-    } else {
-        (Some(lca.as_str()), None)
-    };
+
     let mut handles = vec![];
     for ws in &*state.workspaces {
         if let ProjectWorkspaceKind::Cargo { cargo, .. } = &ws.kind {
-            let test_target = if let Some(namespace_root) = namespace_root {
-                if let Some(package_name) = find_package_name(namespace_root, cargo) {
-                    TestTarget::Package(package_name)
-                } else {
-                    TestTarget::Workspace
-                }
-            } else {
-                TestTarget::Workspace
+            // need to deduplicate `include` to avoid redundant test runs
+            let tests = match params.include {
+                Some(ref include) => include
+                    .iter()
+                    .unique()
+                    .filter_map(|test| {
+                        let (root, remainder) = match test.split_once("::") {
+                            Some((root, remainder)) => (root.to_owned(), Some(remainder)),
+                            None => (test.clone(), None),
+                        };
+                        if let Some(target) = find_test_target(&root, cargo) {
+                            Some((target, remainder))
+                        } else {
+                            tracing::error!("Test target not found for: {test}");
+                            None
+                        }
+                    })
+                    .collect_vec(),
+                None => get_all_targets(cargo).into_iter().map(|target| (target, None)).collect(),
             };
 
-            let handle = CargoTestHandle::new(
-                test_path,
-                state.config.cargo_test_options(None),
-                cargo.workspace_root(),
-                test_target,
-                state.test_run_sender.clone(),
-            )?;
-            handles.push(handle);
+            for (target, path) in tests {
+                let handle = CargoTestHandle::new(
+                    path,
+                    state.config.cargo_test_options(None),
+                    cargo.workspace_root(),
+                    target,
+                    state.test_run_sender.clone(),
+                )?;
+                handles.push(handle);
+            }
         }
     }
     // Each process send finished signal twice, once for stdout and once for stderr
@@ -286,9 +301,7 @@ pub(crate) fn handle_discover_test(
         }
         None => (snap.analysis.discover_test_roots()?, None),
     };
-    for t in &tests {
-        hack_recover_crate_name::insert_name(t.id.clone());
-    }
+
     Ok(lsp_ext::DiscoverTestResults {
         tests: tests
             .into_iter()

src/tools/rust-analyzer/crates/rust-analyzer/src/handlers/request.rs

@@ -24,7 +24,6 @@ mod command;
 mod diagnostics;
 mod discover;
 mod flycheck;
-mod hack_recover_crate_name;
 mod line_index;
 mod main_loop;
 mod mem_docs;