From b0d84727763c882b62b9beb7c9bf7e3bff37fd08 Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Tue, 16 May 2023 10:02:09 -0700 Subject: [PATCH 1/6] switch rewrite to be behind an experiment --- src/client/common/experiments/groups.ts | 4 ++++ .../testing/testController/common/utils.ts | 9 +++++++++ .../testing/testController/controller.ts | 19 +++++++++++-------- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index 5884aafd122d..1ee06469095c 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -14,3 +14,7 @@ export enum TerminalEnvVarActivation { export enum ShowFormatterExtensionPrompt { experiment = 'pythonPromptNewFormatterExt', } +// Experiment to enable the new testing rewrite. +export enum EnableTestAdapterRewrite { + experiment = 'pythonTestAdapter', +} diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index e0bad383d695..255a83003531 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -1,6 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import { EnableTestAdapterRewrite } from '../../../common/experiments/groups'; +import { IExperimentService } from '../../../common/types'; +import { IServiceContainer } from '../../../ioc/types'; + export function fixLogLines(content: string): string { const lines = content.split(/\r?\n/g); return `${lines.join('\r\n')}\r\n`; @@ -50,3 +54,8 @@ export function jsonRPCContent(headers: Map, rawData: string): I remainingRawData, }; } + +export function pythonTestAdapterRewriteEnabled(serviceContainer: IServiceContainer): boolean { + const experiment = serviceContainer.get(IExperimentService); + return experiment.inExperimentSync(EnableTestAdapterRewrite.experiment); +} diff --git a/src/client/testing/testController/controller.ts b/src/client/testing/testController/controller.ts index 58eaa9c890d6..aac4925137f0 100644 --- a/src/client/testing/testController/controller.ts +++ b/src/client/testing/testController/controller.ts @@ -44,6 +44,8 @@ import { PytestTestDiscoveryAdapter } from './pytest/pytestDiscoveryAdapter'; import { PytestTestExecutionAdapter } from './pytest/pytestExecutionAdapter'; import { WorkspaceTestAdapter } from './workspaceTestAdapter'; import { ITestDebugLauncher } from '../common/types'; +import { pythonTestAdapterRewriteEnabled } from './common/utils'; +import { IServiceContainer } from '../../ioc/types'; // Types gymnastics to make sure that sendTriggerTelemetry only accepts the correct types. type EventPropertyType = IEventNamePropertyMapping[EventName.UNITTEST_DISCOVERY_TRIGGER]; @@ -93,6 +95,7 @@ export class PythonTestController implements ITestController, IExtensionSingleAc @inject(IPythonExecutionFactory) private readonly pythonExecFactory: IPythonExecutionFactory, @inject(ITestDebugLauncher) private readonly debugLauncher: ITestDebugLauncher, @inject(ITestOutputChannel) private readonly testOutputChannel: ITestOutputChannel, + @inject(IServiceContainer) private readonly serviceContainer: IServiceContainer, ) { this.refreshCancellation = new CancellationTokenSource(); @@ -242,11 +245,11 @@ export class PythonTestController implements ITestController, IExtensionSingleAc const settings = this.configSettings.getSettings(uri); traceVerbose(`Testing: Refreshing test data for ${uri.fsPath}`); const rewriteTestingEnabled = process.env.ENABLE_PYTHON_TESTING_REWRITE; - if (settings.testing.pytestEnabled) { + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { // Ensure we send test telemetry if it gets disabled again this.sendTestDisabledTelemetry = true; + // ** experiment to roll out NEW test discovery mechanism if (rewriteTestingEnabled) { - // ** rewriteTestingEnabled set to true to use NEW test discovery mechanism const workspace = this.workspaceService.getWorkspaceFolder(uri); traceVerbose(`Discover tests for workspace name: ${workspace?.name} - uri: ${uri.fsPath}`); const testAdapter = @@ -265,8 +268,8 @@ export class PythonTestController implements ITestController, IExtensionSingleAc } else if (settings.testing.unittestEnabled) { // ** Ensure we send test telemetry if it gets disabled again this.sendTestDisabledTelemetry = true; - if (rewriteTestingEnabled) { - // ** rewriteTestingEnabled set to true to use NEW test discovery mechanism + // ** experiment to roll out NEW test discovery mechanism + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { const workspace = this.workspaceService.getWorkspaceFolder(uri); traceVerbose(`Discover tests for workspace name: ${workspace?.name} - uri: ${uri.fsPath}`); const testAdapter = @@ -392,12 +395,12 @@ export class PythonTestController implements ITestController, IExtensionSingleAc const settings = this.configSettings.getSettings(workspace.uri); if (testItems.length > 0) { const rewriteTestingEnabled = process.env.ENABLE_PYTHON_TESTING_REWRITE; - if (settings.testing.pytestEnabled) { + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { sendTelemetryEvent(EventName.UNITTEST_RUN, undefined, { tool: 'pytest', debugging: request.profile?.kind === TestRunProfileKind.Debug, }); - // ** rewriteTestingEnabled set to true to use NEW test discovery mechanism + // ** experiment to roll out NEW test discovery mechanism if (rewriteTestingEnabled) { const testAdapter = this.testAdapters.get(workspace.uri) || @@ -428,8 +431,8 @@ export class PythonTestController implements ITestController, IExtensionSingleAc tool: 'unittest', debugging: request.profile?.kind === TestRunProfileKind.Debug, }); - // ** rewriteTestingEnabled set to true to use NEW test discovery mechanism - if (rewriteTestingEnabled) { + // ** experiment to roll out NEW test discovery mechanism + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { const testAdapter = this.testAdapters.get(workspace.uri) || (this.testAdapters.values().next().value as WorkspaceTestAdapter); From e03d8e7183779d558dcdd10fb6b78dd2418d90c0 Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Tue, 16 May 2023 11:29:00 -0700 Subject: [PATCH 2/6] fix placement of rewrite experiment check --- src/client/testing/common/debugLauncher.ts | 17 ++++++++++------- src/client/testing/testController/controller.ts | 10 ++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/client/testing/common/debugLauncher.ts b/src/client/testing/common/debugLauncher.ts index 5b39bd97a740..3d20816eddfe 100644 --- a/src/client/testing/common/debugLauncher.ts +++ b/src/client/testing/common/debugLauncher.ts @@ -16,6 +16,7 @@ import { getConfigurationsForWorkspace } from '../../debugger/extension/configur import { getWorkspaceFolder, getWorkspaceFolders } from '../../common/vscodeApis/workspaceApis'; import { showErrorMessage } from '../../common/vscodeApis/windowApis'; import { createDeferred } from '../../common/utils/async'; +import { pythonTestAdapterRewriteEnabled } from '../testController/common/utils'; @injectable() export class DebugLauncher implements ITestDebugLauncher { @@ -40,6 +41,7 @@ export class DebugLauncher implements ITestDebugLauncher { options, workspaceFolder, this.configService.getSettings(workspaceFolder.uri), + this.serviceContainer, ); const debugManager = this.serviceContainer.get(IDebugService); @@ -70,6 +72,7 @@ export class DebugLauncher implements ITestDebugLauncher { options: LaunchOptions, workspaceFolder: WorkspaceFolder, configSettings: IPythonSettings, + serviceContainer?: IServiceContainer, ): Promise { let debugConfig = await DebugLauncher.readDebugConfig(workspaceFolder); if (!debugConfig) { @@ -89,7 +92,7 @@ export class DebugLauncher implements ITestDebugLauncher { }); DebugLauncher.applyDefaults(debugConfig!, workspaceFolder, configSettings); - return this.convertConfigToArgs(debugConfig!, workspaceFolder, options); + return this.convertConfigToArgs(debugConfig!, workspaceFolder, options, serviceContainer); } public async readAllDebugConfigs(workspace: WorkspaceFolder): Promise { @@ -170,11 +173,12 @@ export class DebugLauncher implements ITestDebugLauncher { debugConfig: LaunchRequestArguments, workspaceFolder: WorkspaceFolder, options: LaunchOptions, + serviceContainer?: IServiceContainer, ): Promise { const configArgs = debugConfig as LaunchRequestArguments; const testArgs = options.testProvider === 'unittest' ? options.args.filter((item) => item !== '--debug') : options.args; - const script = DebugLauncher.getTestLauncherScript(options.testProvider); + const script = DebugLauncher.getTestLauncherScript(options.testProvider, serviceContainer); const args = script(testArgs); const [program] = args; configArgs.program = program; @@ -204,7 +208,7 @@ export class DebugLauncher implements ITestDebugLauncher { throw Error(`Invalid debug config "${debugConfig.name}"`); } launchArgs.request = 'launch'; - if (options.testProvider === 'pytest' && rewriteTestingEnabled) { + if (options.testProvider === 'pytest' && pythonTestAdapterRewriteEnabled(this.serviceContainer)) { if (options.pytestPort && options.pytestUUID) { launchArgs.env = { ...launchArgs.env, @@ -227,17 +231,16 @@ export class DebugLauncher implements ITestDebugLauncher { return launchArgs; } - private static getTestLauncherScript(testProvider: TestProvider) { - const rewriteTestingEnabled = process.env.ENABLE_PYTHON_TESTING_REWRITE; + private static getTestLauncherScript(testProvider: TestProvider, serviceContainer?: IServiceContainer) { switch (testProvider) { case 'unittest': { - if (rewriteTestingEnabled) { + if (serviceContainer && pythonTestAdapterRewriteEnabled(serviceContainer)) { return internalScripts.execution_py_testlauncher; // this is the new way to run unittest execution, debugger } return internalScripts.visualstudio_py_testlauncher; // old way unittest execution, debugger } case 'pytest': { - if (rewriteTestingEnabled) { + if (serviceContainer && pythonTestAdapterRewriteEnabled(serviceContainer)) { return (testArgs: string[]) => testArgs; } return internalScripts.testlauncher; // old way pytest execution, debugger diff --git a/src/client/testing/testController/controller.ts b/src/client/testing/testController/controller.ts index aac4925137f0..2e316cce462c 100644 --- a/src/client/testing/testController/controller.ts +++ b/src/client/testing/testController/controller.ts @@ -244,12 +244,11 @@ export class PythonTestController implements ITestController, IExtensionSingleAc if (uri) { const settings = this.configSettings.getSettings(uri); traceVerbose(`Testing: Refreshing test data for ${uri.fsPath}`); - const rewriteTestingEnabled = process.env.ENABLE_PYTHON_TESTING_REWRITE; - if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { + if (settings.testing.pytestEnabled) { // Ensure we send test telemetry if it gets disabled again this.sendTestDisabledTelemetry = true; // ** experiment to roll out NEW test discovery mechanism - if (rewriteTestingEnabled) { + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { const workspace = this.workspaceService.getWorkspaceFolder(uri); traceVerbose(`Discover tests for workspace name: ${workspace?.name} - uri: ${uri.fsPath}`); const testAdapter = @@ -394,14 +393,13 @@ export class PythonTestController implements ITestController, IExtensionSingleAc const settings = this.configSettings.getSettings(workspace.uri); if (testItems.length > 0) { - const rewriteTestingEnabled = process.env.ENABLE_PYTHON_TESTING_REWRITE; - if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { + if (settings.testing.pytestEnabled) { sendTelemetryEvent(EventName.UNITTEST_RUN, undefined, { tool: 'pytest', debugging: request.profile?.kind === TestRunProfileKind.Debug, }); // ** experiment to roll out NEW test discovery mechanism - if (rewriteTestingEnabled) { + if (pythonTestAdapterRewriteEnabled(this.serviceContainer)) { const testAdapter = this.testAdapters.get(workspace.uri) || (this.testAdapters.values().next().value as WorkspaceTestAdapter); From 188b4612a23c50dddd0bc83ad983cfc58d60167f Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Tue, 16 May 2023 13:20:29 -0700 Subject: [PATCH 3/6] switch to bool --- src/client/testing/common/debugLauncher.ts | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/client/testing/common/debugLauncher.ts b/src/client/testing/common/debugLauncher.ts index 3d20816eddfe..f9fc72bdcbdf 100644 --- a/src/client/testing/common/debugLauncher.ts +++ b/src/client/testing/common/debugLauncher.ts @@ -41,7 +41,6 @@ export class DebugLauncher implements ITestDebugLauncher { options, workspaceFolder, this.configService.getSettings(workspaceFolder.uri), - this.serviceContainer, ); const debugManager = this.serviceContainer.get(IDebugService); @@ -72,7 +71,6 @@ export class DebugLauncher implements ITestDebugLauncher { options: LaunchOptions, workspaceFolder: WorkspaceFolder, configSettings: IPythonSettings, - serviceContainer?: IServiceContainer, ): Promise { let debugConfig = await DebugLauncher.readDebugConfig(workspaceFolder); if (!debugConfig) { @@ -90,9 +88,10 @@ export class DebugLauncher implements ITestDebugLauncher { path: path.join(EXTENSION_ROOT_DIR, 'pythonFiles'), include: false, }); + DebugLauncher.applyDefaults(debugConfig!, workspaceFolder, configSettings); - return this.convertConfigToArgs(debugConfig!, workspaceFolder, options, serviceContainer); + return this.convertConfigToArgs(debugConfig!, workspaceFolder, options); } public async readAllDebugConfigs(workspace: WorkspaceFolder): Promise { @@ -173,12 +172,12 @@ export class DebugLauncher implements ITestDebugLauncher { debugConfig: LaunchRequestArguments, workspaceFolder: WorkspaceFolder, options: LaunchOptions, - serviceContainer?: IServiceContainer, ): Promise { + const pythonTestAdapterRewriteExperiment = pythonTestAdapterRewriteEnabled(this.serviceContainer); const configArgs = debugConfig as LaunchRequestArguments; const testArgs = options.testProvider === 'unittest' ? options.args.filter((item) => item !== '--debug') : options.args; - const script = DebugLauncher.getTestLauncherScript(options.testProvider, serviceContainer); + const script = DebugLauncher.getTestLauncherScript(options.testProvider, pythonTestAdapterRewriteExperiment); const args = script(testArgs); const [program] = args; configArgs.program = program; @@ -208,7 +207,7 @@ export class DebugLauncher implements ITestDebugLauncher { throw Error(`Invalid debug config "${debugConfig.name}"`); } launchArgs.request = 'launch'; - if (options.testProvider === 'pytest' && pythonTestAdapterRewriteEnabled(this.serviceContainer)) { + if (options.testProvider === 'pytest' && pythonTestAdapterRewriteExperiment) { if (options.pytestPort && options.pytestUUID) { launchArgs.env = { ...launchArgs.env, @@ -231,16 +230,16 @@ export class DebugLauncher implements ITestDebugLauncher { return launchArgs; } - private static getTestLauncherScript(testProvider: TestProvider, serviceContainer?: IServiceContainer) { + private static getTestLauncherScript(testProvider: TestProvider, pythonTestAdapterRewriteExperiment?: boolean) { switch (testProvider) { case 'unittest': { - if (serviceContainer && pythonTestAdapterRewriteEnabled(serviceContainer)) { + if (pythonTestAdapterRewriteExperiment) { return internalScripts.execution_py_testlauncher; // this is the new way to run unittest execution, debugger } return internalScripts.visualstudio_py_testlauncher; // old way unittest execution, debugger } case 'pytest': { - if (serviceContainer && pythonTestAdapterRewriteEnabled(serviceContainer)) { + if (pythonTestAdapterRewriteExperiment) { return (testArgs: string[]) => testArgs; } return internalScripts.testlauncher; // old way pytest execution, debugger From 95dd4a86c7d7574b7fdc7a18054d8a9e109a4416 Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Tue, 16 May 2023 14:55:33 -0700 Subject: [PATCH 4/6] fix tests --- .../testing/common/debugLauncher.unit.test.ts | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/test/testing/common/debugLauncher.unit.test.ts b/src/test/testing/common/debugLauncher.unit.test.ts index b8b7d5c55130..41b95c66040e 100644 --- a/src/test/testing/common/debugLauncher.unit.test.ts +++ b/src/test/testing/common/debugLauncher.unit.test.ts @@ -29,6 +29,7 @@ import { ITestingSettings } from '../../../client/testing/configuration/types'; import { TestProvider } from '../../../client/testing/types'; import { isOs, OSType } from '../../common'; import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types'; +import * as util from '../../../client/testing/testController/common/utils'; use(chaiAsPromised); @@ -45,6 +46,7 @@ suite('Unit Tests - Debug Launcher', () => { let getWorkspaceFoldersStub: sinon.SinonStub; let pathExistsStub: sinon.SinonStub; let readFileStub: sinon.SinonStub; + let pythonTestAdapterRewriteEnabledStub: sinon.SinonStub; const envVars = { FOO: 'BAR' }; setup(async () => { @@ -65,6 +67,8 @@ suite('Unit Tests - Debug Launcher', () => { getWorkspaceFoldersStub = sinon.stub(workspaceApis, 'getWorkspaceFolders'); pathExistsStub = sinon.stub(fs, 'pathExists'); readFileStub = sinon.stub(fs, 'readFile'); + pythonTestAdapterRewriteEnabledStub = sinon.stub(util, 'pythonTestAdapterRewriteEnabled'); + pythonTestAdapterRewriteEnabledStub.returns(false); const appShell = TypeMoq.Mock.ofType(undefined, TypeMoq.MockBehavior.Strict); appShell.setup((a) => a.showErrorMessage(TypeMoq.It.isAny())).returns(() => Promise.resolve(undefined)); @@ -143,19 +147,22 @@ suite('Unit Tests - Debug Launcher', () => { uri: Uri.file(folderPath), }; } - function getTestLauncherScript(testProvider: TestProvider) { - switch (testProvider) { - case 'unittest': { - return path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'visualstudio_py_testlauncher.py'); - } - case 'pytest': { - return path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'testlauncher.py'); - } - default: { - throw new Error(`Unknown test provider '${testProvider}'`); + function getTestLauncherScript(testProvider: TestProvider, pythonTestAdapterRewriteExperiment?: boolean) { + if (!pythonTestAdapterRewriteExperiment) { + switch (testProvider) { + case 'unittest': { + return path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'visualstudio_py_testlauncher.py'); + } + case 'pytest': { + return path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'testlauncher.py'); + } + default: { + throw new Error(`Unknown test provider '${testProvider}'`); + } } } } + function getDefaultDebugConfig(): DebugConfiguration { return { name: 'Debug Unit Test', @@ -178,7 +185,7 @@ suite('Unit Tests - Debug Launcher', () => { expected?: DebugConfiguration, debugConfigs?: string | DebugConfiguration[], ) { - const testLaunchScript = getTestLauncherScript(testProvider); + const testLaunchScript = getTestLauncherScript(testProvider, false); const workspaceFolders = [createWorkspaceFolder(options.cwd), createWorkspaceFolder('five/six/seven')]; getWorkspaceFoldersStub.returns(workspaceFolders); From 1faad0a4ed8a8ee7d2514f637311f0b1ca1188b4 Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Tue, 16 May 2023 15:39:27 -0700 Subject: [PATCH 5/6] add run failure telemetry --- .../testController/pytest/pytestController.ts | 31 +++++++++-------- .../unittest/unittestController.ts | 33 +++++++++++-------- 2 files changed, 37 insertions(+), 27 deletions(-) diff --git a/src/client/testing/testController/pytest/pytestController.ts b/src/client/testing/testController/pytest/pytestController.ts index 793170231210..997e3e29b7ec 100644 --- a/src/client/testing/testController/pytest/pytestController.ts +++ b/src/client/testing/testController/pytest/pytestController.ts @@ -285,19 +285,24 @@ export class PytestController implements ITestFrameworkController { public runTests(testRun: ITestRun, workspace: WorkspaceFolder, token: CancellationToken): Promise { const settings = this.configService.getSettings(workspace.uri); - return this.runner.runTests( - testRun, - { - workspaceFolder: workspace.uri, - cwd: - settings.testing.cwd && settings.testing.cwd.length > 0 - ? settings.testing.cwd - : workspace.uri.fsPath, - token, - args: settings.testing.pytestArgs, - }, - this.idToRawData, - ); + try { + return this.runner.runTests( + testRun, + { + workspaceFolder: workspace.uri, + cwd: + settings.testing.cwd && settings.testing.cwd.length > 0 + ? settings.testing.cwd + : workspace.uri.fsPath, + token, + args: settings.testing.pytestArgs, + }, + this.idToRawData, + ); + } catch (ex) { + sendTelemetryEvent(EventName.UNITTEST_RUN_ALL_FAILED, undefined); + throw new Error(`Failed to run tests: ${ex}`); + } } } diff --git a/src/client/testing/testController/unittest/unittestController.ts b/src/client/testing/testController/unittest/unittestController.ts index ee79103c4e3e..a795620f3ca0 100644 --- a/src/client/testing/testController/unittest/unittestController.ts +++ b/src/client/testing/testController/unittest/unittestController.ts @@ -251,20 +251,25 @@ export class UnittestController implements ITestFrameworkController { testController?: TestController, ): Promise { const settings = this.configService.getSettings(workspace.uri); - return this.runner.runTests( - testRun, - { - workspaceFolder: workspace.uri, - cwd: - settings.testing.cwd && settings.testing.cwd.length > 0 - ? settings.testing.cwd - : workspace.uri.fsPath, - token, - args: settings.testing.unittestArgs, - }, - this.idToRawData, - testController, - ); + try { + return this.runner.runTests( + testRun, + { + workspaceFolder: workspace.uri, + cwd: + settings.testing.cwd && settings.testing.cwd.length > 0 + ? settings.testing.cwd + : workspace.uri.fsPath, + token, + args: settings.testing.unittestArgs, + }, + this.idToRawData, + testController, + ); + } catch (ex) { + sendTelemetryEvent(EventName.UNITTEST_RUN_ALL_FAILED, undefined); + throw new Error(`Failed to run tests: ${ex}`); + } } } From 0b23a0a10fc297c544ac497467aa529d9a8d0275 Mon Sep 17 00:00:00 2001 From: eleanorjboyd Date: Thu, 25 May 2023 10:18:55 -0700 Subject: [PATCH 6/6] format fix --- src/client/testing/testController/common/utils.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 5e3361215c2e..1bf31e80e11a 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -57,7 +57,6 @@ export function jsonRPCContent(headers: Map, rawData: string): I }; } - export function pythonTestAdapterRewriteEnabled(serviceContainer: IServiceContainer): boolean { const experiment = serviceContainer.get(IExperimentService); return experiment.inExperimentSync(EnableTestAdapterRewrite.experiment); @@ -98,4 +97,3 @@ export const startServer = (testIds: string): Promise => reject(error); }); }); -