Skip to content

Commit 8c54b8a

Browse files
eleanorjboydeleanorjboyd
authored
Discovery cancellation (#24713)
fixes #24602
1 parent 4d45042 commit 8c54b8a

File tree

8 files changed

+280
-74
lines changed

8 files changed

+280
-74
lines changed

‎src/client/testing/testController/common/types.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -156,18 +156,19 @@ export interface ITestResultResolver {
156156
}
157157
export interface ITestDiscoveryAdapter {
158158
// ** first line old method signature, second line new method signature
159-
discoverTests(uri: Uri): Promise<DiscoveredTestPayload>;
159+
discoverTests(uri: Uri): Promise<void>;
160160
discoverTests(
161161
uri: Uri,
162-
executionFactory: IPythonExecutionFactory,
162+
executionFactory?: IPythonExecutionFactory,
163+
token?: CancellationToken,
163164
interpreter?: PythonEnvironment,
164-
): Promise<DiscoveredTestPayload>;
165+
): Promise<void>;
165166
}
166167

167168
// interface for execution/runner adapter
168169
export interface ITestExecutionAdapter {
169170
// ** first line old method signature, second line new method signature
170-
runTests(uri: Uri, testIds: string[], profileKind?: boolean | TestRunProfileKind): Promise<ExecutionTestPayload>;
171+
runTests(uri: Uri, testIds: string[], profileKind?: boolean | TestRunProfileKind): Promise<void>;
171172
runTests(
172173
uri: Uri,
173174
testIds: string[],
@@ -176,7 +177,7 @@ export interface ITestExecutionAdapter {
176177
executionFactory?: IPythonExecutionFactory,
177178
debugLauncher?: ITestDebugLauncher,
178179
interpreter?: PythonEnvironment,
179-
): Promise<ExecutionTestPayload>;
180+
): Promise<void>;
180181
}
181182

182183
// Same types as in python_files/unittestadapter/utils.py

‎src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts‎

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
import * as path from 'path';
4-
import { Uri } from 'vscode';
4+
import { CancellationToken, CancellationTokenSource, Uri } from 'vscode';
55
import * as fs from 'fs';
6+
import { ChildProcess } from 'child_process';
67
import {
78
ExecutionFactoryCreateWithEnvironmentOptions,
89
IPythonExecutionFactory,
910
SpawnOptions,
1011
} from '../../../common/process/types';
1112
import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
12-
import { Deferred } from '../../../common/utils/async';
13+
import { createDeferred, Deferred } from '../../../common/utils/async';
1314
import { EXTENSION_ROOT_DIR } from '../../../constants';
1415
import { traceError, traceInfo, traceVerbose, traceWarn } from '../../../logging';
1516
import { DiscoveredTestPayload, ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
@@ -40,24 +41,39 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
4041
async discoverTests(
4142
uri: Uri,
4243
executionFactory?: IPythonExecutionFactory,
44+
token?: CancellationToken,
4345
interpreter?: PythonEnvironment,
44-
): Promise<DiscoveredTestPayload> {
45-
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
46-
this.resultResolver?.resolveDiscovery(data);
46+
): Promise<void> {
47+
const cSource = new CancellationTokenSource();
48+
const deferredReturn = createDeferred<void>();
49+
50+
token?.onCancellationRequested(() => {
51+
traceInfo(`Test discovery cancelled.`);
52+
cSource.cancel();
53+
deferredReturn.resolve();
4754
});
4855

49-
await this.runPytestDiscovery(uri, name, executionFactory, interpreter);
56+
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
57+
// if the token is cancelled, we don't want process the data
58+
if (!token?.isCancellationRequested) {
59+
this.resultResolver?.resolveDiscovery(data);
60+
}
61+
}, cSource.token);
62+
63+
this.runPytestDiscovery(uri, name, cSource, executionFactory, interpreter, token).then(() => {
64+
deferredReturn.resolve();
65+
});
5066

51-
// this is only a placeholder to handle function overloading until rewrite is finished
52-
const discoveryPayload: DiscoveredTestPayload = { cwd: uri.fsPath, status: 'success' };
53-
return discoveryPayload;
67+
return deferredReturn.promise;
5468
}
5569

