#17470 - run unit tests at the crate level not workspace

[duncanawoods]

For #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 #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 longer than the lifetime of the terminal (stored on disk)
• 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 🍑
  • 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 so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-analyzer.git master
$ git push --force-with-lease

The following commits are merge commits:

• 837de78

[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:

• Lens: request:::handle_code_lens -> toproto::code_lens -> toproto::runnable -> CargoTargetSpec
• Explorer: TestController -> request::handle_run_test -> flycheck::CargoTestHandle

Unification to take the best from each would look something like:

• Lens: request:::handle_code_lens -> toproto::code_lens -> executeCommand('testing.run.uri')
• Explorer: 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

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
  • 🟩 test all
  • 🟩 test crate
  • 🟩 test crate with hyphenated name
  • 🟩 test module
  • 🟩 test nested module
  • 🟩 test single
  • 🟨 test adhoc selection (runs excess tests but outcome is ok - see note below)
• 🟩 @command:testing.runAll
• 🟩 @command:testing.runAtCursor
• 🟥 @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?

[duncanawoods]

This one is related to the third party extension so it shouldn't do anything, right?

I believe so. I don't think it's a regression but since I tried it, thought it worth mentioning. I'm surprised it doesn't do anything.

The CI is red but it seems it is unrelated to your PR. Can you try to squash your commit to rerun CI?

Done. Looks like the CI has sorted itself. Strange error.

[HKalbasi]

Thanks!
@bors r+

[bors]

📌 Commit 20d3237 has been approved by HKalbasi

It is now in the queue for this repository.

[bors]

⌛ Testing commit 20d3237 with merge 3cf17dc...

[bors]

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 3cf17dc to master...

[duncanawoods]

Thanks for your time and patience @HKalbasi!