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

Discovery cancellation #24713

Merged
merged 8 commits into from
Jan 14, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
11 changes: 6 additions & 5 deletions src/client/testing/testController/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,19 @@ export interface ITestResultResolver {
}
export interface ITestDiscoveryAdapter {
// ** first line old method signature, second line new method signature
discoverTests(uri: Uri): Promise<DiscoveredTestPayload>;
discoverTests(uri: Uri): Promise<void>;
discoverTests(
uri: Uri,
executionFactory: IPythonExecutionFactory,
executionFactory?: IPythonExecutionFactory,
token?: CancellationToken,
interpreter?: PythonEnvironment,
): Promise<DiscoveredTestPayload>;
): Promise<void>;
}

// interface for execution/runner adapter
export interface ITestExecutionAdapter {
// ** first line old method signature, second line new method signature
runTests(uri: Uri, testIds: string[], profileKind?: boolean | TestRunProfileKind): Promise<ExecutionTestPayload>;
runTests(uri: Uri, testIds: string[], profileKind?: boolean | TestRunProfileKind): Promise<void>;
runTests(
uri: Uri,
testIds: string[],
Expand All @@ -176,7 +177,7 @@ export interface ITestExecutionAdapter {
executionFactory?: IPythonExecutionFactory,
debugLauncher?: ITestDebugLauncher,
interpreter?: PythonEnvironment,
): Promise<ExecutionTestPayload>;
): Promise<void>;
}

// Same types as in python_files/unittestadapter/utils.py
Expand Down
55 changes: 46 additions & 9 deletions src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import * as path from 'path';
import { Uri } from 'vscode';
import { CancellationToken, CancellationTokenSource, Uri } from 'vscode';
import * as fs from 'fs';
import { ChildProcess } from 'child_process';
import {
ExecutionFactoryCreateWithEnvironmentOptions,
IPythonExecutionFactory,
SpawnOptions,
} from '../../../common/process/types';
import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
import { Deferred } from '../../../common/utils/async';
import { createDeferred, Deferred } from '../../../common/utils/async';
import { EXTENSION_ROOT_DIR } from '../../../constants';
import { traceError, traceInfo, traceVerbose, traceWarn } from '../../../logging';
import { DiscoveredTestPayload, ITestDiscoveryAdapter, ITestResultResolver } from '../common/types';
Expand Down Expand Up @@ -40,24 +41,39 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
async discoverTests(
uri: Uri,
executionFactory?: IPythonExecutionFactory,
token?: CancellationToken,
interpreter?: PythonEnvironment,
): Promise<DiscoveredTestPayload> {
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
this.resultResolver?.resolveDiscovery(data);
): Promise<void> {
const cSource = new CancellationTokenSource();
const deferredReturn = createDeferred<void>();

token?.onCancellationRequested(() => {
traceInfo(`Test discovery cancelled.`);
cSource.cancel();
deferredReturn.resolve();
});

await this.runPytestDiscovery(uri, name, executionFactory, interpreter);
const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
// if the token is cancelled, we don't want process the data
if (!token?.isCancellationRequested) {
this.resultResolver?.resolveDiscovery(data);
}
}, cSource.token);

this.runPytestDiscovery(uri, name, cSource, executionFactory, interpreter, token).then(() => {
deferredReturn.resolve();
});

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

async runPytestDiscovery(
uri: Uri,
discoveryPipeName: string,
cSource: CancellationTokenSource,
executionFactory?: IPythonExecutionFactory,
interpreter?: PythonEnvironment,
token?: CancellationToken,
): Promise<void> {
const relativePathToPytest = 'python_files';
const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest);
Expand Down Expand Up @@ -111,6 +127,12 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
args: execArgs,
env: (mutableEnv as unknown) as { [key: string]: string },
});
token?.onCancellationRequested(() => {
traceInfo(`Test discovery cancelled, killing pytest subprocess for workspace ${uri.fsPath}`);
proc.kill();
deferredTillExecClose.resolve();
cSource.cancel();
});
proc.stdout.on('data', (data) => {
const out = fixLogLinesNoTrailing(data.toString());
traceInfo(out);
Expand Down Expand Up @@ -143,6 +165,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
throwOnStdErr: true,
outputChannel: this.outputChannel,
env: mutableEnv,
token,
};

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

const deferredTillExecClose: Deferred<void> = createTestingDeferred();

let resultProc: ChildProcess | undefined;

token?.onCancellationRequested(() => {
traceInfo(`Test discovery cancelled, killing pytest subprocess for workspace ${uri.fsPath}`);
// if the resultProc exists just call kill on it which will handle resolving the ExecClose deferred, otherwise resolve the deferred here.
if (resultProc) {
resultProc?.kill();
} else {
deferredTillExecClose.resolve();
cSource.cancel();
}
});
const result = execService?.execObservable(execArgs, spawnOptions);
resultProc = result?.proc;

// Take all output from the subprocess and add it to the test output channel. This will be the pytest output.
// Displays output to user and ensure the subprocess doesn't run into buffer overflow.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
executionFactory?: IPythonExecutionFactory,
debugLauncher?: ITestDebugLauncher,
interpreter?: PythonEnvironment,
): Promise<ExecutionTestPayload> {
): Promise<void> {
const deferredTillServerClose: Deferred<void> = utils.createTestingDeferred();

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

try {
Expand All @@ -82,15 +76,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
} finally {
await deferredTillServerClose.promise;
}

// placeholder until after the rewrite is adopted
// TODO: remove after adoption.
const executionPayload: ExecutionTestPayload = {
cwd: uri.fsPath,
status: 'success',
error: '',
};
return executionPayload;
}

