Skip to content

Use PythonExecutionService when possible #8338

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 4, 2019
Merged

Use PythonExecutionService when possible #8338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20f153a
Update symbol provider.
karthiknadig Nov 1, 2019
2233385
Update code execution helper.
karthiknadig Nov 1, 2019
f5e1fc2
Switch to typemoq
karthiknadig Nov 2, 2019
6d3f723
Fix for typemoq and ts-mockito limitations
karthiknadig Nov 2, 2019
1812761
Fix helper tests
karthiknadig Nov 2, 2019
943e2f1
Clean up symbol navigator tests
karthiknadig Nov 2, 2019
addbcb6
Address comments.
karthiknadig Nov 5, 2019
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
14 changes: 5 additions & 9 deletions src/client/terminals/codeExecution/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,19 @@ 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';

@injectable()
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>(IDocumentManager);
this.applicationShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
this.configurationService = serviceContainer.get<IConfigurationService>(IConfigurationService);
this.pythonServiceFactory = serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
}
public async normalizeLines(code: string, resource?: Uri): Promise<string> {
try {
Expand All @@ -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) {
Expand Down
13 changes: 4 additions & 9 deletions src/client/testing/navigation/symbolProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand All @@ -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<SymbolInformation[]> {
const rawSymbols = await this.getSymbols(document, token);
if (!rawSymbols) {
Expand Down Expand Up @@ -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) {
Expand Down
68 changes: 36 additions & 32 deletions src/test/terminals/codeExecution/helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -32,28 +31,22 @@ suite('Terminal - Code Execution Helper', () => {
let helper: ICodeExecutionHelper;
let document: TypeMoq.IMock<TextDocument>;
let editor: TypeMoq.IMock<TextEditor>;
let processService: TypeMoq.IMock<IProcessService>;
let configService: TypeMoq.IMock<IConfigurationService>;
let pythonService: TypeMoq.IMock<IPythonExecutionService>;
setup(() => {
const serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
applicationShell = TypeMoq.Mock.ofType<IApplicationShell>();
const envVariablesProvider = TypeMoq.Mock.ofType<IEnvironmentVariablesProvider>();
processService = TypeMoq.Mock.ofType<IProcessService>();
configService = TypeMoq.Mock.ofType<IConfigurationService>();
const pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
pythonSettings.setup(p => p.pythonPath).returns(() => PYTHON_PATH);
pythonService = TypeMoq.Mock.ofType<IPythonExecutionService>();
// 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<IProcessServiceFactory>();
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<IPythonExecutionFactory>();
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<TextDocument>();
Expand All @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading