Repository: rust-lang/rust-analyzer
Pull request #17472 (2024-06-21 15:03:53)

#17470 - run unit tests at the crate level not workspace
For https://github.com/rust-lang/rust-analyzer/issues/17470

Use the test path to identify a package in the workspace and run the unit test there instead of at the workspace.

Veykril: r? @HKalbasi 
duncanawoods: Hi @Veykril @HKalbasi anything I can do to move this PR along? Thanks!
HKalbasi: Hi! Sorry for very late response, I missed the first mention.

Is the goal of this PR is to reduce the compile time of the tests by compiling only the specified crate? Is it consistence with what the `Run test` code lens do? IIRC the code lens always uses the `--workspace` flag. If it differs, then which one is more efficient?
duncanawoods: Hi HKalbasi,

> Is the goal of this PR is to reduce the compile time of the tests by compiling only the specified crate? 

I listed the problems in the issue https://github.com/rust-lang/rust-analyzer/issues/17470

My personal goal is just to efficiently execute a test from a keyboard shortcut. This invokes the Test Explorer version but it caused me too many problems:

- Compile time is sufficiently annoying to need a fix but it's more than that. If any unrelated package does not build then the test won't run which defeats common use cases e.g. running tests as you refactor a dependency. 
- It is also buggy and can run the wrong test. The user asks for a specific test to be run but the command treats it as a search pattern across the whole workspace and will run tests the user did not ask for.
- It also affects run-time behaviour which makes it even slower. As a search pattern, the runner loads every workspace member and tries to run the test. It even tries to run it as a doc test.

> Is it consistence with what the Run test code lens do?

The two runners do need to be unified:

1. The Test Explorer test runner does not add `extraTestBinaryArgs` so you can't add `nocapture`.

2. To properly integrate with VSCode, the Lens should invoke the Test Explorer "run test" command rather than do it's own thing so that:

- the result is shown in the Test Explorer
- the result is shown in the Test Results output pane
- progress for multiple tests is shown in the Test Explorer
- the test can be stopped/rerun etc. using VSCode controls (shown on hover at top of Test Explorer)

This means that the Lens no longer needs to spawn extra terminals to show the output. It would instead show the Test Results pane. The benefit of this is:

- test output is captured for each run so you can compare different runs
- test output is held for long
- test output is organised by test in the explorer and by time in results pane instead of one terminal
- the extra terminals are a pain in the :peach:
  - I mistake them for my real terminal and accidentally try to use them wasting time
  - closing the extra terminals is irritating clean up work after using the test lens
  - I use tmux to handle multiple terminals in vscode so having terminals spawned this way is undesirable

I could create an issue and look at that if you wanted?

> IIRC the code lens always uses the `--workspace` flag. If it differs, then which one is more efficient?

I assume you mean the opposite :) It never uses the workspace flag. I see it uses the "exact" flag which I didn't know about and likely desirable for stopping the "search pattern" behaviour. I'm not clear on the condition it has that causes it not be added though.

The Lens test runner code with `CargoTargetSpec` looks better structured whereas `handle_run_test` and `CargoTestHandle` used by the Test Explorer feels pretty hacky. The Test Explorer needs to run tests at different levels of hierarchy: workspace/package/folder/module/file so `CargoTargetSpec` would need to be extended if made the sole test runner. I imagine it will need care to prevent it growing into something overwhelming though.

HKalbasi: > My personal goal is just to efficiently execute a test from a keyboard shortcut. This invokes the Test Explorer version but it caused me too many problems:

Did you test this PR in your own workflow to see if it fixes your problems?

> To properly integrate with VSCode, the Lens should invoke the Test Explorer "run test" command rather than do it's own thing so that:

I think at the end we probably want to retire the `Run Test` lens and leave it to the client to handle the ui of running each test functionality, for example VSCode shows a green play button for each test which becomes red if test fails, and I think we don't need an additional lens when it is available.

But for now, test explorer is an experimental and second class feature, which is unstable and has less client support, so we should not use it in the stable and heavily used lens functionality. We may want to add an option for opting out of `Run test` lens, or disabling them if test explorer is active, but I don't think we should integrate code lens with the test explorer.

What I meant by keeping the code lens and test explorer consistent, was about the cargo command they use, not ui. That is, if code lens produce `cargo test --package pkg --lib -- test::pattern --exact --nocapture` we in test explorer should strive for emitting exact same command if possible (sometimes it is not, since test explorer support more cases), since the one emitted by the lens is very stable and battle tested. In this PR, I would like to use `--package pkg` instead of changing the working directory if it doesn't have any major drawbacks since it is closer to what lens does.
rustbot: 
There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:
```shell-session
$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-analyzer.git master
$ git push --force-with-lease
```

The following commits are merge commits:
- 837de789aaa65626d98f5c3638a35939c0a02a9f

HKalbasi: @duncanawoods did you intended to change something in your latest commit? I guess your change was lost during the rebase.
duncanawoods: > did you intended to change something in your latest commit

Just syncing the fork... tried out Github's tempting "sync fork" button which rustbot didn't like. I think the original change is still there.

> Did you test this PR in your own workflow to see if it fixes your problems?

Yep, I've used it for a month with no issues so officially certified as "works on my machine"... for one workspace.

> I don't think we should integrate code lens with the test explorer.

I'm not intending to refer to the Test Explorer but the VS Code API testing api `TestController` which is what you have successfully implemented in `test_explorer.ts`.

https://code.visualstudio.com/api/extension-guides/testing

As I understand it, this is the expected way for a language extension to support testing. It originated in the third party Test Explorer extension but was integrated as a native API and provides testing support across the vscode client which I believe includes the explorer, buttons in margins, menus and commands like `testing.runatcursor` which is why I am here. 

> has less client support

To me, that means the `TestController` implementation has more client support but you might mean something different?

As I understand the two flows at the moment:

- request:::handle_code_lens -> toproto::code_lens -> toproto::runnable -> CargoTargetSpec 
- TestController -> request::handle_run_test -> flycheck::CargoTestHandle

Unification to take the best from each would look something like:

- request:::handle_code_lens -> toproto::code_lens -> executeCommand('testing.run.uri')
- TestController -> toproto::runnable -> CargoTargetSpec

i.e.

- all testing to be invoked via the vscode testing api for full client support
- the code lens wouldn't launch it's own runner, just use vscode to execute a test uri
- replace `flycheck::CargoTestHandle` with the battle tested `CargoTargetSpec`
- extend `CargoTargetSpec` for workspace level targets

> We may want to add an option for opting out of Run test lens,

It's already optional in the settings.

> What I meant by keeping the code lens and test explorer consistent

I described some differences previously. It's pretty difficult to tell exactly how much they can vary from inspection because they build the command line from settings objects and there is a lot of branching / optional objects. The lens invocation has many layers to the preparation of the task where arguments and environment can be altered.

As an RA user, I highly value if RA logged every command line to help sort out issues with settings.

Anyway, the primary differences as far as I can tell:

|                | Code Lens - CargoTargetSpec                        | TestController - CargoTestHandle                                    |
| -------------- | -------------------------------------------------- | ------------------------------------------------------------------- |
| Path           | sets current working dir at workspace root         | passes manifest path as an argument                                 |
| Test Locator   | sets "--exact" and "--package"                     | sets "--workspace" and test path                                                 |
| Flags          | can set --ignored (don't think that is valid flag) |                                                                     |
| Settings       | adds "extra_test_binary_args"                      | does not -  so does not support nocapture if set by user               |
| Cargo          | can override cargo entirely from settings          | has a different way of locating Cargo from env                      |
| Toolchain      | sets toolchain env var                             |                                                                     |
| Multiple tests |                                                    | uses --no-fail-fast so a run does not halt on first failure         |
| Output         |                                                    | sets "-Z unstable-options" and "--format=json" for capturing output |

Which is pretty significant e.g. they might even use different toolchains.




HKalbasi: About client support, I meant that code lens is supported by vscode, neovim, emacs, ... (the clients of the r-a language server) but the code in the `test_explorer.ts` file (which works with the vscode testing api I refer to it as the test explorer. The vscode third party extension which is called test explorer is not relevant to this discussion and r-a in general) is only supported by vscode and has no support for other clients yet.

In the table of differences (which is helpful information, thank you) some of them are fundamental differences (e.g. `--format=json`) and some of them are accidental differences (like the test locator which this PR touches). The accidental ones occasionally make some problems (like here), and a solution is to do what the code lens does, if applicable. So by that logic, I would prefer this PR to use `--package` instead of changing the directory.

I'm fine with merging this PR as-is if there is some limitation with the `--package` approach or if you find it unnecessary and are not willing to do this change.
duncanawoods: > I would prefer this PR to use --package instead of changing the directory

1. Done. It can't use the `--exact` flag because it would need to know if the test path was a module or not. 

2. I've added the `extra_test_binary_args` which is other main discrepancy that causes me grief. I can roll that back if you don't want that.

3. testing notes

 tested with:
 - workspace
 - single crate
 - ubuntu 22.04, rust 1.80

tested:
 - test explorer ui
   - :green_square: test all
   - :green_square: test crate
   - :green_square: test crate with hyphenated name
   - :green_square: test module
   - :green_square: test nested module
   - :green_square: test single
   - :yellow_square: test adhoc selection (runs excess tests but outcome is ok - see note below)
 - :green_square: `@command:testing.runAll`
 - :green_square: `@command:testing.runAtCursor`
 - :red_square: `@command:test-explorer.run-all`
    - doesn't do anything
    
4. Note: the existing behaviour is to convert a list of tests to a common root. This can execute many more tests than expected (e.g. asking to run two tests in different packages will run everything in the whole workspace). This can be really bad if you also use unit tests for ui or integration level tests. I believe `cargo test` can be given multiple test paths so it should be possible to just pass the original list.





HKalbasi: > `@command:test-explorer.run-all`

This one is related to the third party extension so it shouldn't do anything, right?

The CI is red but it seems it is unrelated to your PR. Can you try to squash your commit to rerun CI?

Patch:
diff --git a/crates/flycheck/src/lib.rs b/crates/flycheck/src/lib.rs
index ce148037fc..3dd2a91d8f 100644
--- a/crates/flycheck/src/lib.rs
+++ b/crates/flycheck/src/lib.rs
@@ -24,7 +24,7 @@ pub mod project_json;
 mod test_runner;
 
 use command::{CommandHandle, ParseFromLine};
-pub use test_runner::{CargoTestHandle, CargoTestMessage, TestState};
+pub use test_runner::{CargoTestHandle, CargoTestMessage, TestState, TestTarget};
 
 #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
 pub enum InvocationStrategy {
diff --git a/crates/rust-analyzer/src/handlers/request.rs b/crates/rust-analyzer/src/handlers/request.rs
index 44ff273b5a..9bb3146322 100644
--- a/crates/rust-analyzer/src/handlers/request.rs
+++ b/crates/rust-analyzer/src/handlers/request.rs
@@ -27,7 +27,7 @@ use lsp_types::{
     SemanticTokensResult, SymbolInformation, SymbolTag, TextDocumentIdentifier, Url, WorkspaceEdit,
 };
 use paths::Utf8PathBuf;
-use project_model::{ManifestPath, ProjectWorkspaceKind, TargetKind};
+use project_model::{CargoWorkspace, ManifestPath, ProjectWorkspaceKind, TargetKind};
 use serde_json::json;
 use stdx::{format_to, never};
 use syntax::{algo, ast, AstNode, TextRange, TextSize};
@@ -199,6 +199,20 @@ 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.packages().find_map(|p| {
+        let package_name = &cargo[p].name;
+        if package_name.replace('-', "_") == namespace_root {
+            Some(package_name.clone())
+        } else {
+            None
+        }
+    })
+}
+
 pub(crate) fn handle_run_test(
     state: &mut GlobalState,
     params: lsp_ext::RunTestParams,
@@ -206,7 +220,7 @@ 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 ansector of all included tests, and
+    // 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 {
@@ -225,20 +239,31 @@ pub(crate) fn handle_run_test(
             .unwrap_or_default(),
         None => "".to_owned(),
     };
-    let test_path = if lca.is_empty() {
-        None
-    } else if let Some((_, path)) = lca.split_once("::") {
-        Some(path)
+    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 {
-        None
+        (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) {
+                    flycheck::TestTarget::Package(package_name)
+                } else {
+                    flycheck::TestTarget::Workspace
+                }
+            } else {
+                flycheck::TestTarget::Workspace
+            };
+
             let handle = flycheck::CargoTestHandle::new(
                 test_path,
                 state.config.cargo_test_options(),
                 cargo.workspace_root(),
+                test_target,
                 state.test_run_sender.clone(),
             )?;
             handles.push(handle);


Test patch:
diff --git a/crates/flycheck/src/test_runner.rs b/crates/flycheck/src/test_runner.rs
index c136dd1366..74ebca3410 100644
--- a/crates/flycheck/src/test_runner.rs
+++ b/crates/flycheck/src/test_runner.rs
@@ -59,19 +59,38 @@ pub struct CargoTestHandle {
 }
 
 // Example of a cargo test command:
-// cargo test --workspace --no-fail-fast -- module::func -Z unstable-options --format=json
+// 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
+
+#[derive(Debug)]
+pub enum TestTarget {
+    Workspace,
+    Package(String),
+}
 
 impl CargoTestHandle {
     pub fn new(
         path: Option<&str>,
         options: CargoOptions,
         root: &AbsPath,
+        test_target: TestTarget,
         sender: Sender<CargoTestMessage>,
     ) -> std::io::Result<Self> {
         let mut cmd = Command::new(Tool::Cargo.path());
         cmd.env("RUSTC_BOOTSTRAP", "1");
         cmd.arg("test");
-        cmd.arg("--workspace");
+
+        match &test_target {
+            TestTarget::Package(package) => {
+                cmd.arg("--package");
+                cmd.arg(package);
+            }
+            TestTarget::Workspace => {
+                cmd.arg("--workspace");
+            }
+        };
+
         // --no-fail-fast is needed to ensure that all requested tests will run
         cmd.arg("--no-fail-fast");
         cmd.arg("--manifest-path");