From 18dfdc94a62608d905035f8755fef741a28f7af0 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 May 2019 16:22:00 -0700 Subject: [PATCH 1/4] Corrected bug and added tests --- .../DebugClients/launcherProvider.ts | 14 ++-- .../launcherProvider.unit.test.ts | 77 ++++++++++++------- 2 files changed, 55 insertions(+), 36 deletions(-) diff --git a/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts b/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts index bc4b94af90e7..53c6aeba2f4d 100644 --- a/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts +++ b/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts @@ -9,24 +9,24 @@ import * as path from 'path'; import { EXTENSION_ROOT_DIR } from '../../../common/constants'; import { IDebugLauncherScriptProvider, IRemoteDebugLauncherScriptProvider, LocalDebugOptions, RemoteDebugOptions } from '../types'; -const script = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'ptvsd_launcher.py'); +const pathToScript = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'ptvsd_launcher.py'); export class NoDebugLauncherScriptProvider implements IDebugLauncherScriptProvider { - public getLauncherArgs(options: LocalDebugOptions): string[] { + public getLauncherArgs(options: LocalDebugOptions, script: string = pathToScript): string[] { const customDebugger = options.customDebugger ? '--custom' : '--default'; - return [script, customDebugger, '--nodebug', '--client', '--host', options.host, '--port', options.port.toString()]; + return [script.fileToCommandArgument(), customDebugger, '--nodebug', '--client', '--host', options.host, '--port', options.port.toString()]; } } export class DebuggerLauncherScriptProvider implements IDebugLauncherScriptProvider { - public getLauncherArgs(options: LocalDebugOptions): string[] { + public getLauncherArgs(options: LocalDebugOptions, script: string = pathToScript): string[] { const customDebugger = options.customDebugger ? '--custom' : '--default'; - return [script, customDebugger, '--client', '--host', options.host, '--port', options.port.toString()]; + return [script.fileToCommandArgument(), customDebugger, '--client', '--host', options.host, '--port', options.port.toString()]; } } export class RemoteDebuggerLauncherScriptProvider implements IRemoteDebugLauncherScriptProvider { - public getLauncherArgs(options: RemoteDebugOptions): string[] { + public getLauncherArgs(options: RemoteDebugOptions, script: string = pathToScript): string[] { const waitArgs = options.waitUntilDebuggerAttaches ? ['--wait'] : []; - return [script, '--default', '--host', options.host, '--port', options.port.toString()].concat(waitArgs); + return [script.fileToCommandArgument(), '--default', '--host', options.host, '--port', options.port.toString()].concat(waitArgs); } } diff --git a/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts b/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts index 4dd9542a6b0c..13f751ff980a 100644 --- a/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts +++ b/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts @@ -11,38 +11,57 @@ import { DebuggerLauncherScriptProvider, NoDebugLauncherScriptProvider, RemoteDe const expectedPath = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'ptvsd_launcher.py'); +// tslint:disable-next-line:max-func-body-length suite('Debugger - Launcher Script Provider', () => { test('Ensure launcher script exists', async () => { expect(await fs.pathExists(expectedPath)).to.be.deep.equal(true, 'Debugger launcher script does not exist'); }); - test('Test debug launcher args', async () => { - const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }); - const expectedArgs = [expectedPath, '--default', '--client', '--host', 'something', '--port', '1234']; - expect(args).to.be.deep.equal(expectedArgs); - }); - test('Test non-debug launcher args', async () => { - const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }); - const expectedArgs = [expectedPath, '--default', '--nodebug', '--client', '--host', 'something', '--port', '1234']; - expect(args).to.be.deep.equal(expectedArgs); - }); - test('Test debug launcher args and custom ptvsd', async () => { - const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }); - const expectedArgs = [expectedPath, '--custom', '--client', '--host', 'something', '--port', '1234']; - expect(args).to.be.deep.equal(expectedArgs); - }); - test('Test non-debug launcher args and custom ptvsd', async () => { - const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }); - const expectedArgs = [expectedPath, '--custom', '--nodebug', '--client', '--host', 'something', '--port', '1234']; - expect(args).to.be.deep.equal(expectedArgs); - }); - test('Test remote debug launcher args (and do not wait for debugger to attach)', async () => { - const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: false }); - const expectedArgs = [expectedPath, '--default', '--host', 'something', '--port', '1234']; - expect(args).to.be.deep.equal(expectedArgs); - }); - test('Test remote debug launcher args (and wait for debugger to attach)', async () => { - const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: true }); - const expectedArgs = [expectedPath, '--default', '--host', 'something', '--port', '1234', '--wait']; - expect(args).to.be.deep.equal(expectedArgs); + const testsForLaunchProvider = + [ + { + testName: 'When path to ptvsd launcher does not contains spaces', + path: path.join('path', 'to', 'ptvsd_launcher'), + expectedPath: 'path/to/ptvsd_launcher' + }, + { + testName: 'When path to ptvsd launcher contains spaces', + path: path.join('path', 'to', 'ptvsd_launcher', 'with spaces'), + expectedPath: '"path/to/ptvsd_launcher/with spaces"' + } + ]; + + testsForLaunchProvider.forEach(testParams => { + suite(testParams.testName, async () => { + test('Test debug launcher args', async () => { + const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--default', '--client', '--host', 'something', '--port', '1234']; + expect(args).to.be.deep.equal(expectedArgs); + }); + test('Test non-debug launcher args', async () => { + const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--default', '--nodebug', '--client', '--host', 'something', '--port', '1234']; + expect(args).to.be.deep.equal(expectedArgs); + }); + test('Test debug launcher args and custom ptvsd', async () => { + const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--custom', '--client', '--host', 'something', '--port', '1234']; + expect(args).to.be.deep.equal(expectedArgs); + }); + test('Test non-debug launcher args and custom ptvsd', async () => { + const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--custom', '--nodebug', '--client', '--host', 'something', '--port', '1234']; + expect(args).to.be.deep.equal(expectedArgs); + }); + test('Test remote debug launcher args (and do not wait for debugger to attach)', async () => { + const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: false }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--default', '--host', 'something', '--port', '1234']; + expect(args).to.be.deep.equal(expectedArgs); + }); + test('Test remote debug launcher args (and wait for debugger to attach)', async () => { + const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: true }, testParams.path); + const expectedArgs = [testParams.expectedPath, '--default', '--host', 'something', '--port', '1234', '--wait']; + expect(args).to.be.deep.equal(expectedArgs); + }); + }); }); }); From 94a322eda554dc1d4fc2b1e06eec5024a5b3007c Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 May 2019 16:23:12 -0700 Subject: [PATCH 2/4] News entry --- news/2 Fixes/4966.md | 1 + src/client/debugger/debugAdapter/types.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 news/2 Fixes/4966.md diff --git a/news/2 Fixes/4966.md b/news/2 Fixes/4966.md new file mode 100644 index 000000000000..9e03fd5b0842 --- /dev/null +++ b/news/2 Fixes/4966.md @@ -0,0 +1 @@ +getRemoteLauncherCommand should wrap path to ptvsd_launcher.py in quotes diff --git a/src/client/debugger/debugAdapter/types.ts b/src/client/debugger/debugAdapter/types.ts index 886623b65e2f..a50ef0298c38 100644 --- a/src/client/debugger/debugAdapter/types.ts +++ b/src/client/debugger/debugAdapter/types.ts @@ -13,7 +13,7 @@ export type LocalDebugOptions = { port: number; host: string; customDebugger?: b export type RemoteDebugOptions = LocalDebugOptions & { waitUntilDebuggerAttaches: boolean }; export interface IDebugLauncherScriptProvider { - getLauncherArgs(options: T): string[]; + getLauncherArgs(options: T, script?: string): string[]; } export interface ILocalDebugLauncherScriptProvider extends IDebugLauncherScriptProvider { From 2371d41738bb8ce1608d311ea22674e15a4cf8c5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Wed, 8 May 2019 16:59:26 -0700 Subject: [PATCH 3/4] Updated tests --- src/test/extension.unit.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/extension.unit.test.ts b/src/test/extension.unit.test.ts index c9693be71e91..553be3f7121e 100644 --- a/src/test/extension.unit.test.ts +++ b/src/test/extension.unit.test.ts @@ -6,11 +6,10 @@ // tslint:disable:no-any import { expect } from 'chai'; -import * as path from 'path'; import { buildApi } from '../client/api'; import { EXTENSION_ROOT_DIR } from '../client/common/constants'; -const expectedPath = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'ptvsd_launcher.py'); +const expectedPath = `${EXTENSION_ROOT_DIR.fileToCommandArgument()}/pythonFiles/ptvsd_launcher.py`; suite('Extension API Debugger', () => { test('Test debug launcher args (no-wait)', async () => { From 4b626c8d299606a8dd149444afe181da0adb3550 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 10 May 2019 09:41:17 -0700 Subject: [PATCH 4/4] Code review --- .../DebugClients/launcherProvider.ts | 16 ++++++++++------ src/client/debugger/debugAdapter/types.ts | 2 +- .../debugClients/launcherProvider.unit.test.ts | 12 ++++++------ 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts b/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts index 53c6aeba2f4d..455db85cde63 100644 --- a/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts +++ b/src/client/debugger/debugAdapter/DebugClients/launcherProvider.ts @@ -5,28 +5,32 @@ // tslint:disable:max-classes-per-file +import { optional } from 'inversify'; import * as path from 'path'; import { EXTENSION_ROOT_DIR } from '../../../common/constants'; import { IDebugLauncherScriptProvider, IRemoteDebugLauncherScriptProvider, LocalDebugOptions, RemoteDebugOptions } from '../types'; const pathToScript = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'ptvsd_launcher.py'); export class NoDebugLauncherScriptProvider implements IDebugLauncherScriptProvider { - public getLauncherArgs(options: LocalDebugOptions, script: string = pathToScript): string[] { + constructor(@optional() private script: string = pathToScript) { } + public getLauncherArgs(options: LocalDebugOptions): string[] { const customDebugger = options.customDebugger ? '--custom' : '--default'; - return [script.fileToCommandArgument(), customDebugger, '--nodebug', '--client', '--host', options.host, '--port', options.port.toString()]; + return [this.script.fileToCommandArgument(), customDebugger, '--nodebug', '--client', '--host', options.host, '--port', options.port.toString()]; } } export class DebuggerLauncherScriptProvider implements IDebugLauncherScriptProvider { - public getLauncherArgs(options: LocalDebugOptions, script: string = pathToScript): string[] { + constructor(@optional() private script: string = pathToScript) { } + public getLauncherArgs(options: LocalDebugOptions): string[] { const customDebugger = options.customDebugger ? '--custom' : '--default'; - return [script.fileToCommandArgument(), customDebugger, '--client', '--host', options.host, '--port', options.port.toString()]; + return [this.script.fileToCommandArgument(), customDebugger, '--client', '--host', options.host, '--port', options.port.toString()]; } } export class RemoteDebuggerLauncherScriptProvider implements IRemoteDebugLauncherScriptProvider { - public getLauncherArgs(options: RemoteDebugOptions, script: string = pathToScript): string[] { + constructor(@optional() private script: string = pathToScript) { } + public getLauncherArgs(options: RemoteDebugOptions): string[] { const waitArgs = options.waitUntilDebuggerAttaches ? ['--wait'] : []; - return [script.fileToCommandArgument(), '--default', '--host', options.host, '--port', options.port.toString()].concat(waitArgs); + return [this.script.fileToCommandArgument(), '--default', '--host', options.host, '--port', options.port.toString()].concat(waitArgs); } } diff --git a/src/client/debugger/debugAdapter/types.ts b/src/client/debugger/debugAdapter/types.ts index a50ef0298c38..886623b65e2f 100644 --- a/src/client/debugger/debugAdapter/types.ts +++ b/src/client/debugger/debugAdapter/types.ts @@ -13,7 +13,7 @@ export type LocalDebugOptions = { port: number; host: string; customDebugger?: b export type RemoteDebugOptions = LocalDebugOptions & { waitUntilDebuggerAttaches: boolean }; export interface IDebugLauncherScriptProvider { - getLauncherArgs(options: T, script?: string): string[]; + getLauncherArgs(options: T): string[]; } export interface ILocalDebugLauncherScriptProvider extends IDebugLauncherScriptProvider { diff --git a/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts b/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts index 13f751ff980a..a3f67e3c22dd 100644 --- a/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts +++ b/src/test/debugger/debugAdapter/debugClients/launcherProvider.unit.test.ts @@ -33,32 +33,32 @@ suite('Debugger - Launcher Script Provider', () => { testsForLaunchProvider.forEach(testParams => { suite(testParams.testName, async () => { test('Test debug launcher args', async () => { - const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }, testParams.path); + const args = new DebuggerLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234 }); const expectedArgs = [testParams.expectedPath, '--default', '--client', '--host', 'something', '--port', '1234']; expect(args).to.be.deep.equal(expectedArgs); }); test('Test non-debug launcher args', async () => { - const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234 }, testParams.path); + const args = new NoDebugLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234 }); const expectedArgs = [testParams.expectedPath, '--default', '--nodebug', '--client', '--host', 'something', '--port', '1234']; expect(args).to.be.deep.equal(expectedArgs); }); test('Test debug launcher args and custom ptvsd', async () => { - const args = new DebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }, testParams.path); + const args = new DebuggerLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }); const expectedArgs = [testParams.expectedPath, '--custom', '--client', '--host', 'something', '--port', '1234']; expect(args).to.be.deep.equal(expectedArgs); }); test('Test non-debug launcher args and custom ptvsd', async () => { - const args = new NoDebugLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }, testParams.path); + const args = new NoDebugLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234, customDebugger: true }); const expectedArgs = [testParams.expectedPath, '--custom', '--nodebug', '--client', '--host', 'something', '--port', '1234']; expect(args).to.be.deep.equal(expectedArgs); }); test('Test remote debug launcher args (and do not wait for debugger to attach)', async () => { - const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: false }, testParams.path); + const args = new RemoteDebuggerLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: false }); const expectedArgs = [testParams.expectedPath, '--default', '--host', 'something', '--port', '1234']; expect(args).to.be.deep.equal(expectedArgs); }); test('Test remote debug launcher args (and wait for debugger to attach)', async () => { - const args = new RemoteDebuggerLauncherScriptProvider().getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: true }, testParams.path); + const args = new RemoteDebuggerLauncherScriptProvider(testParams.path).getLauncherArgs({ host: 'something', port: 1234, waitUntilDebuggerAttaches: true }); const expectedArgs = [testParams.expectedPath, '--default', '--host', 'something', '--port', '1234', '--wait']; expect(args).to.be.deep.equal(expectedArgs); });