private async runTestsNew(
Expand Down Expand Up @@ -244,7 +229,6 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter {
});

const result = execService?.execObservable(runArgs, spawnOptions);
resultProc = result?.proc;

// Take all output from the subprocess and add it to the test output channel. This will be the pytest output.
// Displays output to user and ensure the subprocess doesn't run into buffer overflow.
Expand Down
59 changes: 46 additions & 13 deletions src/client/testing/testController/unittest/testDiscoveryAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// Licensed under the MIT License.

import * as path from 'path';
import { Uri } from 'vscode';
import { CancellationTokenSource, Uri } from 'vscode';
import { CancellationToken } from 'vscode-jsonrpc';
import { ChildProcess } from 'child_process';
import { IConfigurationService, ITestOutputChannel } from '../../../common/types';
import { EXTENSION_ROOT_DIR } from '../../../constants';
import {
Expand Down Expand Up @@ -40,15 +42,30 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
private readonly envVarsService?: IEnvironmentVariablesProvider,
) {}

public async discoverTests(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise<DiscoveredTestPayload> {
public async discoverTests(
uri: Uri,
executionFactory?: IPythonExecutionFactory,
token?: CancellationToken,
): Promise<void> {
const settings = this.configSettings.getSettings(uri);
const { unittestArgs } = settings.testing;
const cwd = settings.testing.cwd && settings.testing.cwd.length > 0 ? settings.testing.cwd : uri.fsPath;

const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
this.resultResolver?.resolveDiscovery(data);
const cSource = new CancellationTokenSource();
const deferredReturn = createDeferred<void>();

token?.onCancellationRequested(() => {
traceInfo(`Test discovery cancelled.`);
cSource.cancel();
deferredReturn.resolve();
});

const name = await startDiscoveryNamedPipe((data: DiscoveredTestPayload) => {
if (!token?.isCancellationRequested) {
this.resultResolver?.resolveDiscovery(data);
}
}, cSource.token);

// set up env with the pipe name
let env: EnvironmentVariables | undefined = await this.envVarsService?.getEnvironmentVariables(uri);
if (env === undefined) {
Expand All @@ -62,24 +79,22 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
command,
cwd,
outChannel: this.outputChannel,
token,
};

try {
await this.runDiscovery(uri, options, name, cwd, executionFactory);
} finally {
// none
}
// placeholder until after the rewrite is adopted
// TODO: remove after adoption.
const discoveryPayload: DiscoveredTestPayload = { cwd, status: 'success' };
return discoveryPayload;
this.runDiscovery(uri, options, name, cwd, cSource, executionFactory).then(() => {
deferredReturn.resolve();
});

return deferredReturn.promise;
}

async runDiscovery(
uri: Uri,
options: TestCommandOptions,
testRunPipeName: string,
cwd: string,
cSource: CancellationTokenSource,
executionFactory?: IPythonExecutionFactory,
): Promise<void> {
// get and edit env vars
Expand All @@ -103,6 +118,12 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
args,
env: (mutableEnv as unknown) as { [key: string]: string },
});
options.token?.onCancellationRequested(() => {
traceInfo(`Test discovery cancelled, killing unittest subprocess for workspace ${uri.fsPath}`);
proc.kill();
deferredTillExecClose.resolve();
cSource.cancel();
});
proc.stdout.on('data', (data) => {
const out = fixLogLinesNoTrailing(data.toString());
traceInfo(out);
Expand Down Expand Up @@ -148,7 +169,19 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
};
const execService = await executionFactory?.createActivatedEnvironment(creationOptions);

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

// Displays output to user and ensure the subprocess doesn't run into buffer overflow.
// TODO: after a release, remove discovery output from the "Python Test Log" channel and send it to the "Python" channel instead.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
runInstance?: TestRun,
executionFactory?: IPythonExecutionFactory,
debugLauncher?: ITestDebugLauncher,
): Promise<ExecutionTestPayload> {
): Promise<void> {
// deferredTillServerClose awaits named pipe server close
const deferredTillServerClose: Deferred<void> = utils.createTestingDeferred();

Expand Down Expand Up @@ -87,12 +87,6 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter {
} finally {
await deferredTillServerClose.promise;
}
const executionPayload: ExecutionTestPayload = {
cwd: uri.fsPath,
status: 'success',
error: '',
};
return executionPayload;
}

private async runTestsNew(
Expand Down
2 changes: 1 addition & 1 deletion src/client/testing/testController/workspaceTestAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export class WorkspaceTestAdapter {
try {
// ** execution factory only defined for new rewrite way
if (executionFactory !== undefined) {
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, interpreter);
await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, token, interpreter);
} else {
await this.discoveryAdapter.discoverTests(this.workspaceUri);
}
Expand Down
Loading
Loading