Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement simple LSP run tests #68889

Merged
merged 9 commits into from
Jul 19, 2023
Merged

Implement simple LSP run tests #68889

merged 9 commits into from
Jul 19, 2023

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented Jul 6, 2023 •
edited
Loading

First part of dotnet/vscode-csharp#5719

Implements the server side code to handle the run tests command and the codelens for the same.

Client side PR - dotnet/vscode-csharp#5897

The general runtests approach is as follows

  • The client does as little as possible - it calls the server whenever it sees a run tests requests and passes whatever range it was given.
    • We save the files to ensure the contents are up to date on the server when we build.
    • The server streams in messages and progress which the client writes to the test output window and updates the progress bar (cancellable).
  • The server then implements the actual run tests command
    • Shell out to the dotnet CLI to build the project. This is required to ensure the the output dll is relatively up to date with the request snapshot. We cannot guarantee that the server snapshot will 100% match the build output, but we do our best. O# does something similar.
    • Runs test discovery
      • First we search the request snapshot for possible test methods in a given range. This essentially just looks for methods with a matching attribute name. We don't desire 100% accuracy and neither does O#. We'll leave 100% correctness to devkit.
      • Shell out to the vstest console to run test discovery on the project output dll.
      • Figure out which discovered tests are the ones we want to run from the potential methods.
    • Run the discovered tests
      • Shells out to the vstest console to run the discovered test cases. Fairly straightforward, but we need to prettify the output in the console.
  • For codelens we use the same potential test method discovery to tag test methods and test classes. The actual codelens is just a command that has the test method range to run.

run_tests

Followups in next PRs

  1. run settings
  2. finish localization
  3. debug commands

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 6, 2023
@dibarbet dibarbet mentioned this pull request Jul 6, 2023
Merged
@dibarbet dibarbet marked this pull request as ready for review July 6, 2023 02:47
@dibarbet dibarbet requested review from a team as code owners July 6, 2023 02:47
dibarbet
dibarbet commented Jul 6, 2023
View reviewed changes
src/Features/CSharp/Portable/Testing/CSharpTestMethodFinder.cs
[ExportLanguageService(typeof(ITestMethodFinder), LanguageNames.CSharp), Shared]
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal class CSharpTestMethodFinder([ImportMany] IEnumerable<ITestFrameworkMetadata> testFrameworks) : AbstractTestMethodFinder<MethodDeclarationSyntax>(testFrameworks)
Copy link
Member Author

Choose a reason for hiding this comment

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

abstracted out because we have language specific details here.

dibarbet
dibarbet commented Jul 6, 2023
View reviewed changes
src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/DotnetCliHelper.cs Outdated
namespace Microsoft.CodeAnalysis.LanguageServer;

[Export, Shared]
internal class DotnetCliHelper
Copy link
Member Author

Choose a reason for hiding this comment

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

we need access to various dotnet components for two operations currently

  1. We need to run a dotnet build to ensure the project is up to date
  2. We need to find the vstest.console.dll from the appropriate dotnet SDK directory.

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 12, 2023
View reviewed changes
src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/DotnetCliHelper.cs
Contract.ThrowIfFalse(File.Exists(vstestConsole), $"VSTestConsole was not found at {vstestConsole}");
return vstestConsole;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

my eyes glazed over her.e i trust you :)

CyrusNajmabadi
CyrusNajmabadi reviewed Jul 12, 2023
View reviewed changes
src/Features/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Testing/RunTestsHandler.cs
_ => throw new InvalidOperationException($"Unexpected log level {serverConfiguration.MinimumLogLevel}"),
};
}
}
Copy link
Member

Choose a reason for hiding this comment

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

glazed over here as well.

Cosifne
Cosifne reviewed Jul 13, 2023
View reviewed changes
src/Features/LanguageServer/ProtocolUnitTests/CodeLens/CSharpCodeLensTests.cs
@@ -231,4 +231,56 @@ void UseM()
var firstDocumentResult2 = await testLspServer.ExecuteRequestAsync<LSP.CodeLens, LSP.CodeLens>(LSP.Methods.CodeLensResolveName, firstCodeLens, CancellationToken.None);
Assert.NotNull(firstDocumentResult2?.Command?.Title);
}

[Theory, CombinatorialData]
public async Task TestHasRunTestsCommandAsync(bool mutatingLspWorkspace)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add another two tests covering the basic scenario of NUnit and MSTest?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think for now I'll skip these:

  1. The nunit / mstest finding is already exercised in a bunch of other tests at a lower layer, and the codelens code itself doesn't know about them (the command is entirely generic)
  2. I want to avoid another round of CI here.

I'll look at adding them when I do the debugging part.

Cosifne
Cosifne approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@Cosifne Cosifne left a comment

Choose a reason for hiding this comment

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

It seems like what we can do best ... right now 🙂
A few minor sugguestions.

@dibarbet dibarbet merged commit f1bf21e into dotnet:main Jul 19, 2023
@dibarbet dibarbet deleted the dev/dibarbet/unit_testing branch July 19, 2023 17:33
@allisonchou allisonchou mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyamnamboodiripad shyamnamboodiripad shyamnamboodiripad left review comments

@Cosifne Cosifne Cosifne approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Assignees
No one assigned
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dibarbet @CyrusNajmabadi @Cosifne @shyamnamboodiripad