Add RunTestsInContext Command #1782
Conversation
This command tries to replicate the ctrl+R, T experience in full VS. I'm planning on sending a followup PR later in the week to add DebugInContext, and then a PR to the vscode extension to enable the extension. While Omnisharp does have an existing TestCommandService that does some of this, it doesn't quite replicate the experience: it will only run the test method that it's invoked in, not potentially the entire test class or whatever other context it's in. Additionally, it's not unified with the test running framework the test of the V2 test commands use, so I opted to introduce an entirely new command, RunTestsInContext, which will walk up the syntax tree for a given location to find the nearest test method or class, if one exists, and either run the method itself, or run the entire class.
| @@ -7,36 +7,39 @@ namespace OmniSharp.DotNetTest.TestFrameworks | |||
| { | |||
| internal abstract class TestFramework | |||
| { | |||
| private static readonly ImmutableDictionary<string, TestFramework> s_frameworks; | |||
| private static readonly ImmutableArray<TestFramework> s_frameworks; | |||
There was a problem hiding this comment.
It's a neat idea to make this extensible, but the amount of effort that would need to be put in to add another framework makes this a pointless optimization. It causes GetFrameworks() to allocate an iterator on every invocation (which is often), and even in the worst case is GetFramework(string) implemented as a foreach loop over an array will be twice as fast as a dictionary lookup: https://gist.github.com/333fred/2c259fff1ca0e074adead95f01db70ba.
|
I've now added |
| CancellationToken.None); | ||
| } | ||
|
|
||
| throw new InvalidOperationException("The debugger could not be started"); |
There was a problem hiding this comment.
These seem unfortunate, especially for the InContext version of the command, but the existing DebugTestGetStartInfoResponse doesn't have a place for a failure and failure message. I would have modified that to add a field, but I'm unsure if that will break existing clients. Is that a safe change to make, or should I add a new response that adds a failure/reason to it?
There was a problem hiding this comment.
I'm not really sure whether this would be a problem for other clients. Maybe @filipw knows definitively.
There was a problem hiding this comment.
it should be fine - additive changes are generally fine since it's json based.
moreover, the debug tests protocl is - I am pretty sure - only used by VS Code extension since the debugger is proprietary
| return framework.IsTestMethod(methodSymbol); | ||
| } | ||
|
|
||
| foreach (var f in TestFramework.Frameworks) |
There was a problem hiding this comment.
Technically, this means that you can't combine multiple testing framework types in a single file and have them run all at once. I think that's probably fine.
… to make the "no tests found" faster. Added better error reporting for consumption. Fixed a few debugging start bugs.
…ontext' commands This is the omnisharp-vsocde side of OmniSharp/omnisharp-roslyn#1782. I've implemented support for both of those commands, and bound them to the same default shortcuts as in VS proper. These commands can also be invoked from the command palette. I've also done a small bit of refactoring to remove references to legacy project.json support. There is no support for loading this project type in Omnisharp anymore, so this was cruft that was unnecessary at this point.
|
@JoeRobich I've implemented the vscode side here: dotnet/vscode-csharp#3772 |
This command tries to replicate the
CTRL+R, Texperience in full VS.I'm planning on sending a followup PR hopefully this weekend to add DebugInContext(now in this PR), and then a PR to the vscode extension to enable the commands (dotnet/vscode-csharp#3772). While Omnisharp does have an existingTestCommandServicethat does some of this, it doesn't quite replicate the experience: it will only run the test method that it's invoked in, not potentially the entire test class or whatever other context it's in. Additionally, it's not unified with the test running framework the test of the V2 test commands use, so I opted to introduce an entirely new command, RunTestsInContext, which will walk up the syntax tree for a given location to find the nearest test method or class, if one exists, and either run the method itself, or run the entire class.Commit 1 moves Omnisharp forward to the 3.1 LTS SDK, from 3.0.
Commit 2 does some refactoring of the existing test commands to remove
.Resultusage and plumb async all the way through.Commit 3 Implements RunTestsInContext.
Commit 4 is some cleanup.
Commit 5 Implements DebugTestsInContextGetStartInfo.
Commits 6-13 are some cleanup.