Skip to content

Implement client side of test debugging #5965

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

Merged
dibarbet merged 2 commits into from
Aug 1, 2023
Merged

Implement client side of test debugging #5965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05ca5e2
Implement client side of test debugging
dibarbet Jul 28, 2023
b02fdf6
Update to new roslyn version containing debug commands
dibarbet Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
}
},
"defaults": {
"roslyn": "4.8.0-1.23372.1",
"roslyn": "4.8.0-1.23381.8",
"omniSharp": "1.39.7",
"razor": "7.0.0-preview.23328.2",
"razorOmnisharp": "7.0.0-preview.23363.1"
Expand Down
21 changes: 21 additions & 0 deletions src/lsptoolshost/roslynLanguageServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ export class RoslynLanguageServer {

// Register Razor dynamic file info handling
this.registerDynamicFileInfo(this._languageClient);

this.registerDebuggerAttach(this._languageClient);
}

public async stop(): Promise<void> {
Expand Down Expand Up @@ -488,6 +490,25 @@ export class RoslynLanguageServer {
);
}

private registerDebuggerAttach(client: RoslynLanguageClient) {
client.onRequest<RoslynProtocol.DebugAttachParams, RoslynProtocol.DebugAttachResult, void>(
RoslynProtocol.DebugAttachRequest.type,
async (request) => {
const debugConfiguration: vscode.DebugConfiguration = {
name: '.NET Core Attach',
type: 'coreclr',
request: 'attach',
processId: request.processId,
};

const result = await vscode.debug.startDebugging(undefined, debugConfiguration, undefined);
return {
Copy link
Copy Markdown
Contributor

@gregg-miskelly gregg-miskelly Jul 28, 2023

Choose a reason for hiding this comment

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

Does something inside the target process block waiting for Debugger.IsAttached to return true? If not, you likely need a listener to know when the attach is complete (and we might need a debugger change to raise a new event).

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023

Choose a reason for hiding this comment

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

Yeah - the server side blocks and the tests won't start running until it gets this response from the client.

Copy link
Copy Markdown
Contributor

@gregg-miskelly gregg-miskelly Jul 28, 2023

Choose a reason for hiding this comment

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

Right, that isn't good enough -- you are going to return before the real attach is complete

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah the vscode API returns before the attach is complete? That is interesting. So on the server side I could wait for Debugger.IsAttached? Is there any client side API that I could use to know when the real attach is done, or do I have to do it on the server?

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm this is hard because I'm not actually attaching to the server process, but the vs test process. So I can't check Debugger.IsAttached in the server, that check has to be done in the test host, which I don't think we can do easily.

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023

Choose a reason for hiding this comment

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

you likely need a listener to know when the attach is complete (and we might need a debugger change to raise a new event).

Do you have any more details on how to do this?

Also wondering if any of the other vscode debug events might be able to let me know whenthe debugger is actually attached.

/**
         * An {@link Event} which fires when the {@link debug.activeDebugSession active debug session}
         * has changed. *Note* that the event also fires when the active debug session changes
         * to `undefined`.
         */
        export const onDidChangeActiveDebugSession: Event<DebugSession | undefined>;

        /**
         * An {@link Event} which fires when a new {@link DebugSession debug session} has been started.
         */
        export const onDidStartDebugSession: Event<DebugSession>;

        /**
         * An {@link Event} which fires when a custom DAP event is received from the {@link DebugSession debug session}.
         */
        export const onDidReceiveDebugSessionCustomEvent: Event<DebugSessionCustomEvent>;

Copy link
Copy Markdown
Contributor

@gregg-miskelly gregg-miskelly Jul 28, 2023

Choose a reason for hiding this comment

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

If it isn't easy for the test host to call Debugger.IsAttached I think what we should do is have the debugger raise a custom AttachComplete event that we should be able to receive through onDidReceiveDebugSessionCustomEvent. If that sounds good to you, I can open an issue.

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023

Choose a reason for hiding this comment

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

Yeah I think having the debugger raise a complete event sounds perfect. That would then work no matter what process we're trying to attach to.

Copy link
Copy Markdown
Contributor

@gregg-miskelly gregg-miskelly Jul 28, 2023

Choose a reason for hiding this comment

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

I opened #5968. I would suggest completing this PR when you are ready and I can wire things up when we insert a debugger with the fix.

Copy link
Copy Markdown
Member Author

@dibarbet dibarbet Jul 28, 2023

Choose a reason for hiding this comment

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

Perfect thanks! It will likely take a day or two for this PR to go in as the server side changes also need to be reviewed and merged before this goes in.

didAttach: result,
};
}
);
}

private registerExtensionsChanged(languageClient: RoslynLanguageClient) {
// subscribe to extension change events so that we can get notified if C# Dev Kit is added/removed later.
languageClient.addDisposable(
Expand Down
19 changes: 19 additions & 0 deletions src/lsptoolshost/roslynProtocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ export interface RunTestsParams extends lsp.WorkDoneProgressParams, lsp.PartialR
* Note that this does not have to only include tests, for example this could be a range representing a class.
*/
range: lsp.Range;

/**
* Whether the request should attempt to call back to the client to attach a debugger before running the tests.
*/
attachDebugger: boolean;
}

export interface TestProgress {
Expand Down Expand Up @@ -103,6 +108,14 @@ export interface RunTestsPartialResult {
progress?: TestProgress;
}

export interface DebugAttachParams {
processId: number;
}

export interface DebugAttachResult {
didAttach: boolean;
}

export namespace WorkspaceDebugConfigurationRequest {
export const method = 'workspace/debugConfiguration';
export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.clientToServer;
Expand Down Expand Up @@ -140,3 +153,9 @@ export namespace RunTestsRequest {
void
>(method);
}

export namespace DebugAttachRequest {
export const method = 'workspace/attachDebugger';
export const messageDirection: lsp.MessageDirection = lsp.MessageDirection.serverToClient;
export const type = new lsp.RequestType<DebugAttachParams, DebugAttachResult, void>(method);
}
16 changes: 14 additions & 2 deletions src/lsptoolshost/unitTesting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,31 @@ export function registerUnitTestingCommands(
context.subscriptions.push(
vscode.commands.registerTextEditorCommand(
'dotnet.test.runTestsInContext',
async (textEditor: vscode.TextEditor) => runTestsInContext(textEditor, languageServer, dotnetTestChannel)
async (textEditor: vscode.TextEditor) => {
return runTestsInContext(false, textEditor, languageServer, dotnetTestChannel);
}
)
);

context.subscriptions.push(
vscode.commands.registerTextEditorCommand(
'dotnet.test.debugTestsInContext',
async (textEditor: vscode.TextEditor) => {
return runTestsInContext(true, textEditor, languageServer, dotnetTestChannel);
}
)
);
}

async function runTestsInContext(
debug: boolean,
textEditor: vscode.TextEditor,
languageServer: RoslynLanguageServer,
dotnetTestChannel: vscode.OutputChannel
) {
const contextRange: languageClient.Range = { start: textEditor.selection.active, end: textEditor.selection.active };
const textDocument: languageClient.TextDocumentIdentifier = { uri: textEditor.document.fileName };
const request: RunTestsParams = { textDocument: textDocument, range: contextRange };
const request: RunTestsParams = { textDocument: textDocument, range: contextRange, attachDebugger: debug };
await runTests(request, languageServer, dotnetTestChannel);
}

Expand Down