fix testing for packages with multiple targets

fix test running by invoking cargo per package

remove hack_recover_crate_name

make clippy happy

Author: duncan

crates/project-model/src/cargo_workspace.rs

@@ -1,7 +1,7 @@
 //! See [`CargoWorkspace`].
 
-use std::ops;
 use std::str::from_utf8;
+use std::{fmt, ops};
 
 use anyhow::Context;
 use base_db::Env;
@@ -228,6 +228,25 @@ pub enum TargetKind {
     Other,
 }
 
+impl fmt::Display for TargetKind {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "{}",
+            match self {
+                TargetKind::Bin => "bin",
+                TargetKind::Lib { is_proc_macro: true } => "proc-macro",
+                TargetKind::Lib { is_proc_macro: false } => "lib",
+                TargetKind::Example => "example",
+                TargetKind::Test => "test",
+                TargetKind::Bench => "bench",
+                TargetKind::BuildScript => "custom-build",
+                TargetKind::Other => "other",
+            }
+        )
+    }
+}
+
 impl TargetKind {
     fn new(kinds: &[String]) -> TargetKind {
         for kind in kinds {

crates/rust-analyzer/src/command.rs

@@ -14,23 +14,60 @@ 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.
+/// cargo output into a Rust data type where the data is self-describing.
 pub(crate) trait ParseFromLine: Sized + Send + 'static {
     fn from_line(line: &str, error: &mut String) -> Option<Self>;
     fn from_eof() -> Option<Self>;
 }
 
+/// When Cargo output is not fully self-describing, a parser object can supply sufficient
+/// context to interpret the output correctly.
+pub(crate) trait CargoParser<T>: Send + 'static {
+    fn from_line(&self, line: &str, error: &mut String) -> Option<T>;
+    fn from_eof(&self) -> Option<T>;
+}
+
+/// Allow using the ParseFromLine trait as a parser object
+struct StatelessParser<T: ParseFromLine> {
+    marker: PhantomData<T>,
+}
+
+impl<T: ParseFromLine> StatelessParser<T> {
+    pub(crate) fn new() -> Self {
+        StatelessParser { marker: PhantomData }
+    }
+}
+
+impl<T: ParseFromLine> CargoParser<T> for StatelessParser<T> {
+    fn from_line(&self, line: &str, error: &mut String) -> Option<T> {
+        T::from_line(line, error)
+    }
+
+    fn from_eof(&self) -> Option<T> {
+        T::from_eof()
+    }
+}
+
 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 +84,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 +105,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();
                 }
             },
@@ -117,7 +154,17 @@ 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> {
+    pub(crate) fn spawn(command: Command, sender: Sender<T>) -> std::io::Result<Self> {
+        Self::spawn_with_parser(command, StatelessParser::<T>::new(), sender)
+    }
+}
+
+impl<T: Sized + Send + 'static> CommandHandle<T> {
+    pub(crate) fn spawn_with_parser(
+        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 +181,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())

crates/rust-analyzer/src/hack_recover_crate_name.rs

@@ -36,7 +36,6 @@ use crate::{
     config::{Config, RustfmtConfig, WorkspaceSymbolConfig},
     diagnostics::convert_diagnostic,
     global_state::{FetchWorkspaceRequest, GlobalState, GlobalStateSnapshot},
-    hack_recover_crate_name,
     line_index::LineEndings,
     lsp::{
         ext::{
@@ -194,74 +193,88 @@ 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
+            let tests = match params.include {
+                Some(ref include) => include
+                    .iter()
+                    .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
@@ -285,9 +298,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()

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

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