From 83c2f6743996810f20787287df370e5bbc4acb31 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Tue, 1 Oct 2024 23:49:03 -0700 Subject: [PATCH 1/5] add TODO --- src/client/common/process/pythonExecutionFactory.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index d8401a603d03..645d6aa3c151 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -115,13 +115,15 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { pythonPath: options.interpreter ? options.interpreter.path : undefined, }); } + // TODO: Try passing actual interpreter when calling this from Pytest const pythonPath = options.interpreter ? options.interpreter.path - : this.configService.getSettings(options.resource).pythonPath; + : this.configService.getSettings(options.resource).pythonPath; // : this.configService.getSettings(options.resource).pythonPath; MIGHT BE A PROBLEM. const processService: IProcessService = new ProcessService({ ...envVars }); processService.on('exec', this.logger.logProcess.bind(this.logger)); this.disposables.push(processService); + // TODO: Switch ordering on pixi and conda to see if that helps selecting right conda environment to run. --> Looks like done? const condaExecutionService = await this.createCondaExecutionService(pythonPath, processService); if (condaExecutionService) { return condaExecutionService; From 7323bde0463f349f6cb76b59df7036a0609ec566 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 3 Oct 2024 10:56:47 -0700 Subject: [PATCH 2/5] pass in correct interpreter for pytest --- .../testController/pytest/pytestDiscoveryAdapter.ts | 6 ++++++ .../testController/pytest/pytestExecutionAdapter.ts | 5 +++++ src/test/testing/common/testingAdapter.test.ts | 9 ++++++++- .../pytest/pytestDiscoveryAdapter.unit.test.ts | 3 ++- 4 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts index dd7bc9b21847..0e5749efac03 100644 --- a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts +++ b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts @@ -24,6 +24,7 @@ import { hasSymlinkParent, } from '../common/utils'; import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; +import { IInterpreterService } from '../../../interpreter/contracts'; /** * Wrapper class for unittest test discovery. This is where we call `runTestCommand`. #this seems incorrectly copied @@ -34,6 +35,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { private readonly outputChannel: ITestOutputChannel, private readonly resultResolver?: ITestResultResolver, private readonly envVarsService?: IEnvironmentVariablesProvider, + private readonly interpreterService?: IInterpreterService, ) {} async discoverTests(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise { @@ -102,11 +104,15 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { env: mutableEnv, }; + const interpreter = await this.interpreterService?.getActiveInterpreter(); + // Create the Python environment in which to execute the command. const creationOptions: ExecutionFactoryCreateWithEnvironmentOptions = { allowEnvironmentFetchExceptions: false, resource: uri, + interpreter, }; + const execService = await executionFactory?.createActivatedEnvironment(creationOptions); // delete UUID following entire discovery finishing. const execArgs = ['-m', 'pytest', '-p', 'vscode_pytest', '--collect-only'].concat(pytestArgs); diff --git a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts index bfaaab9d6586..54344431dc8f 100644 --- a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts +++ b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts @@ -25,6 +25,7 @@ import { PYTEST_PROVIDER } from '../../common/constants'; import { EXTENSION_ROOT_DIR } from '../../../common/constants'; import * as utils from '../common/utils'; import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; +import { IInterpreterService } from '../../../interpreter/contracts'; export class PytestTestExecutionAdapter implements ITestExecutionAdapter { constructor( @@ -32,6 +33,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { private readonly outputChannel: ITestOutputChannel, private readonly resultResolver?: ITestResultResolver, private readonly envVarsService?: IEnvironmentVariablesProvider, + private readonly interpreterService?: IInterpreterService, ) {} async runTests( @@ -131,10 +133,13 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { } const debugBool = profileKind && profileKind === TestRunProfileKind.Debug; + const interpreter = await this.interpreterService?.getActiveInterpreter(); + // Create the Python environment in which to execute the command. const creationOptions: ExecutionFactoryCreateWithEnvironmentOptions = { allowEnvironmentFetchExceptions: false, resource: uri, + interpreter, }; // need to check what will happen in the exec service is NOT defined and is null const execService = await executionFactory?.createActivatedEnvironment(creationOptions); diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index dcd45b2e56bc..0bdb741f89b9 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -22,8 +22,10 @@ import { TestProvider } from '../../../client/testing/types'; import { PYTEST_PROVIDER, UNITTEST_PROVIDER } from '../../../client/testing/common/constants'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; import { createTypeMoq } from '../../mocks/helper'; +import { IInterpreterService } from '../../../client/interpreter/contracts'; suite('End to End Tests: test adapters', () => { + let interpreterService: IInterpreterService; let resultResolver: ITestResultResolver; let pythonExecFactory: IPythonExecutionFactory; let configService: IConfigurationService; @@ -109,7 +111,7 @@ suite('End to End Tests: test adapters', () => { pythonExecFactory = serviceContainer.get(IPythonExecutionFactory); testController = serviceContainer.get(ITestController); envVarsService = serviceContainer.get(IEnvironmentVariablesProvider); - + interpreterService = serviceContainer.get(IInterpreterService); // create objects that were not injected testOutputChannel = createTypeMoq(); @@ -271,6 +273,7 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, + interpreterService, ); await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => { @@ -326,6 +329,7 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, + interpreterService, ); configService.getSettings(workspaceUri).testing.pytestArgs = []; @@ -415,6 +419,7 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, + interpreterService, ); configService.getSettings(workspaceUri).testing.pytestArgs = []; @@ -491,6 +496,7 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, + interpreterService, ); // set workspace to test workspace folder @@ -1038,6 +1044,7 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, + interpreterService, ); // set workspace to test workspace folder diff --git a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts index 87b91f6ae2da..890756e9e626 100644 --- a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts +++ b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts @@ -20,6 +20,7 @@ import { EXTENSION_ROOT_DIR } from '../../../../client/constants'; import { MockChildProcess } from '../../../mocks/mockChildProcess'; import { Deferred, createDeferred } from '../../../../client/common/utils/async'; import * as util from '../../../../client/testing/testController/common/utils'; +import { IInterpreterService } from '../../../../client/interpreter/contracts'; suite('pytest test discovery adapter', () => { let configService: IConfigurationService; @@ -71,7 +72,7 @@ suite('pytest test discovery adapter', () => { execService = typeMoq.Mock.ofType(); execService.setup((p) => ((p as unknown) as any).then).returns(() => undefined); outputChannel = typeMoq.Mock.ofType(); - + interpreterService = typeMoq.Mock.ofType(); const output = new Observable>(() => { /* no op */ }); From 5a4bf5b22808bc27a65d42f1c2c38117dc2d7814 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 3 Oct 2024 10:59:31 -0700 Subject: [PATCH 3/5] remove completed comments --- src/client/common/process/pythonExecutionFactory.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index 645d6aa3c151..228e8fc1f9ef 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -115,15 +115,14 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { pythonPath: options.interpreter ? options.interpreter.path : undefined, }); } - // TODO: Try passing actual interpreter when calling this from Pytest + const pythonPath = options.interpreter ? options.interpreter.path - : this.configService.getSettings(options.resource).pythonPath; // : this.configService.getSettings(options.resource).pythonPath; MIGHT BE A PROBLEM. + : this.configService.getSettings(options.resource).pythonPath; const processService: IProcessService = new ProcessService({ ...envVars }); processService.on('exec', this.logger.logProcess.bind(this.logger)); this.disposables.push(processService); - // TODO: Switch ordering on pixi and conda to see if that helps selecting right conda environment to run. --> Looks like done? const condaExecutionService = await this.createCondaExecutionService(pythonPath, processService); if (condaExecutionService) { return condaExecutionService; From 907566044571f2919f30afa8244ac4437eb68036 Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Thu, 3 Oct 2024 11:12:23 -0700 Subject: [PATCH 4/5] remove unncessary --- .../testController/pytest/pytestDiscoveryAdapter.unit.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts index 890756e9e626..f365c3e7244a 100644 --- a/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts +++ b/src/test/testing/testController/pytest/pytestDiscoveryAdapter.unit.test.ts @@ -20,7 +20,6 @@ import { EXTENSION_ROOT_DIR } from '../../../../client/constants'; import { MockChildProcess } from '../../../mocks/mockChildProcess'; import { Deferred, createDeferred } from '../../../../client/common/utils/async'; import * as util from '../../../../client/testing/testController/common/utils'; -import { IInterpreterService } from '../../../../client/interpreter/contracts'; suite('pytest test discovery adapter', () => { let configService: IConfigurationService; @@ -72,7 +71,6 @@ suite('pytest test discovery adapter', () => { execService = typeMoq.Mock.ofType(); execService.setup((p) => ((p as unknown) as any).then).returns(() => undefined); outputChannel = typeMoq.Mock.ofType(); - interpreterService = typeMoq.Mock.ofType(); const output = new Observable>(() => { /* no op */ }); From 405c5e0e716b368bcd7594cfac169d109fc39f0c Mon Sep 17 00:00:00 2001 From: Anthony Kim Date: Fri, 4 Oct 2024 11:24:37 -0700 Subject: [PATCH 5/5] don't plumb service through all layers. Pass from the controller --- src/client/testing/testController/common/types.ts | 7 ++++++- src/client/testing/testController/controller.ts | 2 ++ .../pytest/pytestDiscoveryAdapter.ts | 14 ++++++++------ .../testing/testController/workspaceTestAdapter.ts | 4 +++- src/test/testing/common/testingAdapter.test.ts | 8 -------- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/client/testing/testController/common/types.ts b/src/client/testing/testController/common/types.ts index 0942d9d2588c..2e141461eed1 100644 --- a/src/client/testing/testController/common/types.ts +++ b/src/client/testing/testController/common/types.ts @@ -17,6 +17,7 @@ import { ITestDebugLauncher, TestDiscoveryOptions } from '../../common/types'; import { IPythonExecutionFactory } from '../../../common/process/types'; import { EnvironmentVariables } from '../../../common/variables/types'; import { Deferred } from '../../../common/utils/async'; +import { PythonEnvironment } from '../../../pythonEnvironments/info'; export type TestRunInstanceOptions = TestRunOptions & { exclude?: readonly TestItem[]; @@ -215,7 +216,11 @@ export interface ITestResultResolver { export interface ITestDiscoveryAdapter { // ** first line old method signature, second line new method signature discoverTests(uri: Uri): Promise; - discoverTests(uri: Uri, executionFactory: IPythonExecutionFactory): Promise; + discoverTests( + uri: Uri, + executionFactory: IPythonExecutionFactory, + interpreter?: PythonEnvironment, + ): Promise; } // interface for execution/runner adapter diff --git a/src/client/testing/testController/controller.ts b/src/client/testing/testController/controller.ts index dd624078a534..2e6f44bca770 100644 --- a/src/client/testing/testController/controller.ts +++ b/src/client/testing/testController/controller.ts @@ -276,6 +276,7 @@ export class PythonTestController implements ITestController, IExtensionSingleAc this.testController, this.refreshCancellation.token, this.pythonExecFactory, + await this.interpreterService.getActiveInterpreter(workspace.uri), ); } else { traceError('Unable to find test adapter for workspace.'); @@ -297,6 +298,7 @@ export class PythonTestController implements ITestController, IExtensionSingleAc this.testController, this.refreshCancellation.token, this.pythonExecFactory, + await this.interpreterService.getActiveInterpreter(workspace.uri), ); } else { traceError('Unable to find test adapter for workspace.'); diff --git a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts index 0e5749efac03..874a50b29b2c 100644 --- a/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts +++ b/src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts @@ -24,7 +24,7 @@ import { hasSymlinkParent, } from '../common/utils'; import { IEnvironmentVariablesProvider } from '../../../common/variables/types'; -import { IInterpreterService } from '../../../interpreter/contracts'; +import { PythonEnvironment } from '../../../pythonEnvironments/info'; /** * Wrapper class for unittest test discovery. This is where we call `runTestCommand`. #this seems incorrectly copied @@ -35,10 +35,13 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { private readonly outputChannel: ITestOutputChannel, private readonly resultResolver?: ITestResultResolver, private readonly envVarsService?: IEnvironmentVariablesProvider, - private readonly interpreterService?: IInterpreterService, ) {} - async discoverTests(uri: Uri, executionFactory?: IPythonExecutionFactory): Promise { + async discoverTests( + uri: Uri, + executionFactory?: IPythonExecutionFactory, + interpreter?: PythonEnvironment, + ): Promise { const deferredTillEOT: Deferred = createDeferred(); const { name, dispose } = await startDiscoveryNamedPipe((data: DiscoveredTestPayload | EOTTestPayload) => { @@ -46,7 +49,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { }); try { - await this.runPytestDiscovery(uri, name, deferredTillEOT, executionFactory); + await this.runPytestDiscovery(uri, name, deferredTillEOT, executionFactory, interpreter); } finally { await deferredTillEOT.promise; traceVerbose('deferredTill EOT resolved'); @@ -62,6 +65,7 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { discoveryPipeName: string, deferredTillEOT: Deferred, executionFactory?: IPythonExecutionFactory, + interpreter?: PythonEnvironment, ): Promise { const relativePathToPytest = 'python_files'; const fullPluginPath = path.join(EXTENSION_ROOT_DIR, relativePathToPytest); @@ -104,8 +108,6 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter { env: mutableEnv, }; - const interpreter = await this.interpreterService?.getActiveInterpreter(); - // Create the Python environment in which to execute the command. const creationOptions: ExecutionFactoryCreateWithEnvironmentOptions = { allowEnvironmentFetchExceptions: false, diff --git a/src/client/testing/testController/workspaceTestAdapter.ts b/src/client/testing/testController/workspaceTestAdapter.ts index a0e65cfb5061..6c86e35385f6 100644 --- a/src/client/testing/testController/workspaceTestAdapter.ts +++ b/src/client/testing/testController/workspaceTestAdapter.ts @@ -14,6 +14,7 @@ import { ITestDiscoveryAdapter, ITestExecutionAdapter, ITestResultResolver } fro import { IPythonExecutionFactory } from '../../common/process/types'; import { ITestDebugLauncher } from '../common/types'; import { buildErrorNodeOptions } from './common/utils'; +import { PythonEnvironment } from '../../pythonEnvironments/info'; /** * This class exposes a test-provider-agnostic way of discovering tests. @@ -115,6 +116,7 @@ export class WorkspaceTestAdapter { testController: TestController, token?: CancellationToken, executionFactory?: IPythonExecutionFactory, + interpreter?: PythonEnvironment, ): Promise { sendTelemetryEvent(EventName.UNITTEST_DISCOVERING, undefined, { tool: this.testProvider }); @@ -130,7 +132,7 @@ export class WorkspaceTestAdapter { try { // ** execution factory only defined for new rewrite way if (executionFactory !== undefined) { - await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory); + await this.discoveryAdapter.discoverTests(this.workspaceUri, executionFactory, interpreter); } else { await this.discoveryAdapter.discoverTests(this.workspaceUri); } diff --git a/src/test/testing/common/testingAdapter.test.ts b/src/test/testing/common/testingAdapter.test.ts index 0bdb741f89b9..d1731dff8d9a 100644 --- a/src/test/testing/common/testingAdapter.test.ts +++ b/src/test/testing/common/testingAdapter.test.ts @@ -22,10 +22,8 @@ import { TestProvider } from '../../../client/testing/types'; import { PYTEST_PROVIDER, UNITTEST_PROVIDER } from '../../../client/testing/common/constants'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; import { createTypeMoq } from '../../mocks/helper'; -import { IInterpreterService } from '../../../client/interpreter/contracts'; suite('End to End Tests: test adapters', () => { - let interpreterService: IInterpreterService; let resultResolver: ITestResultResolver; let pythonExecFactory: IPythonExecutionFactory; let configService: IConfigurationService; @@ -111,7 +109,6 @@ suite('End to End Tests: test adapters', () => { pythonExecFactory = serviceContainer.get(IPythonExecutionFactory); testController = serviceContainer.get(ITestController); envVarsService = serviceContainer.get(IEnvironmentVariablesProvider); - interpreterService = serviceContainer.get(IInterpreterService); // create objects that were not injected testOutputChannel = createTypeMoq(); @@ -273,7 +270,6 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, - interpreterService, ); await discoveryAdapter.discoverTests(workspaceUri, pythonExecFactory).finally(() => { @@ -329,7 +325,6 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, - interpreterService, ); configService.getSettings(workspaceUri).testing.pytestArgs = []; @@ -419,7 +414,6 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, - interpreterService, ); configService.getSettings(workspaceUri).testing.pytestArgs = []; @@ -496,7 +490,6 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, - interpreterService, ); // set workspace to test workspace folder @@ -1044,7 +1037,6 @@ suite('End to End Tests: test adapters', () => { testOutputChannel.object, resultResolver, envVarsService, - interpreterService, ); // set workspace to test workspace folder