API: Correlate debug sessions and test runs.

[connor4312]

Currently, debug sessions and test runs live in two different worlds. This leads to some confusion, because stopping or restarting a debug session using the debug toolbar doesn't stop or restart the test run. Several extension authors have raised this as a pain point.

I propose a simple API that would allow the editor to correlate the two:

declare module 'vscode' {
	export interface DebugSessionOptions {
		/**
		 * Signals to the editor that the debug session was started from a test run
		 * request. This is used to link the lifecycle of the debug session and
		 * test run in UI actions.
		 */
		testRun?: TestRun;
	}
}

Then it can be passed in vscode.debug.startDebugging(folder, launchConfig, { testRun: myTestRun });

[connor4312]

This is now implemented behind the usual proposed shenanigans. Let me know how it works or if folks think anything is missing. This is a simple API so I think we could finalize it next iteration.

@mxschmitt I think you mentioned this before, + @jdneo @DanTup @eleanorjboyd

[connor4312]

We could technically make this work without API in some cases by using AsyncLocalStorage to associate sessions started from calls to the runHandlers. But I think we still want API as that is a rather exotic feature that can be defeated though various means.

[eleanorjboyd]

Gave this a try on the python extension and seems to be fixing the problem we were seeing about not getting the test_ids. I am still seeing the "restart" button in the debug panel leading to the debug session ending, which in the python extension signals to end the run instance. This means that using the refresh button results in the UI not being updated for test success or failures after since the run instance was ended. Should I change my code to not have the run instance end there? How should the extensions know when debug was done vs restarted in terms of ending the test run object?

[connor4312]

In the current implementation, hitting the restart button won't touch the debug session at all but will instead cancel the test run, wait for the run to complete, and then immediately start a new run with the same parameters:

vscode/src/vs/workbench/contrib/debug/browser/debugService.ts

Lines 846 to 857 in b814ff2

	if (session.correlatedTestRun) {
	if (!session.correlatedTestRun.completedAt) {
	this.testService.cancelTestRun(session.correlatedTestRun.id);
	await Event.toPromise(session.correlatedTestRun.onComplete);
	// todo@connor4312 is there any reason to wait for the debug session to
	// terminate? I don't think so, test extension should already handle any
	// state conflicts...
	}
	this.testService.runResolvedTests(session.correlatedTestRun.request);
	return;
	}

You should be able to keep updating states and such until you call testRun.end()

[DanTup]

In Dart, we were already handling debug session ends and calling .end() on the test run (whether it was created by VS Code, or by us). We don't seem to have any bugs with stopping debug sessions that are test runs.

However, restart doesn't work great - because it seems to end the test run, then start a new debug session that is not tied to a test run (so you don't get the results recorded). It's not clear to me if your fix here will solve that, but I wonder if there are going to have issues with test runs where VS Code doesn't have all of the related data. We discussed this before in #144169 (comment) about "Test: Rerun Last Run" not working because of this. If we let VS Code handle restarting test runs by just calling runTests, I think any context that we weren't able to capture in the TestRun won't come back to us, and we'll start a test run with some parameters missing.

[eleanorjboyd]

In the current implementation, hitting the restart button won't touch the debug session at all but will instead cancel the test run, wait for the run to complete, and then immediately start a new run with the same parameters:

vscode/src/vs/workbench/contrib/debug/browser/debugService.ts

Lines 846 to 857 in b814ff2

	if (session.correlatedTestRun) {
	if (!session.correlatedTestRun.completedAt) {
	this.testService.cancelTestRun(session.correlatedTestRun.id);
	await Event.toPromise(session.correlatedTestRun.onComplete);
	// todo@connor4312 is there any reason to wait for the debug session to
	// terminate? I don't think so, test extension should already handle any
	// state conflicts...
	}
	this.testService.runResolvedTests(session.correlatedTestRun.request);
	return;
	}

You should be able to keep updating states and such until you call testRun.end()

As I debug it I am still seeing onDidTerminateDebugSession be called when I click "restart". Here is a link to where that is in the extension: https://github.com/microsoft/vscode-python/blob/3e17bbe075daeb443bc0c47bc804f66f9cd1ce36/src/client/testing/common/debugLauncher.ts#L51. If that sounds wrong I can also send over my branch since I might have set it up incorrectly. Thanks

[connor4312]

Yep, please send your branch, thanks!

[eleanorjboyd]

here it is: https://github.com/eleanorjboyd/vscode-python/tree/debug-test-session-proposed

[connor4312]

Thanks, fixed

[vscodenpa]

Friendly ping! Looks like this issue requires some further steps to be verified. Please provide us with the steps necessary to verify this issue.

[connor4312]

1. Use the selfhost test provider in VS Code
2. Debug a test, and stop on a breakpoint somewhere
3. Click the "restart" action in the debug toolbar
4. Verify that the test run is cancelled and a new test run is started as debugging is restarted.

[DanTup]

@connor4312 I'm not sure if you wanted me to verify anything, but I still don't completely understand the functionality here - see my notes above at #214486 (comment)

[connor4312]

This doesn't affect how debug sessions stop at the moment, only how they restart. When a restart happens, the old test run request is cancelled, and once it ends a new test run request comes in with an identical profile and set of tests as was initially used to start the first run. This should fix the issue of the second, 'untracked' debug session.

It sounds like profiles are the right mechanic for your use case. I made some improvements to the case of "if they should only apply to specific files or folders" that you mentioned in your original comment a couple years ago. I you would like some UX improvements to make them more viable I would be happy to discuss 🙂

[DanTup]

This doesn't affect how debug sessions stop at the moment, only how they restart. When a restart happens, the old test run request is cancelled, and once it ends a new test run request comes in with an identical profile and set of tests as was initially used to start the first run.

Yep, understood. But in the case where a test run is created by us (so we can record test results) but doesn't contain all of the information to run a specific set of tests:

function runMySpecialSetOfTests() {
    const request = new vs.TestRunRequest();
    const run = this.controller.createTestRun(request, undefined, true);

    // Record results on `run`
}

Then won't Restart now end up running the entire set of tests, rather than just restarting the debug session as it did before (which might record results against the original run, but at least ran the correct set of tests)?

I'm assuming if I never set testRun then I won't be affected (we'll continue with the same behaviour), and I could look at providing this in the future if I can move over to profiles or something? Do you have any links to issues about the profile changes so I can see if they handle the cases I had issues with?

[connor4312]

Then won't Restart now end up running the entire set of tests, rather than just restarting the debug session as it did before (which might record results against the original run, but at least ran the correct set of tests)?

Yea -- just like the "rerun" actions would (which as you previously mentioned aren't good in your current setup.)

I'm assuming if I never set testRun then I won't be affected

Yep that's right

Do you have any links to issues about the profile changes so I can see if they handle the cases I had issues with?

Just a query which captures most things, but not everything.

Notably I think:

• testing: ensure run all runs everything, use consistent profile for re-runs #214260 the fix for the tagged ones I mentioned before
• Make Run Tests and Debug Tests default profile in sync  #166642 not sure if applicable, but if profiles across kinds have the same name then they'll be kept in sync when the default changes
• [Testing]: Get selected default Run/Debug TestProfile(s) #193160 allows getting and setting the default profiles, so you can programmatically query and change them.

[jdneo]

Hi, sorry for the late reply.

I tested the new api today, I can still hit debug.onDidTerminateDebugSession when restart the debug session, is this as expected? We use debug.onDidTerminateDebugSession to do clean up jobs.

[connor4312]

When restarting when a test run is provided, the test run is cancelled. Usually this will result in the extension stopping the debug session which will then trigger onDidTerminateDebugSession 00ce462#diff-e79a396155cbc97a29fd0a1a4741975d29336d0683d751c7a4856c2ac1e46dbeR844-R855