5670
async runPytestDiscovery(
5771
uri: Uri,
5872
discoveryPipeName: string,
73+
cSource: CancellationTokenSource,
5974
executionFactory?: IPythonExecutionFactory,
6075
interpreter?: PythonEnvironment,
76+
token?: CancellationToken,
6177
): Promise<void> {
6278
const relativePathToPytest = 'python_files';
6379
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
@@ -111,6 +127,12 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
111127
args: execArgs,
112128
env: (mutableEnv as unknown) as { [key: string]: string },
113129
});
130+
token?.onCancellationRequested(() => {
131+
traceInfo(`Test discovery cancelled, killing pytest subprocess for workspace ${uri.fsPath}`);
132+
proc.kill();
133+
deferredTillExecClose.resolve();
134+
cSource.cancel();
135+
});
114136
proc.stdout.on('data', (data) => {
115137
const out = fixLogLinesNoTrailing(data.toString());
116138
traceInfo(out);
@@ -143,6 +165,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
143165
throwOnStdErr: true,
144166
outputChannel: this.outputChannel,
145167
env: mutableEnv,
168+
token,
146169
};
147170

148171
// Create the Python environment in which to execute the command.
@@ -154,7 +177,21 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
154177
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
155178

156179
const deferredTillExecClose: Deferred<void> = createTestingDeferred();
180+
181+
let resultProc: ChildProcess | undefined;
182+
183+
token?.onCancellationRequested(() => {
184+
traceInfo(`Test discovery cancelled, killing pytest subprocess for workspace ${uri.fsPath}`);
185+
// if the resultProc exists just call kill on it which will handle resolving the ExecClose deferred, otherwise resolve the deferred here.
186+
if (resultProc) {
187+
resultProc?.kill();
188+
} else {
189+
deferredTillExecClose.resolve();
190+
cSource.cancel();
191+
}
192+
});
157193
const result = execService?.execObservable(execArgs, spawnOptions);
194+
resultProc = result?.proc;
158195

159196
// Take all output from the subprocess and add it to the test output channel. This will be the pytest output.
160197
// Displays output to user and ensure the subprocess doesn't run into buffer overflow.

‎src/client/testing/testController/pytest/pytestExecutionAdapter.ts‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
3838
executionFactory?: IPythonExecutionFactory,
3939
debugLauncher?: ITestDebugLauncher,
4040
interpreter?: PythonEnvironment,
41-
): Promise<ExecutionTestPayload> {
41+
): Promise<void> {
4242
const deferredTillServerClose: Deferred<void> = utils.createTestingDeferred();
4343

4444
// create callback to handle data received on the named pipe
@@ -59,12 +59,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
5959
);
6060
runInstance?.token.onCancellationRequested(() => {
6161
traceInfo(`Test run cancelled, resolving 'TillServerClose' deferred for ${uri.fsPath}.`);
62-
const executionPayload: ExecutionTestPayload = {
63-
cwd: uri.fsPath,
64-
status: 'success',
65-
error: '',
66-
};
67-
return executionPayload;
6862
});
6963

7064
try {
@@ -82,15 +76,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
8276
} finally {
8377
await deferredTillServerClose.promise;
8478
}
85-
86-
// placeholder until after the rewrite is adopted
87-
// TODO: remove after adoption.
88-
const executionPayload: ExecutionTestPayload = {
89-
cwd: uri.fsPath,
90-
status: 'success',
91-
error: '',
92-
};
93-
return executionPayload;
9479
}
9580

9681
private async runTestsNew(
@@ -244,7 +229,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
244229
});
245230

246231
const result = execService?.execObservable(runArgs, spawnOptions);
247-
resultProc = result?.proc;
248232

249233
// Take all output from the subprocess and add it to the test output channel. This will be the pytest output.
250234
// Displays output to user and ensure the subprocess doesn't run into buffer overflow.

‎src/client/testing/testController/unittest/testDiscoveryAdapter.ts‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
// Licensed under the MIT License.
33

44
import * as path from 'path';
5-
import { Uri } from 'vscode';
5+
import { CancellationTokenSource, Uri } from 'vscode';
6+
import { CancellationToken } from 'vscode-jsonrpc';
7+
import { ChildProcess } from 'child_process';
68
import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
79
import { EXTENSION_ROOT_DIR } from '../../../constants';
810
import {
@@ -40,15 +42,31 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
4042
private readonly envVarsService?: IEnvironmentVariablesProvider,
4143
) {}
4244

