Endpoints for running and debugging all tests in a class #1089

akshita31 · 2018-01-13T01:32:21Z

Added endpoints to enable "Run All Tests" in class and "Debug All Tests" in class

Please review : @DustinCampbell @rchande @TheRealPiotrP @filipw

akshita31 · 2018-01-13T01:55:34Z

Omnisharp-vscode side : dotnet/vscode-csharp#1961

rchande

Some questions

rchande · 2018-01-17T20:25:55Z

src/OmniSharp.DotNetTest/DebugSessionManager.cs

@@ -78,10 +78,14 @@ public void EndSession()
        }

        public Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)
+            => DebugGetStartInfoAsync(new string[] { methodName }, testFrameworkName, targetFrameworkVersion, cancellationToken);
+


Extra blank line

Fixed that.

rchande · 2018-01-17T20:26:55Z

src/OmniSharp.DotNetTest/Models/DebugTestClassGetStartInfoRequest.cs

+    [OmniSharpEndpoint(OmniSharpEndpoints.V2.DebugTestsInClassGetStartInfo, typeof(DebugTestClassGetStartInfoRequest), typeof(DebugTestGetStartInfoResponse))]
+    class DebugTestClassGetStartInfoRequest :  Request
+    {
+        public string[] MethodsInClass { get; set; }


If this is just a list of methods, what happens if someone passes methods that aren't all in the same class?

rchande · 2018-01-17T20:27:39Z

src/OmniSharp.DotNetTest/Models/RunTestInClassRequest.cs

+namespace OmniSharp.DotNetTest.Models
+{
+    [OmniSharpEndpoint(OmniSharpEndpoints.V2.RunAllTestsInClass, typeof(RunTestsInClassRequest), typeof(RunTestResponse[]))]
+    public class RunTestsInClassRequest : Request


I feel like this could share a base class with the DebugTests request (they're identical, no?)

rchande · 2018-01-17T20:30:56Z

src/OmniSharp.DotNetTest/TestManager.cs

@@ -76,6 +76,11 @@ public static TestManager Create(Project project, DotNetCliService dotNetCli, IE
        public abstract GetTestStartInfoResponse GetTestStartInfo(string methodName, string testFrameworkName, string targetFrameworkVersion);

        public abstract Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken);
+
+        public virtual Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string[] methodNames, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)


What other types derive from TestManager? Should they implement this?

There's just two -- a legacy one for the old 'dotnet test' on project.json projects and the current 'dotnet vstest' version. I would only expect us to make this work for the newer stuff.

I have implemented this function only for the VSTestManager and not LegacyTestManager. Is that required ?

What is the behavior if you open a project with legacy tests? Does the run tests button show up? If it does, and you click it, what happens?

A project with legacy tests == a project.json project. I don't think we should invest effort in that. So long as we don't break the ability to run/debug a single test in a project.json project, I don't think we need to make any changes to LegacyTestManager.

Sounds good. I'm happy with our solution of not showing the indicator for legacy projects.

rchande · 2018-01-17T20:31:44Z

src/OmniSharp.DotNetTest/VSTestManager.cs

        {
            VerifyTestFramework(testFrameworkName);

-            var testCases = await DiscoverTestsAsync(methodName, targetFrameworkVersion, cancellationToken);
+            var testCases = new List<TestCase>();
+            foreach(var methodName in methodNames)


Formatting:

foreach (....) { //stuff }

rchande · 2018-01-17T20:32:55Z

src/OmniSharp.DotNetTest/Services/RunTestsInClassService.cs

+namespace OmniSharp.DotNetTest.Services
+{
+    [OmniSharpHandler(OmniSharpEndpoints.V2.RunAllTestsInClass, LanguageNames.CSharp)]
+    internal class RunTestsInClassService : BaseTestService<RunTestsInClassRequest, RunTestResponse[]>


How similar is this to the class that runs a single test in a class?

It is the same now, only the type of request differs. For one it is RunTestRequest and for the other it is RunTestInClassRequest and the corresponding fields - MethodName and MethodNames also differ.

DustinCampbell · 2018-01-19T17:50:47Z

src/OmniSharp.DotNetTest/DebugSessionManager.cs

@@ -78,10 +78,14 @@ public void EndSession()
        }

        public Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)
+            => DebugGetStartInfoAsync(new string[] { methodName }, testFrameworkName, targetFrameworkVersion, cancellationToken);
+


DustinCampbell · 2018-01-19T17:52:58Z

src/OmniSharp.DotNetTest/Services/RunTestsInClassService.cs

+            if (testManager.IsConnected)
+            {
+                foreach (var methodName in request.MethodNames)
+                    responses.Add(testManager.RunTest(methodName, request.TestFrameworkName, request.TargetFrameworkVersion));


Should there be a method that can run tests in bulk (taking several method names) rather than looping through the tests and running them individually?

I would expect running several tests at once to be faster.

Looking at what you've done with DebugGetStartInfoAsync in VSTestManager, I think you should probably do the same for RunTest.

DustinCampbell · 2018-01-19T17:54:25Z

src/OmniSharp.DotNetTest/TestManager.cs

@@ -76,6 +76,11 @@ public static TestManager Create(Project project, DotNetCliService dotNetCli, IE
        public abstract GetTestStartInfoResponse GetTestStartInfo(string methodName, string testFrameworkName, string targetFrameworkVersion);

        public abstract Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken);
+
+        public virtual Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string[] methodNames, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)


There's just two -- a legacy one for the old 'dotnet test' on project.json projects and the current 'dotnet vstest' version. I would only expect us to make this work for the newer stuff.

DustinCampbell · 2018-01-19T17:54:34Z

src/OmniSharp.DotNetTest/TestManager.cs

+        public virtual Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string[] methodNames, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)
+        {
+            throw new NotImplementedException();
+        }


Missing blank line.

rchande

Can you confirm the legacy behavior if the button shows up? Otherwise looks good

rchande · 2018-01-22T23:45:13Z

src/OmniSharp.DotNetTest/Services/DebugTestClassService.cs

+            : base(workspace, dotNetCli, eventEmitter, loggerFactory)
+        {
+            _debugSessionManager = debugSessionManager;
+        }


Needs a blank line.

rchande · 2018-01-22T23:45:44Z

src/OmniSharp.DotNetTest/Services/RunTestsInClassService.cs

+
+        protected override RunTestResponse HandleRequest(RunTestsInClassRequest request, TestManager testManager)
+        {
+


Extra blank line

rchande · 2018-01-22T23:47:09Z

src/OmniSharp.DotNetTest/TestManager.cs

@@ -76,6 +76,11 @@ public static TestManager Create(Project project, DotNetCliService dotNetCli, IE
        public abstract GetTestStartInfoResponse GetTestStartInfo(string methodName, string testFrameworkName, string targetFrameworkVersion);

        public abstract Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken);
+
+        public virtual Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string[] methodNames, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)


What is the behavior if you open a project with legacy tests? Does the run tests button show up? If it does, and you click it, what happens?

rchande · 2018-01-22T23:47:30Z

src/OmniSharp.DotNetTest/VSTestManager.cs

@@ -184,10 +191,19 @@ public override GetTestStartInfoResponse GetTestStartInfo(string methodName, str
        }

        public override RunTestResponse RunTest(string methodName, string testFrameworkName, string targetFrameworkVersion)
+        => RunTest(new string[] { methodName }, testFrameworkName, targetFrameworkVersion);


DustinCampbell · 2018-01-23T16:37:30Z

src/OmniSharp.DotNetTest/VSTestManager.cs

+            var testCasesList = new List<TestCase>();
+            foreach (var methodName in methodNames)
+            {
+                testCasesList.AddRange(DiscoverTests(methodName, targetFrameworkVersion));


This will cause test discovery to be performed for every method in the class, which is unnecessarily slow. Could you update DiscoverTests to take multiple method names?

DustinCampbell · 2018-01-23T20:10:23Z

src/OmniSharp.DotNetTest/VSTestManager.cs

@@ -302,7 +292,7 @@ public override RunTestResponse RunTest(string[] methodNames, string testFramewo

                            testName = testName.Trim();

-                            if (testName.Equals(methodName, StringComparison.Ordinal))
+                            if (Array.Exists(methodNames, methodName => testName.Equals(methodName, StringComparison.Ordinal)))


Nit: If you want to remove the O(n) operation here, you could add the method names to a HashSet<string>(StringComparison.Ordinal) and test that for containership here.

akshita31 · 2018-01-23T21:58:34Z

The "run all tests" and "debug all tests" codelens will appear only for the MsBuild type project system and not for the legacy projects.

rchande

LGTM--thanks!

rchande · 2018-01-24T16:59:51Z

src/OmniSharp.DotNetTest/Models/BaseTestClassRequest.cs

+    {
+        public string[] MethodNames { get; set; }
+        public string TestFrameworkName { get; set; }
+        /// <summary>


Above the doc comment

rchande · 2018-01-24T17:02:58Z

src/OmniSharp.DotNetTest/TestManager.cs

@@ -76,6 +76,11 @@ public static TestManager Create(Project project, DotNetCliService dotNetCli, IE
        public abstract GetTestStartInfoResponse GetTestStartInfo(string methodName, string testFrameworkName, string targetFrameworkVersion);

        public abstract Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string methodName, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken);
+
+        public virtual Task<DebugTestGetStartInfoResponse> DebugGetStartInfoAsync(string[] methodNames, string testFrameworkName, string targetFrameworkVersion, CancellationToken cancellationToken)


Sounds good. I'm happy with our solution of not showing the indicator for legacy projects.

DustinCampbell

Looks great to me!

akshita31 added 7 commits January 5, 2018 18:38

New endpoint for running all tests

3980673

Removed method name from run test in class

a6876e1

Added code for debug all tests

6167298

Debug All Tests running

2e8dd45

Code clean up

2d4d3bb

Added newline

0aa7772

Merge branch 'master' into class_test

00f3344

akshita31 mentioned this pull request Jan 13, 2018

"Run All Tests" and "Debug All Tests" dotnet/vscode-csharp#1961

Merged

rchande suggested changes Jan 17, 2018

View reviewed changes

Renamed MethodNames and added Base Class

bf2fc45

DustinCampbell requested changes Jan 19, 2018

View reviewed changes

akshita31 added 2 commits January 19, 2018 17:07

Changes to return only one run test response

ddf6978

Merge branch 'master' into class_test

7bfbb59

rchande approved these changes Jan 22, 2018

View reviewed changes

Resolved review comments

c5b404b

DustinCampbell reviewed Jan 23, 2018

View reviewed changes

Discover tests using an array of strings

21f36bc

DustinCampbell reviewed Jan 23, 2018

View reviewed changes

Added hashset for faster searching

9ecc6a2

rchande approved these changes Jan 24, 2018

View reviewed changes

Added new line above doc comment

c67062c

DustinCampbell approved these changes Jan 24, 2018

View reviewed changes

Ravi Chande added 2 commits January 29, 2018 09:06

Merge branch 'master' into class_test

34da9a3

Merge branch 'master' into class_test

e9ce2b5

rchande merged commit 759f062 into OmniSharp:master Jan 29, 2018

akshita31 deleted the class_test branch January 29, 2018 21:03


		protected override RunTestResponse HandleRequest(RunTestsInClassRequest request, TestManager testManager)
		{

Endpoints for running and debugging all tests in a class #1089

Endpoints for running and debugging all tests in a class #1089

Conversation

akshita31 commented Jan 13, 2018 • edited Loading

akshita31 commented Jan 13, 2018

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Jan 23, 2018

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

akshita31 commented Jan 13, 2018 •

edited

Loading