Skip to content

Conversation

farlee2121
Copy link
Contributor

@farlee2121 farlee2121 commented Sep 15, 2025

Test Explorer VSTest Migration

Depends on FSAutoComplete PR 1383

Motivation

Before this PR

The Ionide test explorer run on dotnet test and TRX files.
Test discovery and test execution are implemented directly in Ionide.

After this PR

Test discovery and execution are moved to the language server, allowing them to be used across clients.
Users can revert to the dotnet test-based approach with the setting FSharp.TestExplorer.UseLegacyDotnetCliIntegration

When using the LSP discovery

  • we now support Microsoft.Testing.Platform VSTestBridge projects
  • We have access to test code locations provided by the test libraries themselves
  • We don't have to run tests to discover them, which makes test discovery much faster
  • We don't need to create temporary artifacts (i.e. TRX files)
  • VSTest manages test execution across assemblies, reducing our responsibility for managing resource utilization

We will not yet support Microsoft Testing Platform projects that don't use VSTestBridge.

Outstanding issues

  • I tried to support .runsettings (ionide 2093 and 2040), but there are outstanding questions on how to support it via VSTestConsoleWrapper.

  • Microsoft Testing Platform VSTestBridge projects report their test locations as the end of a test's code block.
    There's not really a way for us to fix this.

  • Additional work is needed to support pure Microsoft Testing Platform projects.

  • We'll probably want to remove the dotnet test-based code once we're confident the LSP-based holds up to user expectations

  • There is some duplication of logic between Ionide and FSAutoComplete, specifically around deciding what projects are test projects

I also considered moving more logic to the language server for better testing, but I decided to hold off.
The current progress is useful, and supporting Microsoft.Testing.Platform is likely to add new constraints to the design.

…used

Also refactor toward simpler swappability of the two approaches
Does not currently work for partial test runs.
Also doesn't work if there are MSTest theory tests due to discrepancy between test discovery and test results
Still has a bug where NUnit doesn't respect the filter, but works for all other frameworks
…d of waiting and simulating a user continue command
This was mainly motivated by test debugging.

Before this feature, the debugger would launch for every test project even if you only wanted to debug one test.
With this change, it'll only launch the one project.

This also partially mitigates the bug where NUnit doesn't respect filters, now those test won't show unless some tests from an nunit project are selected
…-level handlers

Also enables experimentation with running discovery before a test run to make the explorer more robust for projects without live update support
…sults

The dotnet cli approach fundamentally expected tests to be run and merged by project, but VSTest doesn't.
MergeTestResultsToExplorer really only needed the project and target framework for creating new items.
By adding project and target framework to the each  test result, we can merge test items without expecting them to be batched by project
Test projects without code analysis updates (i.e C# projects or projects using Microsoft.Testing.Platform.VSTestBridge)
will show new tests if you run a group of tests that changed, but those new tests will not have code locations.

The lack of code locations in this limited case is very confusing and degrades the experience significantly.

I looked at several options to fill in these locations.
- VSTest does not return test locations on test runs, so mapping a location from results doesn't work
- Running discovery before a test run caused test runs to feel sluggish. Especially when trying to run individual tests, which is a core workflow. I tried project-specific discovery and parallel discovery/test run, but still had an average ~1s delay.

However, test discovery, even with thousands of tests, only takes a few seconds. That feels sluggish before the test run, but is inconsequential after the test run. The discovery is generally finished before the user has evaluated the test results.

This pattern also has some precedent. Visual Studio will generally discover new tests in a group only after you run the group.
Thanks for coming to my ted talk.
else
do! runTests_WithLanguageServer mergeTestResultsToExplorer testController.items req testRun
testRun.``end`` ()
do! discoverTests_WithLangaugeServer testItemFactory testController.items tryGetLocation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test runs can add test to the explorer. but the test run will not provide a code location.
This makes for a confusing and bad experience.

I tried a number of alternatives, and this one felt the best.
Running after the test run avoids the test run feeling sluggish, and the discovery still runs fast enough (even with thousands of tests) that it's generally done by the time a dev is ready for a new action.

More information

A key alternative here would be running test discovery whenever a the test project build artifact is updated.
I think that's more in line with what Visual Studio does, but was a bigger deviation from our existing workflow

showStarted progress.ActiveTests
mergeResults TrimMissing.NoTrim progress.TestResults

let appendTestResultToOutput (testResult: TestResultDTO) =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found printing all the test results to the test output was helpful feedback. I felt more confident that the test run went as expected or could see any error messages.

I could see an argument that printing all the test results feels noisy for big test runs.

We currently store the test's FullyQualifiedName in the id and separate it from other id components with --

This led to incorrectly splitting the test name if it contained a --

It may be a good idea to dynamically add a field to TestItem for FullyQualifiedName similar to what was donen for TestFramework
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the test explorer to use FSAutoComplete and VSTest for test discovery and execution, moving away from the current dotnet CLI-based approach. This enables standardized test code locations, improved performance, and better integration with VSTest ecosystem features.

Key changes:

  • Added LSP-based test discovery and execution methods alongside existing dotnet CLI approach
  • Implemented new DTO types for test items, results, and progress updates
  • Added configuration option to toggle between legacy dotnet CLI integration and new LSP approach

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/Core/LanguageService.fs Added LSP methods for test discovery and execution with progress notifications
src/Core/DTO.fs Added comprehensive DTOs for test items, results, and communication with language server
src/Components/TestExplorer.fs Implemented dual-mode test explorer supporting both LSP and CLI approaches
release/package.json Added configuration setting to control test integration method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@TheAngryByrd
Copy link
Member

i'm going to test over the next few days. I'll leave feedback as I go.

When I ran a test a second time against IcedTasks I started getting an error:

[20:21:44 DEBUG] [TestExplorer] TestRunRequest hp {
  include: [
    MP {
      id: 'c:\\Users\\jimmy\\Repositories\\public\\TheAngryByrd\\IcedTasks2\\tests\\IcedTasks.Tests.NS21\\IcedTasks.Tests.NS21.fsproj -- IcedTasks.AsyncEx.AsyncExBuilder.Return',
      uri: [Ia],
      children: [Object],
      range: [Getter/Setter],
      label: [Getter/Setter],
      description: [Getter/Setter],
      sortText: [Getter/Setter],
      canResolveChildren: [Getter/Setter],
      busy: [Getter/Setter],
      error: [Getter/Setter],
      tags: [Getter/Setter]
    },
    MP {
      id: 'c:\\Users\\jimmy\\Repositories\\public\\TheAngryByrd\\IcedTasks2\\tests\\IcedTasks.Tests.NS20\\IcedTasks.Tests.NS20.fsproj -- IcedTasks.AsyncEx.AsyncExBuilder.Return',
      uri: [Ia],
      children: [Object],
      range: [Getter/Setter],
      label: [Getter/Setter],
      description: [Getter/Setter],
      sortText: [Getter/Setter],
      canResolveChildren: [Getter/Setter],
      busy: [Getter/Setter],
      error: [Getter/Setter],
      tags: [Getter/Setter]
    },
    MP {
      id: 'c:\\Users\\jimmy\\Repositories\\public\\TheAngryByrd\\IcedTasks2\\tests\\IcedTasks.Tests\\IcedTasks.Tests.fsproj -- IcedTasks.AsyncEx.AsyncExBuilder.Return',
      uri: [Ia],
      children: [Object],
      range: [Getter/Setter],
      label: [Getter/Setter],
      description: [Getter/Setter],
      sortText: [Getter/Setter],
      canResolveChildren: [Getter/Setter],
      busy: [Getter/Setter],
      error: [Getter/Setter],
      tags: [Getter/Setter]
    }
  ],
  exclude: [],
  profile: b7 {
    controllerId: 'fsharp-test-controller',
    profileId: -1169225476,
    kind: 1,
    g: 'Run F# Tests',
    runHandler: [Function: runHandler],
    _tag: undefined,
    j: false
  },
  continuous: false,
  preserveFocus: true
}
[20:22:02 DEBUG] [TestExplorer] Test Filter Expression: (FullyQualifiedName~IcedTasks.AsyncEx.AsyncExBuilder.Return)|(FullyQualifiedName~IcedTasks.AsyncEx.AsyncExBuilder.Return)|(FullyQualifiedName~IcedTasks.AsyncEx.AsyncExBuilder.Return)
[20:22:05 DEBUG] [TestExplorer] Test run failed with exception Error: An item with the same key has already been added. Key: IcedTasks.AsyncEx.AsyncExBuilder.Return.Simple return
    at Dictionary__Add_5BDDA1 (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\fable_modules\fable-library.4.2.2\MutableMap.js:273:1)
    at new Dictionary (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\fable_modules\fable-library.4.2.2\MutableMap.js:19:1)
    at ArrayExt_venn (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\Components\TestExplorer.js:43:37)
    at Interactions_mergeTestResultsToExplorer (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\Components\TestExplorer.js:1430:1)
    at c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\Components\TestExplorer.js:1731:1
    at f2 (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\fable_modules\fable-library.4.2.2\Util.js:572:1)
    at mergeResults (c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\Components\TestExplorer.js:1624:1)
    at c:\Users\jimmy\Repositories\public\TheAngryByrd\ionide-vscode-fsharp\release\webpack:\out\Components\TestExplorer.js:1700:1

@TheAngryByrd
Copy link
Member

Oh hell yes, being able to show multiple projects. ❤️

image

@TheAngryByrd
Copy link
Member

Thinking about that error. it's probably because I do have the same tests across different projects/tfms.

Copy link
Member

@TheAngryByrd TheAngryByrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good over! Just some nits and questions.


warn
$"No tests discovered for the following projects. Make sure your tests can be run with `dotnet test` \n {projectList}"
do! discoverTests_WithLanguageServer testItemFactory rootTestCollection tryGetLocation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking. Do we still need to build up front doing test discovery this way? I am slightly concerned we're building too often. This could hurt for large projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of. I decided to have the full refresh still build the projects, because I'd be confused if I clicked refresh and my new tests didn't show up.

We could look at discovering tests without building when the project is initially loaded.

Eventually I'd like to move to tests being discovered whenever the build artifacts change.
That'd be more inline with the visual studio experience and narrows the gap in experience between projects that have code analysis-based update and those that don't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds like a good follow-up issue for this. Could you write it up so either future us or others could contribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to discover tests without building when first loading the workspace.
It'll still build when the refresh button is clicked or when they run tests.

It feels much more responsive. The only potential loss is that the initial test list won't be up to date if the code has been changed since the last build. But the code will build and the explorer will update when tests are run.

To clarify, you want me to make an issue for watching build artifacts and responsively discovering tests?

| Skipped

module TestResultOutcome =
let ofOutcomeDto (outcomeDto: TestOutcomeDTO) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting a warning here about:

Enums may take values outside known cases. For example, the value 'enum<TestOutcomeDTO> (5)' may indicate a case not covered by the pattern(s)

Copy link
Contributor Author

@farlee2121 farlee2121 Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to throw an error on an expected value.
My reasoning was that the only way we should have an unexpected error is if the language server changes.
In which case we'd want to know right away in development that we failed to sync the data contracts.

The downside, if a new test outcome value slips through it creates a more disruptive experience for effected users (though probably also a less confusing one than it mapping to another test outcome) .

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we control both sides of that DU I think it makes sense to fail early.

option |> Option.iter f
option

let tryFallback f opt =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be able to use Option.orElseWith instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a handy core function to know about.

| Some _ -> opt
| None -> f ()

let tryFallbackValue fallbackOpt opt =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you might be able to use Option.orElse instead of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Good call

tryFallback -> Option.orElseWith
tryFallbackValue -> Option.orElse
This should not happen. If it does, it means ionide and the language server are out of sync
@farlee2121
Copy link
Contributor Author

Thinking about that error. it's probably because I do have the same tests across different projects/tfms.

The regression testing repository I use has a project with multiple target frameworks, and it behaves as expected.

I did confirm that IcedTasks throws this error when I try to run all tests, but not when I try to run the projects one at a time. I'll need to dig deeper.

If they've never built their solution, then we won't be able to load tests until they restore anyway.

If they have built their solution, we probably don't need to build it again. It's much faster to discover tests without the build and, unlike clicking the refresh button, there is a much smaller chance they're looking for missing test or otherwise solving a weird explorer state where we'd want fresh build artifacts.
IcedTasks experienced this issue when all tests were run.
Merging tests results used to be per-project, but now is not. So the test result venn requires project scope data
@farlee2121
Copy link
Contributor Author

@TheAngryByrd I fixed the conflicting test name issue you encountered in IcedTasks and added a replication of the cause to my regression testing repo

@TheAngryByrd
Copy link
Member

Thanks for all the hard work on this!

@TheAngryByrd TheAngryByrd merged commit 252c795 into ionide:main Sep 27, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants