VSCode Test Explorer - bugs running tests for packages with multiple targets

[duncanawoods]

There are a number of bugs running tests for packages that define multiple targets:

Running tests for a target can run unrelated tests in other packages / targets.

RA tries to run a test from a fully qualified name like:

root::module::test_name

a) RA assumes the root is a package name but it can be the name of a target inside a package.

b) RA does not supply cargo test with target parameters e.g. --bin name to run in the correct test

c) if test code is shared between multiple targets:

• RA will be asked to run a test for each target
• RA will try to combine the name of all tests into a single pattern with unpredictable results

e.g. if there are targets named "target1", "target2", "target3", RA will run every test in the workspace containing the word "target".

The results of a test are often assigned against the wrong target or crate

This is because the cargo test json output does not contain the package or target of the test it has run. RA assigns the result using hack_recover_crate_name which guesses at an answer.

[duncanawoods]

Suggested fixes

1. Running Tests

1 a) and 1 b) can be solved by extending test_runner::TestTarget and request::find_package_name to find a matching target not just the package.

c) When invoking a test from source code shared by multiple targets, there are two main options:

i) run the test for all the targets => this is what vscode is trying to do right now and would be fixed by replacing RA's effort to combine tests into a single path and running each requested test with separate invocations of cargo test.

ii) run the test for one target only => when developing a test, it's often undesirable to run it for every target. It's likely slow and redundant e.g. a common reason for multiple targets is a library with a cli wrapper. Identifying a default target to use would likely match the developer's intention.

2. Test Results

The current RA design tries to parse cargo command output without the context of the original command. This is insufficient to match test output to targets. The required fix is to extend the cargo command running infrastructure to make command context available when the output is being interpreted.

[duncanawoods]

Background Info

• the default target in a package can be a lib or a bin
• there can be multiple bin targets but only one lib
• tests can be specific to a target or in shared code used by multiple targets

I am testing against a workspace with two members that define additional targets:

• test_bin
  • bin target
  • contains
    • test_bin_lib : a lib target
    • test_bin2 : am extra bin target
• test_lib
  • lib target
  • contains
    • test_lib_bin1 : a bin target
    • test_lib_bin2 : a second bin target

Each with shared and target specific tests.

[duncanawoods]

@HKalbasi

[HKalbasi]

@duncanawoods Thanks for the write up.

This is because the cargo test json output does not contain the package or target of the test it has run.

I think the way to go is to make cargo test include the target name in the json, and we shouldn't extend our hacks to support cargo's incomplete output.

Last time I checked, people are working on cargo test json output, but cargo is slow to update, for example each PR needs 12 weeks to reach stable.

Another promising project is cargo nextest. It seems they already have a non broken json output, and even if we need more data, I guess they are willing to add (merge PRs) quickly. @duncanawoods are you willing to add support for running cargo nextest in addition of cargo test in the test explorer?

[duncanawoods]

Hi @HKalbasi

I think the way to go is to make cargo test include the target name in the json

I'm even more pessimistic about cargo given the json output feature has been unstable for 7 years, I don't think it's changed much since then and I don't fancy our chances getting a breaking format change incorporated.

nextest

Yeah, I have taken a look at it. Most of it's features benefit CI (test-per-process, performance, retries, timeouts, build archives) or the terminal (can connect an interactive session to interrogate status).

I don't see too much it can offer RA. It's coverage and miri integrations intrigue me and might be a useful path for RA to support "test with coverage" and "test with leak detection" type features.

It has two unstable json formats (libtest-json, libtest-json-plus) and both contain the target name. I agree the prospect of getting PRs adopted feels brighter! I'll take a look at integrating it.

we shouldn't extend our hacks to support cargo's incomplete output.

I believe the change to CargoActor (or an equivalent) is an appropriate bug fix and likely necessary beyond this issue.

It's unusual for RA to not know anything about what triggered a cargo command when it handles it's output. I don't think it's sustainable for a user interface and would expect future scenarios e.g. error reporting or needing to know which UI element triggered an action so that appropriate visual updates can be made. Retaining information about the source of an async command in order to handle it's progress/results is just a basic building-block to me.

If it appears as a hack, are there some adjustments that would make it look more intentional?

(note, the fixes for identifying and running tests in the correct target are unrelated to cargo output).