43-
public async discoverTests(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise<DiscoveredTestPayload> {
45+
public async discoverTests(
46+
uri: Uri,
47+
executionFactory?: IPythonExecutionFactory,
48+
token?: CancellationToken,
49+
): Promise<void> {
4450
const settings = this.configSettings.getSettings(uri);
4551
const { unittestArgs } = settings.testing;
4652
const cwd = settings.testing.cwd && settings.testing.cwd.length > 0 ? settings.testing.cwd : uri.fsPath;
4753

48-
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
49-
this.resultResolver?.resolveDiscovery(data);
54+
const cSource = new CancellationTokenSource();
55+
// Create a deferred to return to the caller
56+
const deferredReturn = createDeferred<void>();
57+
58+
token?.onCancellationRequested(() => {
59+
traceInfo(`Test discovery cancelled.`);
60+
cSource.cancel();
61+
deferredReturn.resolve();
5062
});
5163

64+
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
65+
if (!token?.isCancellationRequested) {
66+
this.resultResolver?.resolveDiscovery(data);
67+
}
68+
}, cSource.token);
69+
5270
// set up env with the pipe name
5371
let env: EnvironmentVariables | undefined = await this.envVarsService?.getEnvironmentVariables(uri);
5472
if (env === undefined) {
@@ -62,24 +80,22 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
6280
command,
6381
cwd,
6482
outChannel: this.outputChannel,
83+
token,
6584
};
6685

67-
try {
68-
await this.runDiscovery(uri, options, name, cwd, executionFactory);
69-
} finally {
70-
// none
71-
}
72-
// placeholder until after the rewrite is adopted
73-
// TODO: remove after adoption.
74-
const discoveryPayload: DiscoveredTestPayload = { cwd, status: 'success' };
75-
return discoveryPayload;
86+
this.runDiscovery(uri, options, name, cwd, cSource, executionFactory).then(() => {
87+
deferredReturn.resolve();
88+
});
89+
90+
return deferredReturn.promise;
7691
}
7792

7893
async runDiscovery(
7994
uri: Uri,
8095
options: TestCommandOptions,
8196
testRunPipeName: string,
8297
cwd: string,
98+
cSource: CancellationTokenSource,
8399
executionFactory?: IPythonExecutionFactory,
84100
): Promise<void> {
85101
// get and edit env vars
@@ -103,6 +119,12 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
103119
args,
104120
env: (mutableEnv as unknown) as { [key: string]: string },
105121
});
122+
options.token?.onCancellationRequested(() => {
123+
traceInfo(`Test discovery cancelled, killing unittest subprocess for workspace ${uri.fsPath}`);
124+
proc.kill();
125+
deferredTillExecClose.resolve();
126+
cSource.cancel();
127+
});
106128
proc.stdout.on('data', (data) => {
107129
const out = fixLogLinesNoTrailing(data.toString());
108130
traceInfo(out);
@@ -148,7 +170,19 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
148170
};
149171
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);
150172

173+
let resultProc: ChildProcess | undefined;
174+
options.token?.onCancellationRequested(() => {
175+
traceInfo(`Test discovery cancelled, killing unittest subprocess for workspace ${uri.fsPath}`);
176+
// if the resultProc exists just call kill on it which will handle resolving the ExecClose deferred, otherwise resolve the deferred here.
177+
if (resultProc) {
178+
resultProc?.kill();
179+
} else {
180+
deferredTillExecClose.resolve();
181+
cSource.cancel();
182+
}
183+
});
151184
const result = execService?.execObservable(args, spawnOptions);
185+
resultProc = result?.proc;
152186

153187
// Displays output to user and ensure the subprocess doesn't run into buffer overflow.
154188
// TODO: after a release, remove discovery output from the "Python Test Log" channel and send it to the "Python" channel instead.

‎src/client/testing/testController/unittest/testExecutionAdapter.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
4747
runInstance?: TestRun,
4848
executionFactory?: IPythonExecutionFactory,
4949
debugLauncher?: ITestDebugLauncher,
50-
): Promise<ExecutionTestPayload> {
50+
): Promise<void> {
5151
// deferredTillServerClose awaits named pipe server close
5252
const deferredTillServerClose: Deferred<void> = utils.createTestingDeferred();
5353

@@ -87,12 +87,6 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
8787
} finally {
8888
await deferredTillServerClose.promise;
8989
}
90-
const executionPayload: ExecutionTestPayload = {
91-
cwd: uri.fsPath,
92-
status: 'success',
93-
error: '',
94-
};
95-
return executionPayload;
9690
}
9791

9892
private async runTestsNew(

‎src/client/testing/testController/workspaceTestAdapter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ export class WorkspaceTestAdapter {
134134
try {
135135
// ** execution factory only defined for new rewrite way
136136
if (executionFactory !== undefined) {
137-
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, interpreter);
137+
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, token, interpreter);
138138
} else {
139139
await this.discoveryAdapter.discoverTests(this.workspaceUri);
140140
}

0 commit comments

Comments
 (0)