diff --git a/src/client/terminals/codeExecution/helper.ts b/src/client/terminals/codeExecution/helper.ts index 490673ed40f3..ee707df767a3 100644 --- a/src/client/terminals/codeExecution/helper.ts +++ b/src/client/terminals/codeExecution/helper.ts @@ -8,8 +8,7 @@ import { IApplicationShell, IDocumentManager } from '../../common/application/ty import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../common/constants'; import '../../common/extensions'; import { traceError } from '../../common/logger'; -import { IProcessServiceFactory } from '../../common/process/types'; -import { IConfigurationService } from '../../common/types'; +import { IPythonExecutionFactory } from '../../common/process/types'; import { IServiceContainer } from '../../ioc/types'; import { ICodeExecutionHelper } from '../types'; @@ -17,13 +16,11 @@ import { ICodeExecutionHelper } from '../types'; export class CodeExecutionHelper implements ICodeExecutionHelper { private readonly documentManager: IDocumentManager; private readonly applicationShell: IApplicationShell; - private readonly processServiceFactory: IProcessServiceFactory; - private readonly configurationService: IConfigurationService; + private readonly pythonServiceFactory: IPythonExecutionFactory; constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { this.documentManager = serviceContainer.get(IDocumentManager); this.applicationShell = serviceContainer.get(IApplicationShell); - this.processServiceFactory = serviceContainer.get(IProcessServiceFactory); - this.configurationService = serviceContainer.get(IConfigurationService); + this.pythonServiceFactory = serviceContainer.get(IPythonExecutionFactory); } public async normalizeLines(code: string, resource?: Uri): Promise { try { @@ -33,10 +30,9 @@ export class CodeExecutionHelper implements ICodeExecutionHelper { // On windows cr is not handled well by python when passing in/out via stdin/stdout. // So just remove cr from the input. code = code.replace(new RegExp('\\r', 'g'), ''); - const pythonPath = this.configurationService.getSettings(resource).pythonPath; const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'normalizeForInterpreter.py'), code]; - const processService = await this.processServiceFactory.create(resource); - const proc = await processService.exec(pythonPath, args, { throwOnStdErr: true }); + const processService = await this.pythonServiceFactory.create({ resource }); + const proc = await processService.exec(args, { throwOnStdErr: true }); return proc.stdout; } catch (ex) { diff --git a/src/client/testing/navigation/symbolProvider.ts b/src/client/testing/navigation/symbolProvider.ts index f854cc75548c..66fdf00e0ef0 100644 --- a/src/client/testing/navigation/symbolProvider.ts +++ b/src/client/testing/navigation/symbolProvider.ts @@ -7,8 +7,7 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { CancellationToken, DocumentSymbolProvider, Location, Range, SymbolInformation, SymbolKind, TextDocument, Uri } from 'vscode'; import { traceError } from '../../common/logger'; -import { IProcessServiceFactory } from '../../common/process/types'; -import { IConfigurationService } from '../../common/types'; +import { IPythonExecutionFactory } from '../../common/process/types'; import { EXTENSION_ROOT_DIR } from '../../constants'; type RawSymbol = { namespace: string; name: string; range: Range }; @@ -20,10 +19,7 @@ type Symbols = { @injectable() export class TestFileSymbolProvider implements DocumentSymbolProvider { - constructor( - @inject(IConfigurationService) private readonly configurationService: IConfigurationService, - @inject(IProcessServiceFactory) private readonly processServiceFactory: IProcessServiceFactory - ) {} + constructor(@inject(IPythonExecutionFactory) private readonly pythonServiceFactory: IPythonExecutionFactory) {} public async provideDocumentSymbols(document: TextDocument, token: CancellationToken): Promise { const rawSymbols = await this.getSymbols(document, token); if (!rawSymbols) { @@ -53,10 +49,9 @@ export class TestFileSymbolProvider implements DocumentSymbolProvider { if (document.isDirty) { scriptArgs.push(document.getText()); } - const pythonPath = this.configurationService.getSettings(document.uri).pythonPath; const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'symbolProvider.py'), ...scriptArgs]; - const processService = await this.processServiceFactory.create(document.uri); - const proc = await processService.exec(pythonPath, args, { throwOnStdErr: true, token }); + const pythonService = await this.pythonServiceFactory.create({ resource: document.uri }); + const proc = await pythonService.exec(args, { throwOnStdErr: true, token }); return JSON.parse(proc.stdout); } catch (ex) { diff --git a/src/test/terminals/codeExecution/helper.test.ts b/src/test/terminals/codeExecution/helper.test.ts index be8185f2b238..bfe0633f9647 100644 --- a/src/test/terminals/codeExecution/helper.test.ts +++ b/src/test/terminals/codeExecution/helper.test.ts @@ -14,8 +14,7 @@ import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../../client/common/cons import '../../../client/common/extensions'; import { BufferDecoder } from '../../../client/common/process/decoder'; import { ProcessService } from '../../../client/common/process/proc'; -import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; -import { IConfigurationService, IPythonSettings } from '../../../client/common/types'; +import { IPythonExecutionFactory, IPythonExecutionService } from '../../../client/common/process/types'; import { OSType } from '../../../client/common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types'; import { IServiceContainer } from '../../../client/ioc/types'; @@ -32,28 +31,22 @@ suite('Terminal - Code Execution Helper', () => { let helper: ICodeExecutionHelper; let document: TypeMoq.IMock; let editor: TypeMoq.IMock; - let processService: TypeMoq.IMock; - let configService: TypeMoq.IMock; + let pythonService: TypeMoq.IMock; setup(() => { const serviceContainer = TypeMoq.Mock.ofType(); documentManager = TypeMoq.Mock.ofType(); applicationShell = TypeMoq.Mock.ofType(); const envVariablesProvider = TypeMoq.Mock.ofType(); - processService = TypeMoq.Mock.ofType(); - configService = TypeMoq.Mock.ofType(); - const pythonSettings = TypeMoq.Mock.ofType(); - pythonSettings.setup(p => p.pythonPath).returns(() => PYTHON_PATH); + pythonService = TypeMoq.Mock.ofType(); // tslint:disable-next-line:no-any - processService.setup((x: any) => x.then).returns(() => undefined); - configService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); + pythonService.setup((x: any) => x.then).returns(() => undefined); envVariablesProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({})); - const processServiceFactory = TypeMoq.Mock.ofType(); - processServiceFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(processService.object)); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProcessServiceFactory), TypeMoq.It.isAny())).returns(() => processServiceFactory.object); + const pythonExecFactory = TypeMoq.Mock.ofType(); + pythonExecFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(pythonService.object)); + serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPythonExecutionFactory), TypeMoq.It.isAny())).returns(() => pythonExecFactory.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDocumentManager), TypeMoq.It.isAny())).returns(() => documentManager.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => applicationShell.object); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())).returns(() => envVariablesProvider.object); - serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IConfigurationService), TypeMoq.It.isAny())).returns(() => configService.object); helper = new CodeExecutionHelper(serviceContainer.object); document = TypeMoq.Mock.ofType(); @@ -63,19 +56,20 @@ suite('Terminal - Code Execution Helper', () => { async function ensureBlankLinesAreRemoved(source: string, expectedSource: string) { const actualProcessService = new ProcessService(new BufferDecoder()); - processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns((file, args, options) => { - return actualProcessService.exec.apply(actualProcessService, [file, args, options]); + pythonService + .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns((args, options) => { + return actualProcessService.exec.apply(actualProcessService, [PYTHON_PATH, args, options]); }); const normalizedZCode = await helper.normalizeLines(source); // In case file has been saved with different line endings. expectedSource = expectedSource.splitLines({ removeEmptyEntries: false, trim: false }).join(EOL); expect(normalizedZCode).to.be.equal(expectedSource); } - test('Ensure blank lines are NOT removed when code is not indented (simple)', async function () { + test('Ensure blank lines are NOT removed when code is not indented (simple)', async function() { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && (await isPythonVersion('2.7'))) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -87,19 +81,20 @@ suite('Terminal - Code Execution Helper', () => { test('Ensure there are no multiple-CR elements in the normalized code.', async () => { const code = ['import sys', '', '', '', 'print(sys.executable)', '', 'print("1234")', '', '', 'print(1)', 'print(2)']; const actualProcessService = new ProcessService(new BufferDecoder()); - processService.setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) - .returns((file, args, options) => { - return actualProcessService.exec.apply(actualProcessService, [file, args, options]); + pythonService + .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .returns((args, options) => { + return actualProcessService.exec.apply(actualProcessService, [PYTHON_PATH, args, options]); }); const normalizedCode = await helper.normalizeLines(code.join(EOL)); const doubleCrIndex = normalizedCode.indexOf('\r\r'); expect(doubleCrIndex).to.be.equal(-1, 'Double CR (CRCRLF) line endings detected in normalized code snippet.'); }); ['', '1', '2', '3', '4', '5', '6', '7', '8'].forEach(fileNameSuffix => { - test(`Ensure blank lines are removed (Sample${fileNameSuffix})`, async function () { + test(`Ensure blank lines are removed (Sample${fileNameSuffix})`, async function() { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && (await isPythonVersion('2.7'))) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -108,10 +103,10 @@ suite('Terminal - Code Execution Helper', () => { const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_normalized.py`), 'utf8'); await ensureBlankLinesAreRemoved(code, expectedCode); }); - test(`Ensure last two blank lines are preserved (Sample${fileNameSuffix})`, async function () { + test(`Ensure last two blank lines are preserved (Sample${fileNameSuffix})`, async function() { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && (await isPythonVersion('2.7'))) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -120,10 +115,10 @@ suite('Terminal - Code Execution Helper', () => { const expectedCode = await fs.readFile(path.join(TEST_FILES_PATH, `sample${fileNameSuffix}_normalized.py`), 'utf8'); await ensureBlankLinesAreRemoved(code + EOL, expectedCode + EOL); }); - test(`Ensure last two blank lines are preserved even if we have more than 2 trailing blank lines (Sample${fileNameSuffix})`, async function () { + test(`Ensure last two blank lines are preserved even if we have more than 2 trailing blank lines (Sample${fileNameSuffix})`, async function() { // This test has not been working for many months in Python 2.7 under // Windows.Tracked by #2544. - if (isOs(OSType.Windows) && await isPythonVersion('2.7')) { + if (isOs(OSType.Windows) && (await isPythonVersion('2.7'))) { // tslint:disable-next-line:no-invalid-this return this.skip(); } @@ -133,7 +128,7 @@ suite('Terminal - Code Execution Helper', () => { await ensureBlankLinesAreRemoved(code + EOL + EOL + EOL + EOL, expectedCode + EOL); }); }); - test('Display message if there\s no active file', async () => { + test('Display message if there\'s no active file', async () => { documentManager.setup(doc => doc.activeTextEditor).returns(() => undefined); const uri = await helper.getFileToExecute(); @@ -233,14 +228,20 @@ suite('Terminal - Code Execution Helper', () => { }); test('saveFileIfDirty will not fail if file is not opened', async () => { - documentManager.setup(d => d.textDocuments).returns(() => []).verifiable(TypeMoq.Times.once()); + documentManager + .setup(d => d.textDocuments) + .returns(() => []) + .verifiable(TypeMoq.Times.once()); await helper.saveFileIfDirty(Uri.file(`${__filename}.py`)); documentManager.verifyAll(); }); test('File will be saved if file is dirty', async () => { - documentManager.setup(d => d.textDocuments).returns(() => [document.object]).verifiable(TypeMoq.Times.once()); + documentManager + .setup(d => d.textDocuments) + .returns(() => [document.object]) + .verifiable(TypeMoq.Times.once()); document.setup(doc => doc.isUntitled).returns(() => false); document.setup(doc => doc.isDirty).returns(() => true); document.setup(doc => doc.languageId).returns(() => PYTHON_LANGUAGE); @@ -253,7 +254,10 @@ suite('Terminal - Code Execution Helper', () => { }); test('File will be not saved if file is not dirty', async () => { - documentManager.setup(d => d.textDocuments).returns(() => [document.object]).verifiable(TypeMoq.Times.once()); + documentManager + .setup(d => d.textDocuments) + .returns(() => [document.object]) + .verifiable(TypeMoq.Times.once()); document.setup(doc => doc.isUntitled).returns(() => false); document.setup(doc => doc.isDirty).returns(() => false); document.setup(doc => doc.languageId).returns(() => PYTHON_LANGUAGE); diff --git a/src/test/testing/navigation/symbolNavigator.unit.test.ts b/src/test/testing/navigation/symbolNavigator.unit.test.ts index 8b4e2cc2f656..0cfef1c989c7 100644 --- a/src/test/testing/navigation/symbolNavigator.unit.test.ts +++ b/src/test/testing/navigation/symbolNavigator.unit.test.ts @@ -5,45 +5,49 @@ import { expect } from 'chai'; import * as path from 'path'; -import { anything, deepEqual, instance, mock, verify, when } from 'ts-mockito'; import * as typemoq from 'typemoq'; import { CancellationToken, CancellationTokenSource, Range, SymbolInformation, SymbolKind, TextDocument, Uri } from 'vscode'; -import { ConfigurationService } from '../../../client/common/configuration/service'; -import { ProcessService } from '../../../client/common/process/proc'; -import { ProcessServiceFactory } from '../../../client/common/process/processFactory'; -import { ExecutionResult, IProcessService, IProcessServiceFactory } from '../../../client/common/process/types'; -import { IConfigurationService, IDocumentSymbolProvider } from '../../../client/common/types'; +import { ExecutionResult, IPythonExecutionFactory, IPythonExecutionService } from '../../../client/common/process/types'; +import { IDocumentSymbolProvider } from '../../../client/common/types'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { TestFileSymbolProvider } from '../../../client/testing/navigation/symbolProvider'; // tslint:disable:max-func-body-length no-any suite('Unit Tests - Navigation Command Handler', () => { let symbolProvider: IDocumentSymbolProvider; - let configService: IConfigurationService; - let processFactory: IProcessServiceFactory; - let processService: IProcessService; + let pythonExecFactory: typemoq.IMock; + let pythonService: typemoq.IMock; let doc: typemoq.IMock; let token: CancellationToken; setup(() => { - configService = mock(ConfigurationService); - processFactory = mock(ProcessServiceFactory); - processService = mock(ProcessService); + pythonService = typemoq.Mock.ofType(); + pythonExecFactory = typemoq.Mock.ofType(); + + // Both typemoq and ts-mockito fail to resolve promises on dynamically created mocks + // A solution is to mock the `then` on the mock that the `Promise` resolves to. + // typemoq: https://github.com/florinn/typemoq/issues/66#issuecomment-315681245 + // ts-mockito: https://github.com/NagRock/ts-mockito/issues/163#issuecomment-536210863 + // In this case, the factory below returns a promise that is a mock of python service + // so we need to mock the `then` on the service. + pythonService.setup((x: any) => x.then).returns(() => undefined); + + pythonExecFactory.setup(factory => factory.create(typemoq.It.isAny())).returns(async () => pythonService.object); + doc = typemoq.Mock.ofType(); token = new CancellationTokenSource().token; - symbolProvider = new TestFileSymbolProvider(instance(configService), instance(processFactory)); }); test('Ensure no symbols are returned when file has not been saved', async () => { doc.setup(d => d.isUntitled) .returns(() => true) .verifiable(typemoq.Times.once()); + symbolProvider = new TestFileSymbolProvider(pythonExecFactory.object); const symbols = await symbolProvider.provideDocumentSymbols(doc.object, token); expect(symbols).to.be.lengthOf(0); doc.verifyAll(); }); test('Ensure no symbols are returned when there are errors in running the code', async () => { - when(configService.getSettings(anything())).thenThrow(new Error('Kaboom')); doc.setup(d => d.isUntitled) .returns(() => false) .verifiable(typemoq.Times.once()); @@ -54,14 +58,19 @@ suite('Unit Tests - Navigation Command Handler', () => { .returns(() => Uri.file(__filename)) .verifiable(typemoq.Times.atLeastOnce()); + pythonService + .setup(service => service.exec(typemoq.It.isAny(), typemoq.It.isAny())) + .returns(async () => { + return { stdout: '' }; + }); + + symbolProvider = new TestFileSymbolProvider(pythonExecFactory.object); const symbols = await symbolProvider.provideDocumentSymbols(doc.object, token); - verify(configService.getSettings(anything())).once(); expect(symbols).to.be.lengthOf(0); doc.verifyAll(); }); test('Ensure no symbols are returned when there are no symbols to be returned', async () => { - const pythonPath = 'Hello There'; const docUri = Uri.file(__filename); const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'symbolProvider.py'), docUri.fsPath]; const proc: ExecutionResult = { @@ -76,22 +85,20 @@ suite('Unit Tests - Navigation Command Handler', () => { doc.setup(d => d.uri) .returns(() => docUri) .verifiable(typemoq.Times.atLeastOnce()); - when(configService.getSettings(anything())).thenReturn({ pythonPath } as any); - when(processFactory.create(anything())).thenResolve(instance(processService)); - when(processService.exec(pythonPath, anything(), anything())).thenResolve(proc); - doc.setup(d => d.isDirty).returns(() => false); - doc.setup(d => d.uri).returns(() => docUri); + pythonService + .setup(service => service.exec(typemoq.It.isValue(args), typemoq.It.isAny())) + .returns(async () => proc) + .verifiable(typemoq.Times.once()); + + symbolProvider = new TestFileSymbolProvider(pythonExecFactory.object); const symbols = await symbolProvider.provideDocumentSymbols(doc.object, token); - verify(configService.getSettings(anything())).once(); - verify(processFactory.create(anything())).once(); - verify(processService.exec(pythonPath, deepEqual(args), deepEqual({ throwOnStdErr: true, token }))).once(); expect(symbols).to.be.lengthOf(0); doc.verifyAll(); + pythonService.verifyAll(); }); test('Ensure symbols are returned', async () => { - const pythonPath = 'Hello There'; const docUri = Uri.file(__filename); const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'symbolProvider.py'), docUri.fsPath]; const proc: ExecutionResult = { @@ -131,19 +138,18 @@ suite('Unit Tests - Navigation Command Handler', () => { doc.setup(d => d.uri) .returns(() => docUri) .verifiable(typemoq.Times.atLeastOnce()); - when(configService.getSettings(anything())).thenReturn({ pythonPath } as any); - when(processFactory.create(anything())).thenResolve(instance(processService)); - when(processService.exec(pythonPath, anything(), anything())).thenResolve(proc); - doc.setup(d => d.isDirty).returns(() => false); - doc.setup(d => d.uri).returns(() => docUri); + pythonService + .setup(service => service.exec(typemoq.It.isValue(args), typemoq.It.isAny())) + .returns(async () => proc) + .verifiable(typemoq.Times.once()); + + symbolProvider = new TestFileSymbolProvider(pythonExecFactory.object); const symbols = (await symbolProvider.provideDocumentSymbols(doc.object, token)) as SymbolInformation[]; - verify(configService.getSettings(anything())).once(); - verify(processFactory.create(anything())).once(); - verify(processService.exec(pythonPath, deepEqual(args), deepEqual({ throwOnStdErr: true, token }))).once(); expect(symbols).to.be.lengthOf(3); doc.verifyAll(); + pythonService.verifyAll(); expect(symbols[0].kind).to.be.equal(SymbolKind.Class); expect(symbols[0].name).to.be.equal('one'); expect(symbols[0].location.range).to.be.deep.equal(new Range(1, 2, 3, 4));