Test tree pollution when renaming a test

This does include a bug report, but the main purpose is to get feedback on a modified approach to building the test tree (see Proposed Solution).

Describe the bug

2023-04-27-ionide-test-pollution-on-rename

When renaming a test, the original name and intermediate name values show up in the explorer and won't go away until VSCode is restarted.

Steps to reproduce

Still trying to pin it down, but I'm running into several problems that could share a solution and probably make the specific cause here irrelevant anyway.

Possibly related issues

PROBLEM: Deleted tests don't go away until the IDE is restarted

PROBLEM: Run all never completes

I know this can be solved by changing TestExplorer line 178 from TimeSpan.Parse(duration).TotalMilliseconds to the following code block, but I think the root cause is broader.

  let success, ts = TimeSpan.TryParse(duration)
  if success then ts.TotalMilliseconds else Unchecked.defaultof<float>

PROBLEM: Reused tests incorrectly show at the top of the test list and not in the correct places in the test hierarchy. Here's an example of how I reuse tests

let minTestsForType<'a when 'a :> IComparable<'a> and 'a : equality> rangeGen = 
    testList $"Min {typeof<'a>.Name}" [
        testProperty "Min inclusive" <| fun (i:'a) ->
            Spec.isValid (Spec.min i) i

        testProperty "Any value greater than or equal to min is valid" <| fun (min: 'a) ->
            let arb = rangeGen (Some min, Option.None) |> Arb.fromGen
            Prop.forAll arb <| fun (i:'a) ->
                Spec.isValid (Spec.min min) i

        testProperty "Any value less than min is invalid" <| fun (min: 'a) ->
            let arb = rangeGen (Option.None, Some min) |> Arb.fromGen
            Prop.forAll arb <| fun (i:'a) ->
                min <> i ==> lazy(
                    not (Spec.isValid (Spec.min min) i)
                )
    ] 

testList "Min" [
        minTestsForType<int> Gen.intRange
        minTestsForType<DateTime> Gen.dateTimeRange
        minTestsForType<NormalFloat> Gen.normalDoubleRange
    ]

I realize this is an unusual usecase.

Proposed solution

I can elaborate on the problems or split them out if desired, but I have a solution I think will solve all of them and a few other use cases. My main goal with this issue is to get feedback before implementing, since the change could be big.

As I understand it, the test tree is currently built off of tests discovered by the language server and ultimately handed to TestExplorer off via Notifications.testDetected on a per-file basis. Tests detected by the language server are the source of truth.

My proposal is to make the trx file the source of truth, then use a lookup of language server-discovered tests to fill in location data.

Pros and Cons

PRO: The trx represents a full picture of the tests as the runner sees them
- PRO: This makes it easier to keep the test list up to date with both additions and removals since we have a full picture
- PRO: We can show tests from any framework with dotnet cli integration
- This includes tests written in other .NET languages
- CON: not all tests will have source locations and the features tied to location
- PRO: Can always run tests to refresh the test list. Can get out of weird states reliably without restarting the IDE
PRO?: We should be able to reload the explorer state from the trx without parsing the code. Might be faster
- CON: First-time discovery depends on building and running test, which may be slower
CON: This is a significant change from the existing approach

We could still use language server notifications of update test names without rerunning the tests.

I'll try implementing this approach if the team gives it a thumbs up.

Machine info

OS: Windows
.NET SDK version: 7.0.201
Ionide version: v7.5.3

ionide / ionide-vscode-fsharp