fix testing packages with multiple targets #19005

duncanawoods · 2025-01-22T21:51:22Z

This PR fixes #18955 and other bugs related to RA executing the wrong test and/or assigning results incorrectly.

It makes test execution aware of the difference between packages and targets.

It removes the need to use hack_recover_crate_name to match test output to tests.

request::handle_run_test
- no longer tries (and fails) to reduce a list of tests to a single pattern
test_runner::CargoTestHandle
- no longer runs patterns against the workspace and now always runs tests in the correct target
CargoActor
- is extended to take a parser that has the required context for parsing tool output correctly
main_loop
- every CargoTestMessage in the event queue now contains the required information to assign to correct test

HKalbasi · 2025-02-07T11:37:05Z

Ah sorry I forgot this. Now that the hack_recover_crate_name is gone, this actually reduces the amount of workarounds so I'm happy to merge this if you remove the merge commits.

duncanawoods · 2025-02-07T12:27:29Z

Oh that's great, thank you @HKalbasi !

Do you have any thoughts about adding some testing to this feature?

Having seen the dizzying number of permutations of how tests are defined and how they might be invoked I'm nervous about regressions. I'm thinking of an integration test using a sample workspace that tries to define every type of test and we might be able to simulate vscode interaction with "code --command" .

HKalbasi · 2025-02-07T16:06:28Z

If you want to add some end to end tests, look at https://github.com/rust-lang/rust-analyzer/blob/master/crates/rust-analyzer/tests/slow-tests/main.rs which creates a cargo project and communicate with rust-analyzer using lsp. But those tests slowdown CI so I would suggest having at most one test about test-explorer there (which can test many operations, most of the time is spent on the loading) and test other corner cases using unit tests that takes some hard coded json of cargo and/or assert text of the cargo commands instead of executing them.

Veykril · 2025-02-17T09:48:00Z

crates/project-model/src/cargo_workspace.rs

+                TargetKind::Example => "example",
+                TargetKind::Test => "test",
+                TargetKind::Bench => "bench",
+                TargetKind::BuildScript => "custom-build",


Suggested change

TargetKind::BuildScript => "custom-build",

TargetKind::BuildScript => "build-script",

The actual name cargo uses for this target kind is custom-build which is what needs to be used in commands and interpreting output.

https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.Target.html

I'm using the display trait for formatting cargo arguments (maybe this is wrong?) so this change would require a new function to_cargo_str or something. If your desire is to fix the code-smell of the display not matching the enum, a more correct change might be to change the enum name from BuildScript to CustomBuild.

(possibly a little moot for this bug fix since I'm not sure cargo test accepts tests in custom-build targets and I haven't tested it)

Let's make this a method (instead of Display) and give it a comment regarding what you explained to me (that is that these strings are significant to cargo).

Done.

Much clearer intention and can now return an option to make it clear that not all RA TargetKinds can be converted to a Cargo TargetKinds.

Veykril · 2025-02-17T09:49:53Z

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 {


Can we just discard this trait in favor of CargoParser? This code is already fairly convoluted and this isn't really helping that

I believe it can. Shall I make the change?

(I implemented this as an additional trait to minimise the level of change to unrelated code. I didn't know if there were particular motivations for the stateless parser and changing every Cargo invocation seemed like I would be over-presumptuous.)

I guess its fine for now

Done.

I went ahead to see what was affected and it was minimal and a decent simplification. If you don't want it, I'll back it out.

fix test running by invoking cargo per package remove hack_recover_crate_name make clippy happy fix testing for packages with multiple targets fix test running by invoking cargo per package remove hack_recover_crate_name make clippy happy fix testing for packages with multiple targets fix bad merge replace TargetKind::fmt with TargetKind::as_cargo_target to clarify intention dedupulicate requested test runs replace ParseFromLine with CargoParser formatting - remove trailing space formatting for rustfmt CI

Veykril · 2025-02-27T15:17:35Z

Looks good to me now, though I don't know anything about the test runner code so thats up to @HKalbasi to decide

duncanawoods · 2025-03-08T14:24:58Z

@HKalbasi good to go?

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2025

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from d1acee6 to 9e4161c Compare January 22, 2025 22:15

rustbot added has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2025

This comment has been minimized.

Sign in to view

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from adcd9cf to c756cb6 Compare February 7, 2025 12:16

rustbot removed has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 7, 2025

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from c756cb6 to 1a9da62 Compare February 7, 2025 12:17

HKalbasi self-assigned this Feb 10, 2025

Veykril reviewed Feb 17, 2025

View reviewed changes

rustbot added has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2025

This comment has been minimized.

Sign in to view

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from ef40ec7 to 07446d6 Compare February 19, 2025 13:27

rustbot removed has-merge-commits S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2025

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from 07446d6 to 8eff517 Compare February 19, 2025 13:31

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch 4 times, most recently from b6be8c6 to 5e2b50f Compare February 27, 2025 12:13

duncanawoods force-pushed the 18955---fix-running-tests-for-packages-with-multiple-targets branch from 5e2b50f to 2f5a7a6 Compare February 27, 2025 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix testing packages with multiple targets #19005

fix testing packages with multiple targets #19005

duncanawoods commented Jan 22, 2025 •

edited

Loading

This comment has been minimized.

HKalbasi commented Feb 7, 2025

duncanawoods commented Feb 7, 2025 •

edited

Loading

HKalbasi commented Feb 7, 2025

Veykril Feb 17, 2025

duncanawoods Feb 17, 2025 •

edited

Loading

Veykril Feb 24, 2025 •

edited

Loading

duncanawoods Feb 27, 2025

Veykril Feb 17, 2025

duncanawoods Feb 17, 2025

Veykril Feb 24, 2025

duncanawoods Feb 27, 2025

This comment has been minimized.

Veykril commented Feb 27, 2025

duncanawoods commented Mar 8, 2025

	TargetKind::BuildScript => "custom-build",
	TargetKind::BuildScript => "build-script",

fix testing packages with multiple targets #19005

Are you sure you want to change the base?

fix testing packages with multiple targets #19005

Conversation

duncanawoods commented Jan 22, 2025 • edited Loading

This comment has been minimized.

HKalbasi commented Feb 7, 2025

duncanawoods commented Feb 7, 2025 • edited Loading

HKalbasi commented Feb 7, 2025

Veykril Feb 17, 2025

Choose a reason for hiding this comment

duncanawoods Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Veykril Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

duncanawoods Feb 27, 2025

Choose a reason for hiding this comment

Veykril Feb 17, 2025

Choose a reason for hiding this comment

duncanawoods Feb 17, 2025

Choose a reason for hiding this comment

Veykril Feb 24, 2025

Choose a reason for hiding this comment

duncanawoods Feb 27, 2025

Choose a reason for hiding this comment

This comment has been minimized.

Veykril commented Feb 27, 2025

duncanawoods commented Mar 8, 2025

duncanawoods commented Jan 22, 2025 •

edited

Loading

duncanawoods commented Feb 7, 2025 •

edited

Loading

duncanawoods Feb 17, 2025 •

edited

Loading

Veykril Feb 24, 2025 •

edited

Loading