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.

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

Implement LSP debug test commands #69267

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Implement LSP debug test commands #69267

merged 4 commits into from
Jul 31, 2023

Conversation

dibarbet
Copy link
Member

Implements server side of test debugging. Part of dotnet/vscode-csharp#5719

See the client PR for a gif - dotnet/vscode-csharp#5965

@dibarbet dibarbet requested a review from a team as a code owner July 28, 2023 00:57
@dibarbet dibarbet added the LSP issues related to the roslyn language server protocol implementation label Jul 28, 2023
@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 28, 2023
@@ -120,12 +120,24 @@
<data name="Aborted" xml:space="preserve">
<value>Aborted!</value>
</data>
<data name="Attaching_debugger_to_process" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

It looks like should be called
Attaching_debugger_to_process_0

if (attachDebugger)
{
// When we want to debug tests we need to use a custom test launcher so that we get called back with the process to attach to.
vsTestConsoleWrapper.RunTestsWithCustomTestHost(testCases, runSettings: null, handler, new DebugTestHostLauncher(progress, clientLanguageServerManager));
Copy link
Member

Choose a reason for hiding this comment

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

Just to check, O# also doesn't support runSettings, is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

O# does, but I haven't implemented it yet here. That will be my next followup.

// Send an explicit request to the client to tell it to attach to the debugger and wait for the response.
// We want to wait for the attach to complete before we continue.
var task = Task.Run(async () => await AttachDebuggerAsync(processId, cancellationToken), cancellationToken);
return task.WaitAndGetResult_CanCallOnBackground(cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked at the client-side PR. But it feels like
the client-side debug test command is waiting for the server debug test, and the server then waits for the client to attach the debugger to the process. 🤔 Are the 'debugger attaching' work lives in a separate thread?

Copy link
Member

Choose a reason for hiding this comment

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

I just feel this looks like a deadlock, maybe it's fine as long as you test that works😄

Copy link
Member Author

@dibarbet dibarbet Jul 31, 2023

Choose a reason for hiding this comment

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

Not 100% sure on the question - but yes the client is not fully blocked waiting for the run tests command to complete. It needs to be able to handle the partial results (e.g. the logging) and this debug attach request while its waiting for the overall request to complete.

@Cosifne
Copy link
Member

Cosifne commented Jul 31, 2023

A few minor questions and I will sign this off after they get answer :)

@dibarbet dibarbet enabled auto-merge July 31, 2023 20:24
@dibarbet dibarbet merged commit 441b6cc into dotnet:main Jul 31, 2023
24 checks passed
@ghost ghost added this to the Next milestone Jul 31, 2023
@dibarbet dibarbet deleted the debug_tests branch July 31, 2023 22:50
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE LSP issues related to the roslyn language server protocol implementation untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants