Skip to content

Commit b313fc7

Browse files
eleanorjboydeleanorjboyd
committed
fixes based on comments
1 parent 07bce78 commit b313fc7

File tree

5 files changed

+76
-103
lines changed

5 files changed

+76
-103
lines changed

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

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,52 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
3-
import { CancellationTokenSource, Uri } from 'vscode';
3+
import { CancellationToken, CancellationTokenSource, Disposable, Uri } from 'vscode';
44
import { Deferred } from '../../../common/utils/async';
55
import { traceError, traceInfo, traceVerbose } from '../../../logging';
6-
import { createDiscoveryErrorPayload, fixLogLinesNoTrailing } from './utils';
7-
import { ITestResultResolver } from './types';
6+
import { createDiscoveryErrorPayload, fixLogLinesNoTrailing, startDiscoveryNamedPipe } from './utils';
7+
import { DiscoveredTestPayload, ITestResultResolver } from './types';
88

99
/**
1010
* Test provider type for logging purposes.
1111
*/
1212
export type TestProvider = 'pytest' | 'unittest';
1313

14+
/**
15+
* Sets up the discovery named pipe and wires up cancellation.
16+
* @param resultResolver The resolver to handle discovered test data
17+
* @param token Optional cancellation token from the caller
18+
* @param uri Workspace URI for logging
19+
* @returns Object containing the pipe name, cancellation source, and disposable for the external token handler
20+
*/
21+
export async function setupDiscoveryPipe(
22+
resultResolver: ITestResultResolver | undefined,
23+
token: CancellationToken | undefined,
24+
uri: Uri,
25+
): Promise<{ pipeName: string; cancellation: CancellationTokenSource; tokenDisposable: Disposable | undefined }> {
26+
const discoveryPipeCancellation = new CancellationTokenSource();
27+
28+
// Wire up cancellation from external token and store the disposable
29+
const tokenDisposable = token?.onCancellationRequested(() => {
30+
traceInfo(`Test discovery cancelled.`);
31+
discoveryPipeCancellation.cancel();
32+
});
33+
34+
// Start the named pipe with the discovery listener
35+
const discoveryPipeName = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
36+
if (!token?.isCancellationRequested) {
37+
resultResolver?.resolveDiscovery(data);
38+
}
39+
}, discoveryPipeCancellation.token);
40+
41+
traceVerbose(`Created discovery pipe: ${discoveryPipeName} for workspace ${uri.fsPath}`);
42+
43+
return {
44+
pipeName: discoveryPipeName,
45+
cancellation: discoveryPipeCancellation,
46+
tokenDisposable,
47+
};
48+
}
49+
1450
/**
1551
* Creates standard process event handlers for test discovery subprocess.
1652
* Handles stdout/stderr logging and error reporting on process exit.

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

Lines changed: 18 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
import * as path from 'path';
4-
import { CancellationToken, CancellationTokenSource, Uri } from 'vscode';
4+
import { CancellationToken, Uri } from 'vscode';
55
import { ChildProcess } from 'child_process';
66
import {
77
ExecutionFactoryCreateWithEnvironmentOptions,
@@ -12,48 +12,13 @@ import { IConfigurationService } from '../../../common/types';
1212
import { Deferred } from '../../../common/utils/async';
1313
import { EXTENSION_ROOT_DIR } from '../../../constants';
1414
import { traceError, traceInfo, traceVerbose } from '../../../logging';
15-
import { DiscoveredTestPayload, ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
16-
import { createTestingDeferred, startDiscoveryNamedPipe } from '../common/utils';
15+
import { ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
16+
import { createTestingDeferred } from '../common/utils';
1717
import { IEnvironmentVariablesProvider } from '../../../common/variables/types';
1818
import { PythonEnvironment } from '../../../pythonEnvironments/info';
1919
import { useEnvExtension, getEnvironment, runInBackground } from '../../../envExt/api.internal';
2020
import { buildPytestEnv as configureSubprocessEnv, handleSymlinkAndRootDir } from './pytestHelpers';
21-
import { cleanupOnCancellation, createProcessHandlers } from '../common/discoveryHelpers';
22-
23-
/**
24-
* Sets up the discovery named pipe and wires up cancellation.
25-
* @param resultResolver The resolver to handle discovered test data
26-
* @param token Optional cancellation token from the caller
27-
* @param uri Workspace URI for logging
28-
* @returns Object containing the pipe name and cancellation source
29-
*/
30-
async function setupDiscoveryPipe(
31-
resultResolver: ITestResultResolver | undefined,
32-
token: CancellationToken | undefined,
33-
uri: Uri,
34-
): Promise<{ pipeName: string; cancellation: CancellationTokenSource }> {
35-
const discoveryPipeCancellation = new CancellationTokenSource();
36-
37-
// Wire up cancellation from external token
38-
token?.onCancellationRequested(() => {
39-
traceInfo(`Test discovery cancelled.`);
40-
discoveryPipeCancellation.cancel();
41-
});
42-
43-
// Start the named pipe with the discovery listener
44-
const discoveryPipeName = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
45-
if (!token?.isCancellationRequested) {
46-
resultResolver?.resolveDiscovery(data);
47-
}
48-
}, discoveryPipeCancellation.token);
49-
50-
traceVerbose(`Created discovery pipe: ${discoveryPipeName} for workspace ${uri.fsPath}`);
51-
52-
return {
53-
pipeName: discoveryPipeName,
54-
cancellation: discoveryPipeCancellation,
55-
};
56-
}
21+
import { cleanupOnCancellation, createProcessHandlers, setupDiscoveryPipe } from '../common/discoveryHelpers';
5722

5823
/**
5924
* Configures the subprocess environment for pytest discovery.
@@ -69,7 +34,7 @@ async function configureDiscoveryEnv(
6934
): Promise<NodeJS.ProcessEnv> {
7035
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, 'python_files');
7136
const envVars = await envVarsService?.getEnvironmentVariables(uri);
72-
const mutableEnv = await configureSubprocessEnv(envVars, fullPluginPath, discoveryPipeName);
37+
const mutableEnv = configureSubprocessEnv(envVars, fullPluginPath, discoveryPipeName);
7338
return mutableEnv;
7439
}
7540

@@ -90,11 +55,11 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
9055
interpreter?: PythonEnvironment,
9156
): Promise<void> {
9257
// Setup discovery pipe and cancellation
93-
const { pipeName: discoveryPipeName, cancellation: discoveryPipeCancellation } = await setupDiscoveryPipe(
94-
this.resultResolver,
95-
token,
96-
uri,
97-
);
58+
const {
59+
pipeName: discoveryPipeName,
60+
cancellation: discoveryPipeCancellation,
61+
tokenDisposable,
62+
} = await setupDiscoveryPipe(this.resultResolver, token, uri);
9863

9964
// Setup process handlers deferred (used by both execution paths)
10065
const deferredTillExecClose: Deferred<void> = createTestingDeferred();
@@ -137,7 +102,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
137102
traceInfo(`Started pytest discovery subprocess (environment extension) for workspace ${uri.fsPath}`);
138103

139104
// Wire up cancellation and process events
140-
token?.onCancellationRequested(() => {
105+
const envExtCancellationHandler = token?.onCancellationRequested(() => {
141106
cleanupOnCancellation('pytest', proc, deferredTillExecClose, discoveryPipeCancellation, uri);
142107
});
143108
proc.stdout.on('data', handlers.onStdout);
@@ -148,6 +113,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
148113
});
149114

150115
await deferredTillExecClose.promise;
116+
envExtCancellationHandler?.dispose();
151117
traceInfo(`Pytest discovery completed for workspace ${uri.fsPath}`);
152118
return;
153119
}
@@ -185,9 +151,10 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
185151
};
186152

187153
let resultProc: ChildProcess | undefined;
154+
let cancellationHandler: import('vscode').Disposable | undefined;
188155

189-
// Set up cancellation handler before execObservable to catch early cancellations
190-
const cancellationHandler = token?.onCancellationRequested(() => {
156+
// Set up cancellation handler after all early return checks
157+
cancellationHandler = token?.onCancellationRequested(() => {
191158
traceInfo(`Cancellation requested during pytest discovery for workspace ${uri.fsPath}`);
192159
cleanupOnCancellation('pytest', resultProc, deferredTillExecClose, discoveryPipeCancellation, uri);
193160
});
@@ -199,6 +166,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
199166
if (!resultProc) {
200167
traceError(`Failed to spawn pytest discovery subprocess for workspace ${uri.fsPath}`);
201168
deferredTillExecClose.resolve();
169+
cancellationHandler?.dispose();
202170
return;
203171
}
204172
traceInfo(`Started pytest discovery subprocess (execution factory) for workspace ${uri.fsPath}`);
@@ -215,7 +183,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
215183

216184
cancellationHandler?.dispose();
217185

218-
// Check for early cancellation before awaitingre awaiting
186+
// Check for early cancellation before awaiting
219187
if (token?.isCancellationRequested) {
220188
traceInfo(`Pytest discovery was cancelled before process completion for workspace ${uri.fsPath}`);
221189
deferredTillExecClose.resolve();
@@ -230,6 +198,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
230198
throw error;
231199
} finally {
232200
traceVerbose(`Cleaning up pytest discovery resources for workspace ${uri.fsPath}`);
201+
tokenDisposable?.dispose();
233202
discoveryPipeCancellation.dispose();
234203
}
235204
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export async function handleSymlinkAndRootDir(cwd: string, pytestArgs: string[])
3030
}
3131
// if user has provided `--rootdir` then use that, otherwise add `cwd`
3232
// root dir is required so pytest can find the relative paths and for symlinks
33-
addValueIfKeyNotExist(pytestArgs, '--rootdir', cwd);
33+
pytestArgs = addValueIfKeyNotExist(pytestArgs, '--rootdir', cwd);
3434
return pytestArgs;
3535
}
3636

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

Lines changed: 16 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,59 +1,23 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import { CancellationToken, CancellationTokenSource, Uri } from 'vscode';
4+
import { CancellationToken, Uri } from 'vscode';
55
import { ChildProcess } from 'child_process';
66
import { IConfigurationService } from '../../../common/types';
77
import { EXTENSION_ROOT_DIR } from '../../../constants';
8-
import { DiscoveredTestPayload, ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
8+
import { ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
99
import { IEnvironmentVariablesProvider } from '../../../common/variables/types';
1010
import {
1111
ExecutionFactoryCreateWithEnvironmentOptions,
1212
IPythonExecutionFactory,
1313
SpawnOptions,
1414
} from '../../../common/process/types';
15-
import { startDiscoveryNamedPipe } from '../common/utils';
1615
import { traceError, traceInfo, traceVerbose } from '../../../logging';
1716
import { getEnvironment, runInBackground, useEnvExtension } from '../../../envExt/api.internal';
1817
import { PythonEnvironment } from '../../../pythonEnvironments/info';
1918
import { createTestingDeferred } from '../common/utils';
2019
import { buildDiscoveryCommand, buildUnittestEnv as configureSubprocessEnv } from './unittestHelpers';
21-
import { cleanupOnCancellation, createProcessHandlers } from '../common/discoveryHelpers';
22-
23-
/**
24-
* Sets up the discovery named pipe and wires up cancellation.
25-
* @param resultResolver The resolver to handle discovered test data
26-
* @param token Optional cancellation token from the caller
27-
* @param uri Workspace URI for logging
28-
* @returns Object containing the pipe name and cancellation source
29-
*/
30-
async function setupDiscoveryPipe(
31-
resultResolver: ITestResultResolver | undefined,
32-
token: CancellationToken | undefined,
33-
uri: Uri,
34-
): Promise<{ pipeName: string; cancellation: CancellationTokenSource }> {
35-
const discoveryPipeCancellation = new CancellationTokenSource();
36-
37-
// Wire up cancellation from external token
38-
token?.onCancellationRequested(() => {
39-
traceInfo(`Test discovery cancelled.`);
40-
discoveryPipeCancellation.cancel();
41-
});
42-
43-
// Start the named pipe with the discovery listener
44-
const discoveryPipeName = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
45-
if (!token?.isCancellationRequested) {
46-
resultResolver?.resolveDiscovery(data);
47-
}
48-
}, discoveryPipeCancellation.token);
49-
50-
traceVerbose(`Created discovery pipe: ${discoveryPipeName} for workspace ${uri.fsPath}`);
51-
52-
return {
53-
pipeName: discoveryPipeName,
54-
cancellation: discoveryPipeCancellation,
55-
};
56-
}
20+
import { cleanupOnCancellation, createProcessHandlers, setupDiscoveryPipe } from '../common/discoveryHelpers';
5721

