Test Explorer Support via VSTest #1383
Conversation
| vstest.CancelTestRun() | ||
| printfn "Test Run Cancelled") | ||
|
|
||
| vstest.RunTests(sources, null, options, runHandler) |
There was a problem hiding this comment.
@nohwnd is the best to help with VSTest stuff. But, I see there is a RunTestsWithCustomTestHost API on VsTestConsoleWrapper. From there, you have LaunchTestHost method with TestProcessStartInfo parameter, so that you can start the process yourself and then you know its id. That should be more reliable than parsing process id from the logs, which you stated doesn't work with older versions of VSTest.
There was a problem hiding this comment.
https://github.com/microsoft/vstest/blob/main/playground/TestPlatform.Playground/Program.cs#L116 this is how you'd do it. The playground project in VSTest is also the best way to experiment with this kind of stuff. If you build, you will have attachVS tool available that we build in the same repo, this tool will automatically attach your VisualStudio to the processes that you choose. E.g. setting VSTEST_RUNNER_DEBUG_ATTACHVS to 1, in this file https://github.com/microsoft/vstest/blob/main/playground/TestPlatform.Playground/Environment.cs#L14 will attach vs to vstest.console process automatically so you can debug both the client and the server.
In vstest.console you are interested in DesignMode client, that is where the messages from vstestconsolewrapper are processed.
For debugging there are 2 possible flows, launch with debugger attached (this would happen before testhost starts), and attach debugger (this is driven from the testhost after it starts). VS is using AttachDebugger, which is more modern. But it depends on you what you use.
https://github.com/microsoft/vstest/blob/main/docs/Overview.md here is documentation of the protocol, if you search there for "debugger" you will hopefully find the info you need, and the description of the flow, and the versions of interfaces that support it.
But honestly this is not something that people do every day, so that protocol description might be lacking all the info you need to successfully implement this. Feel free to ask more questions. :)
There was a problem hiding this comment.
https://github.com/microsoft/vstest/blob/main/playground/TestPlatform.Playground/Program.cs#L252 there you probably want to implement ITestHostLauncher3, that also gives you info about tfm, and the dlls you will debug.
This is interesting for multi target framework runs, where the debugger might differ between .net framework and .net.
There was a problem hiding this comment.
Thanks for the help!
ITestHostLauncher3 was the solution to my debugging challenges.
RunTestsWithCustomTestHost only has overloads taking ITestHostLauncher, so I overlooked ITestHostLauncher2 and ITestHostLauncher3. Poking around the TestPlatform code, it looks like there's some casting going on.
There was a problem hiding this comment.
Also, the TestPlatform playground has been indispensable as I've tried to learn how to work with VSTest.
I'm grateful to the team for open sourcing it!
I extracted a standalone copy of the TestPlatform Playground. That's where I run most of my experiments
There was a problem hiding this comment.
Sweet! The playground and attachvs is my work and I find it super useful almost every day as well, glad to see I am not the only one 👍
Yeah that api is not great, we do awful lot to avoid breaking public api and break users. And also don't want to add more and more copies of the public api with all the combinations of possible new interfaces. So keeping the old interface in the api, and then adapting it internally to the newest version, if needed, is the way that multiple things are implemented. But it leads to making it difficult to discover.
|
For NUnit, this is more likely to be an NUnit bug. I see strange filtering logic here. When running with VsTestConsoleWrapper, DesignMode will be true, and that's likely why this is different from I think what might be worth trying is to embed the filter in runsettings, instead of TestPlatformOptions. Meanwhile, I think it's worth reporting this bug to NUnit. |
|
@Youssef1313 I agree the filter issue is probably something to report to NUnit. I tried your idea to set the filter in runsettings, but it didn't seem to change the behavior. |
There are two things you can try with runsettings. First is forcing DesignMode to be false. Second is passing the filter via TestCaseFilter. If passing via TestCaseFilter doesn't work too, then that's yet another NUnit bug. |
|
MSTest handles that here: xUnit.net handles that here: @nohwnd btw, why do frameworks need to do reflection here? Is there no public API that exposes the needed info to adapters? |
|
Setting This tag doesn't appear in the official documentation, do you think it'd be risky to use? |
|
@nohwnd is the best to answer here. |
NUnit doesn't respect test filters when VSTest is in DesignMode, which it is by default with VsTestConsoleWrapper ionide#1383 (comment)
|
I'm looking to add .runsettings support to the Ionide test explorer. It appears Is there another way to support runsettings that accounts for the runsettings file location? |
|
@nohwnd Do you have some ideas on the .runsettings challenges with VSTestConsoleWrapper |
This is preparation for adding notifications for incremental discovery reporting
VSTestAdapter is too specific. We already know we'll need to expand to accomodate Microsoft.Testing.Platform
Still has a bug where NUnit doesn't respect the filter, but works for all other frameworks
Also required figuring out graceful cancellation behavior for test runs
RunTestsWithCustomTestHost is the official way to attach a debugger with VSTest
Broke out the debug attach message because it needs to be a blocking call. VSTest will continue executing the tests as soon as AttachDebuggerToProcess returns I also fixed the LSP level test explorer tests, which were failing because the sample test projects needed to be built at part of the unit test, since the build artifacts are being cleared before every test run
FsAutoComplete.Tests.TestExplorer expects the sample projects to be built once before the tests are run, but FsAutoComplete.Tests.Lsp expects the sample projects to be build for every test. Splitting up the sample projects reduces chances that they could break each other when the run in parallel or in random order
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
NUnit doesn't respect test filters when VSTest is in DesignMode, which it is by default with VsTestConsoleWrapper ionide#1383 (comment)
…s (i.e projects not restored or built)
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the test explorer functionality from relying on dotnet test and TRX files to using the VSTest platform directly via the Microsoft.TestPlatform.TranslationLayer. This change moves test discovery and execution from the Ionide client to the FsAutoComplete language server, enabling standardized test functionality across all FSAC clients while improving performance and compatibility with Microsoft.Testing.Platform projects.
Key Changes
- Introduces VSTest-based test discovery and execution infrastructure in the language server
- Adds comprehensive test suite for test explorer functionality
- Migrates test functionality from client-side to server-side implementation
Reviewed Changes
Copilot reviewed 42 out of 46 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/FsAutoComplete.Tests.TestExplorer/ | New comprehensive test project for VSTest wrapper functionality with sample projects |
| test/FsAutoComplete.Tests.Lsp/TestExplorerTests.fs | New LSP-level integration tests for test discovery and execution |
| test/FsAutoComplete.Tests.Lsp/DotnetCli.fs | New helper module for executing dotnet CLI commands in tests |
| src/FsAutoComplete/LspServers/ | Updates to LSP server interfaces and implementations to support test endpoints |
| src/FsAutoComplete.Core/VSTestWrapper.fs | New VSTest platform wrapper providing test discovery and execution APIs |
| src/FsAutoComplete.Core/TestServer.fs | New data models and conversion utilities for test items and results |
| paket.dependencies & project files | Added dependencies for Microsoft.TestPlatform packages |
| // NOTE: This is an NUnit bug. NUnit doesn't respect filters when VSTest is in Design Mode, which VsTestConsoleWrapper is by default | ||
| // https://github.com/ionide/FsAutoComplete/pull/1383#issuecomment-3245590606 |
There was a problem hiding this comment.
[nitpick] This comment explains a workaround for an external library limitation but could be more actionable. Consider documenting what the expected behavior should be when this bug is fixed.
| // NOTE: This is an NUnit bug. NUnit doesn't respect filters when VSTest is in Design Mode, which VsTestConsoleWrapper is by default | |
| // https://github.com/ionide/FsAutoComplete/pull/1383#issuecomment-3245590606 | |
| // NOTE: This is an NUnit bug. NUnit doesn't respect filters when VSTest is in Design Mode, which VsTestConsoleWrapper is by default. | |
| // https://github.com/ionide/FsAutoComplete/pull/1383#issuecomment-3245590606 | |
| // EXPECTED BEHAVIOR WHEN BUG IS FIXED: When NUnit respects filters in Design Mode, this test should only run and report results for the filtered test(s). | |
| // At that point, update or remove this workaround and ensure the test expectations match the correct filtered output. |
| Level = string level } |] } | ||
|
|
||
|
|
||
| lspClient.NotifyTestDiscoveryUpdate(dto) |> Async.RunSynchronously |
There was a problem hiding this comment.
Using Async.RunSynchronously within an async context can lead to deadlocks and blocks the thread pool. Consider using 'do!' to properly await the async operation instead.
| TestResults = [||] | ||
| ActiveTests = [||] } | ||
|
|
||
| Async.RunSynchronously(async { do! lspClient.NotifyTestRunUpdate(dto) }, cancellationToken = tokenSource.Token) |
There was a problem hiding this comment.
Using Async.RunSynchronously within an async context can cause deadlocks and blocks threads. Consider restructuring to use 'do!' for proper async composition.
| Async.RunSynchronously(async { do! lspClient.NotifyTestRunUpdate(dto) }, cancellationToken = tokenSource.Token) | |
| do! lspClient.NotifyTestRunUpdate(dto) |
| let onAttachDebugger (processId: int) : bool = | ||
| let result = | ||
| Async.RunSynchronously(lspClient.AttachDebuggerForTestRun(processId), cancellationToken = tokenSource.Token) | ||
|
|
||
| match result with | ||
| | Ok didAttach -> didAttach | ||
| | Error err -> | ||
| logger.warn ( | ||
| Log.setMessageI | ||
| $"Failed to attach debugger for test run with Process Id: {processId}; Error Code: {err.Code}; Error message: {err.Message}" | ||
| ) | ||
|
|
||
| false | ||
|
|
There was a problem hiding this comment.
Using Async.RunSynchronously can cause deadlocks when called from async contexts. Consider making the onAttachDebugger function async and using 'let!' instead.
| let onAttachDebugger (processId: int) : bool = | |
| let result = | |
| Async.RunSynchronously(lspClient.AttachDebuggerForTestRun(processId), cancellationToken = tokenSource.Token) | |
| match result with | |
| | Ok didAttach -> didAttach | |
| | Error err -> | |
| logger.warn ( | |
| Log.setMessageI | |
| $"Failed to attach debugger for test run with Process Id: {processId}; Error Code: {err.Code}; Error message: {err.Message}" | |
| ) | |
| false | |
| let onAttachDebugger (processId: int) : Async<bool> = | |
| async { | |
| let! result = lspClient.AttachDebuggerForTestRun(processId) | |
| match result with | |
| | Ok didAttach -> return didAttach | |
| | Error err -> | |
| logger.warn ( | |
| Log.setMessageI | |
| $"Failed to attach debugger for test run with Process Id: {processId}; Error Code: {err.Code}; Error message: {err.Message}" | |
| ) | |
| return false | |
| } |
TheAngryByrd
left a comment
TheAngryByrd
left a comment
There was a problem hiding this comment.
Couple of minor changes. This is looking great!
|
|
||
| override this.TestDiscoverTests() : Async<LspResult<PlainNotification option>> = | ||
| asyncResult { | ||
| let! testDTOs = |
There was a problem hiding this comment.
Do add OTel/Logging like other methods in this class.
| let tryGetWorkspaceProjects (workspaceLoader: IWorkspaceLoader) (workspace: WorkspaceChosen) = | ||
| match workspace with | ||
| | WorkspaceChosen.NotChosen -> Error "No workspace loaded. Can't discover tests" | ||
| | WorkspaceChosen.Projs projectPaths -> | ||
| projectPaths | ||
| |> List.ofSeq | ||
| |> List.map string | ||
| |> workspaceLoader.LoadProjects | ||
| |> Ok |
There was a problem hiding this comment.
| let tryGetWorkspaceProjects (workspaceLoader: IWorkspaceLoader) (workspace: WorkspaceChosen) = | |
| match workspace with | |
| | WorkspaceChosen.NotChosen -> Error "No workspace loaded. Can't discover tests" | |
| | WorkspaceChosen.Projs projectPaths -> | |
| projectPaths | |
| |> List.ofSeq | |
| |> List.map string | |
| |> workspaceLoader.LoadProjects | |
| |> Ok |
Instead of loading projects again, you'll want to reuse the currently loaded projects as this will help with any project related changes like adding files, adding packages etc.
You can do something like:
let! projects = projectOptions |> AsyncAval.forceAsync
let! testProjects = projectOptions.ToValueSeq() |> TestProjectHelpers.tryGetTestProjectsIn DiscoverTests/RunTests
| this.DiscoveredTests.AddRange(lastChunk) | ||
| notifyDiscoveryProgress (lastChunk |> List.ofSeq |> Progress) | ||
|
|
||
| member this.HandleLogMessage(level: TestMessageLevel, message: string) : unit = |
There was a problem hiding this comment.
Is it worth logging the message on the server side as well?
|
I looked into the test failures. It appears that a random group of tests in AdjustConstantTests semi-regularly fails. The error is always thrown from I assume this timeout is why random tests in the group are intermittently failing. |
Yeah, we technically could use PullDiagnostics but I'd fear we would possibly regress on the push diagnostics part. This has definitely been a thing that's been noisy for years tho. |
5 participants
Test Explorer VSTest Migration
NOTE: It looks like a lot of changed files, but many of them are sample test projects or .fsi files
Motivation
dotnet testparameter we useBefore 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.
Test functionality is also built on VSTestConsoleWrapper instead of
dotnet test.This comes with several advantages
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 also 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.
Old draft PR text
Short Version: I'm unsure how to properly debug tests using Microsoft.TestPlatform.TranslationLayer. All the interactions with vstest are in this file.
Existing Behavior
Ionide is the F# tooling for VSCode. Ionide's test explorer currently works off of
dotnet testwith the TRX logger.This approach will no longer work with the transition to Microsoft.Testing.Platform.
Proposed Change
This PR moves Ionide's test platform integration to the language server and replaces
dotnet testand trx files with Microsoft.TestPlatform.TranlationLayer.This should make Ionide more consistent with test explorers in other IDEs and hopefully sets us up for future work on dual vstest/MTP support.
So far I've got test discovering, running, and incremental updates streaming to the client.
Current Challenges
Debugging tests
I'm stuck on running tests with the debugger attached.
I got a demonstration working by parsing the processId from the log output.
But, it appears that doesn't work with vstest versions 17.10.0 and earlier.
I also looked at
ITestRunEventsHandler.LaunchProcessWithDebuggerAttachedandITestHostLauncher/RunTestsWithCustomTestHost, but I was not able to demonstrate any of the custom launch methods being called. I may be missing something hiding in plain sight.NUnit doesn't respect filters
I'm using this project as a sample environment for testing both the old and new Ionide test explorer.
Filtering to a specific NUnit test using
dotnet test --filter "expression-here"only logs results for the expected test.However, TranslationLayer returns TestResults for all of the NUnit tests given the same filter expression.
TranslationLayer respects the filter expression for every other test library.