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

fix testing packages with multiple targets

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)]

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())

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
     }
 }

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
     }
 }

crates/rust-analyzer/src/hack_recover_crate_name.rs

@@ -1,25 +0,0 @@
-//! Currently cargo does not emit crate name in the `cargo test --format=json`, which needs to be changed. This
-//! module contains a way to recover crate names in a very hacky and wrong way.
-
-// FIXME(hack_recover_crate_name): Remove this module.
-
-use std::sync::{Mutex, MutexGuard, OnceLock};
-
-use ide_db::FxHashMap;
-
-static STORAGE: OnceLock<Mutex<FxHashMap<String, String>>> = OnceLock::new();
-
-fn get_storage() -> MutexGuard<'static, FxHashMap<String, String>> {
-    STORAGE.get_or_init(|| Mutex::new(FxHashMap::default())).lock().unwrap()
-}
-
-pub(crate) fn insert_name(name_with_crate: String) {
-    let Some((_, name_without_crate)) = name_with_crate.split_once("::") else {
-        return;
-    };
-    get_storage().insert(name_without_crate.to_owned(), name_with_crate);
-}
-
-pub(crate) fn lookup_name(name_without_crate: String) -> Option<String> {
-    get_storage().get(&name_without_crate).cloned()
-}

crates/rust-analyzer/src/handlers/request.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,20 +194,48 @@ 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,
@@ -216,53 +243,41 @@ pub(crate) fn handle_run_test(
     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()

crates/rust-analyzer/src/lib.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;

crates/rust-analyzer/src/main_loop.rs

@@ -26,7 +26,6 @@ use crate::{
         FetchBuildDataResponse, FetchWorkspaceRequest, FetchWorkspaceResponse, GlobalState,
         file_id_to_url, url_to_file_id,
     },
-    hack_recover_crate_name,
     handlers::{
         dispatch::{NotificationDispatcher, RequestDispatcher},
         request::empty_diagnostic_report,
@@ -37,7 +36,7 @@ use crate::{
     },
     lsp_ext,
     reload::{BuildDataProgress, ProcMacroProgress, ProjectWorkspaceProgress},
-    test_runner::{CargoTestMessage, TestState},
+    test_runner::{CargoTestMessage, CargoTestOutput, TestState},
 };
 
 pub fn main_loop(config: Config, connection: Connection) -> anyhow::Result<()> {
@@ -665,9 +664,7 @@ impl GlobalState {
                     .filter_map(|f| snapshot.analysis.discover_tests_in_file(f).ok())
                     .flatten()
                     .collect::<Vec<_>>();
-                for t in &tests {
-                    hack_recover_crate_name::insert_name(t.id.clone());
-                }
+
                 Task::DiscoverTest(lsp_ext::DiscoverTestResults {
                     tests: tests
                         .into_iter()
@@ -964,30 +961,29 @@ impl GlobalState {
     }
 
     fn handle_cargo_test_msg(&mut self, message: CargoTestMessage) {
-        match message {
-            CargoTestMessage::Test { name, state } => {
+        match message.output {
+            CargoTestOutput::Test { name, state } => {
                 let state = match state {
                     TestState::Started => lsp_ext::TestState::Started,
                     TestState::Ignored => lsp_ext::TestState::Skipped,
                     TestState::Ok => lsp_ext::TestState::Passed,
                     TestState::Failed { stdout } => lsp_ext::TestState::Failed { message: stdout },
                 };
-                let Some(test_id) = hack_recover_crate_name::lookup_name(name) else {
-                    return;
-                };
+                let test_id = format!("{}::{name}", message.target.target);
+
                 self.send_notification::<lsp_ext::ChangeTestState>(
                     lsp_ext::ChangeTestStateParams { test_id, state },
                 );
             }
-            CargoTestMessage::Suite => (),
-            CargoTestMessage::Finished => {
+            CargoTestOutput::Suite => (),
+            CargoTestOutput::Finished => {
                 self.test_run_remaining_jobs = self.test_run_remaining_jobs.saturating_sub(1);
                 if self.test_run_remaining_jobs == 0 {
                     self.send_notification::<lsp_ext::EndRunTest>(());
                     self.test_run_session = None;
                 }
             }
-            CargoTestMessage::Custom { text } => {
+            CargoTestOutput::Custom { text } => {
                 self.send_notification::<lsp_ext::AppendOutputToRunTest>(text);
             }
         }

crates/rust-analyzer/src/test_runner.rs

@@ -3,12 +3,13 @@
 
 use crossbeam_channel::Sender;
 use paths::AbsPath;
+use project_model::TargetKind;
 use serde::Deserialize as _;
 use serde_derive::Deserialize;
 use toolchain::Tool;
 
 use crate::{
-    command::{CommandHandle, ParseFromLine},
+    command::{CargoParser, CommandHandle},
     flycheck::CargoOptions,
 };
 
@@ -25,9 +26,15 @@ pub(crate) enum TestState {
     },
 }
 
+#[derive(Debug)]
+pub(crate) struct CargoTestMessage {
+    pub target: TestTarget,
+    pub output: CargoTestOutput,
+}
+
 #[derive(Debug, Deserialize)]
 #[serde(tag = "type", rename_all = "camelCase")]
-pub(crate) enum CargoTestMessage {
+pub(crate) enum CargoTestOutput {
     Test {
         name: String,
         #[serde(flatten)]
@@ -40,19 +47,33 @@ pub(crate) enum CargoTestMessage {
     },
 }
 
-impl ParseFromLine for CargoTestMessage {
-    fn from_line(line: &str, _: &mut String) -> Option<Self> {
+pub(crate) struct CargoTestOutputParser {
+    pub target: TestTarget,
+}
+
+impl CargoTestOutputParser {
+    pub(crate) fn new(test_target: &TestTarget) -> Self {
+        Self { target: test_target.clone() }
+    }
+}
+
+impl CargoParser<CargoTestMessage> for CargoTestOutputParser {
+    fn from_line(&self, line: &str, _error: &mut String) -> Option<CargoTestMessage> {
         let mut deserializer = serde_json::Deserializer::from_str(line);
         deserializer.disable_recursion_limit();
-        if let Ok(message) = CargoTestMessage::deserialize(&mut deserializer) {
-            return Some(message);
-        }
 
-        Some(CargoTestMessage::Custom { text: line.to_owned() })
+        Some(CargoTestMessage {
+            target: self.target.clone(),
+            output: if let Ok(message) = CargoTestOutput::deserialize(&mut deserializer) {
+                message
+            } else {
+                CargoTestOutput::Custom { text: line.to_owned() }
+            },
+        })
     }
 
-    fn from_eof() -> Option<Self> {
-        Some(CargoTestMessage::Finished)
+    fn from_eof(&self) -> Option<CargoTestMessage> {
+        Some(CargoTestMessage { target: self.target.clone(), output: CargoTestOutput::Finished })
     }
 }
 
@@ -62,14 +83,14 @@ pub(crate) struct CargoTestHandle {
 }
 
 // Example of a cargo test command:
-// cargo test --workspace --no-fail-fast -- -Z unstable-options --format=json
-// or
-// cargo test --package my-package --no-fail-fast -- module::func -Z unstable-options --format=json
+//
+// cargo test --package my-package --bin my_bin --no-fail-fast -- module::func -Z unstable-options --format=json
 
-#[derive(Debug)]
-pub(crate) enum TestTarget {
-    Workspace,
-    Package(String),
+#[derive(Debug, Clone)]
+pub(crate) struct TestTarget {
+    pub package: String,
+    pub target: String,
+    pub kind: TargetKind,
 }
 
 impl CargoTestHandle {
@@ -84,15 +105,18 @@ impl CargoTestHandle {
         cmd.env("RUSTC_BOOTSTRAP", "1");
         cmd.arg("test");
 
-        match &test_target {
-            TestTarget::Package(package) => {
-                cmd.arg("--package");
-                cmd.arg(package);
-            }
-            TestTarget::Workspace => {
-                cmd.arg("--workspace");
-            }
-        };
+        cmd.arg("--package");
+        cmd.arg(&test_target.package);
+
+        if let TargetKind::Lib { .. } = test_target.kind {
+            // no name required with lib because there can only be one lib target per package
+            cmd.arg("--lib");
+        } else if let Some(cargo_target) = test_target.kind.as_cargo_target() {
+            cmd.arg(format!("--{cargo_target}"));
+            cmd.arg(&test_target.target);
+        } else {
+            tracing::warn!("Running test for unknown cargo target {:?}", test_target.kind);
+        }
 
         // --no-fail-fast is needed to ensure that all requested tests will run
         cmd.arg("--no-fail-fast");
@@ -110,6 +134,8 @@ impl CargoTestHandle {
             cmd.arg(extra_arg);
         }
 
-        Ok(Self { _handle: CommandHandle::spawn(cmd, sender)? })
+        Ok(Self {
+            _handle: CommandHandle::spawn(cmd, CargoTestOutputParser::new(&test_target), sender)?,
+        })
     }
 }