5822
/**
5923
* Configures the subprocess environment for unittest discovery.
@@ -68,7 +32,7 @@ async function configureDiscoveryEnv(
6832
discoveryPipeName: string,
6933
): Promise<NodeJS.ProcessEnv> {
7034
const envVars = await envVarsService?.getEnvironmentVariables(uri);
71-
const mutableEnv = await configureSubprocessEnv(envVars, discoveryPipeName);
35+
const mutableEnv = configureSubprocessEnv(envVars, discoveryPipeName);
7236
return mutableEnv;
7337
}
7438

@@ -89,11 +53,11 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
8953
interpreter?: PythonEnvironment,
9054
): Promise<void> {
9155
// Setup discovery pipe and cancellation
92-
const { pipeName: discoveryPipeName, cancellation: discoveryPipeCancellation } = await setupDiscoveryPipe(
93-
this.resultResolver,
94-
token,
95-
uri,
96-
);
56+
const {
57+
pipeName: discoveryPipeName,
58+
cancellation: discoveryPipeCancellation,
59+
tokenDisposable,
60+
} = await setupDiscoveryPipe(this.resultResolver, token, uri);
9761

9862
// Setup process handlers deferred (used by both execution paths)
9963
const deferredTillExecClose = createTestingDeferred();
@@ -133,7 +97,7 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
13397
traceInfo(`Started unittest discovery subprocess (environment extension) for workspace ${uri.fsPath}`);
13498

13599
// Wire up cancellation and process events
136-
token?.onCancellationRequested(() => {
100+
const envExtCancellationHandler = token?.onCancellationRequested(() => {
137101
cleanupOnCancellation('unittest', proc, deferredTillExecClose, discoveryPipeCancellation, uri);
138102
});
139103
proc.stdout.on('data', handlers.onStdout);
@@ -144,6 +108,7 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
144108
});
145109

146110
await deferredTillExecClose.promise;
111+
envExtCancellationHandler?.dispose();
147112
traceInfo(`Unittest discovery completed for workspace ${uri.fsPath}`);
148113
return;
149114
}
@@ -181,9 +146,10 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
181146
};
182147

183148
let resultProc: ChildProcess | undefined;
149+
let cancellationHandler: import('vscode').Disposable | undefined;
184150

185-
// Set up cancellation handler before execObservable to catch early cancellations
186-
const cancellationHandler = token?.onCancellationRequested(() => {
151+
// Set up cancellation handler after all early return checks
152+
cancellationHandler = token?.onCancellationRequested(() => {
187153
traceInfo(`Cancellation requested during unittest discovery for workspace ${uri.fsPath}`);
188154
cleanupOnCancellation('unittest', resultProc, deferredTillExecClose, discoveryPipeCancellation, uri);
189155
});
@@ -195,6 +161,7 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
195161
if (!resultProc) {
196162
traceError(`Failed to spawn unittest discovery subprocess for workspace ${uri.fsPath}`);
197163
deferredTillExecClose.resolve();
164+
cancellationHandler?.dispose();
198165
return;
199166
}
200167
traceInfo(`Started unittest discovery subprocess (execution factory) for workspace ${uri.fsPath}`);
@@ -226,6 +193,7 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
226193
throw error;
227194
} finally {
228195
traceVerbose(`Cleaning up unittest discovery resources for workspace ${uri.fsPath}`);
196+
tokenDisposable?.dispose();
229197
discoveryPipeCancellation.dispose();
230198
}
231199
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@ import { traceInfo } from '../../../logging';
77
* Builds the environment variables required for unittest discovery.
88
* Sets TEST_RUN_PIPE for communication.
99
*/
10-
export async function buildUnittestEnv(
10+
export function buildUnittestEnv(
1111
envVars: { [key: string]: string | undefined } | undefined,
1212
discoveryPipeName: string,
13-
): Promise<{ [key: string]: string | undefined }> {
13+
): { [key: string]: string | undefined } {
1414
const mutableEnv = {
1515
...envVars,
1616
};

0 commit comments

Comments
 (0)