From 3f3527dedb7e76890ba14a688a4cf056c0c478c9 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 31 Oct 2019 09:12:32 -0700 Subject: [PATCH 1/4] Null check terminal processes before calling on them in ext host Fixes #81821 --- .../workbench/api/common/extHostTerminalService.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index bf24a7408c2..4785afa1cda 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -369,7 +369,7 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ if (this._terminalProcesses[id]) { // Extension pty terminal only - when virtual process resize fires it means that the // terminal's maximum dimensions changed - this._terminalProcesses[id].resize(cols, rows); + this._terminalProcesses[id]?.resize(cols, rows); } } @@ -465,12 +465,12 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } public $acceptProcessInput(id: number, data: string): void { - this._terminalProcesses[id].input(data); + this._terminalProcesses[id]?.input(data); } public $acceptProcessResize(id: number, cols: number, rows: number): void { try { - this._terminalProcesses[id].resize(cols, rows); + this._terminalProcesses[id]?.resize(cols, rows); } catch (error) { // We tried to write to a closed pipe / channel. if (error.code !== 'EPIPE' && error.code !== 'ERR_IPC_CHANNEL_CLOSED') { @@ -480,15 +480,15 @@ export abstract class BaseExtHostTerminalService implements IExtHostTerminalServ } public $acceptProcessShutdown(id: number, immediate: boolean): void { - this._terminalProcesses[id].shutdown(immediate); + this._terminalProcesses[id]?.shutdown(immediate); } public $acceptProcessRequestInitialCwd(id: number): void { - this._terminalProcesses[id].getInitialCwd().then(initialCwd => this._proxy.$sendProcessInitialCwd(id, initialCwd)); + this._terminalProcesses[id]?.getInitialCwd().then(initialCwd => this._proxy.$sendProcessInitialCwd(id, initialCwd)); } public $acceptProcessRequestCwd(id: number): void { - this._terminalProcesses[id].getCwd().then(cwd => this._proxy.$sendProcessCwd(id, cwd)); + this._terminalProcesses[id]?.getCwd().then(cwd => this._proxy.$sendProcessCwd(id, cwd)); } public $acceptProcessRequestLatency(id: number): number { From c0e630fd7ac8c3d765688328b76bbb1d8bfba7f7 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 31 Oct 2019 10:43:36 -0700 Subject: [PATCH 2/4] Pass assertion failure to done --- .../src/singlefolder-tests/terminal.test.ts | 102 +++++++++++++----- .../workspace.tasks.test.ts | 6 +- 2 files changed, 80 insertions(+), 28 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index 7138319d459..e25292bf250 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -14,7 +14,11 @@ suite('window namespace tests', () => { suite('Terminal', () => { test('sendText immediately after createTerminal should not throw', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(terminal, term); + try { + equal(terminal, term); + } catch (e) { + done(e); + } terminal.dispose(); reg1.dispose(); const reg2 = window.onDidCloseTerminal(() => { @@ -28,7 +32,11 @@ suite('window namespace tests', () => { test('onDidCloseTerminal event fires when terminal is disposed', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(terminal, term); + try { + equal(terminal, term); + } catch (e) { + done(e); + } terminal.dispose(); reg1.dispose(); const reg2 = window.onDidCloseTerminal(() => { @@ -41,10 +49,18 @@ suite('window namespace tests', () => { test('processId immediately after createTerminal should fetch the pid', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(terminal, term); + try { + equal(terminal, term); + } catch (e) { + done(e); + } reg1.dispose(); terminal.processId.then(id => { - ok(id > 0); + try { + ok(id > 0); + } catch (e) { + done(e); + } terminal.dispose(); const reg2 = window.onDidCloseTerminal(() => { reg2.dispose(); @@ -57,7 +73,11 @@ suite('window namespace tests', () => { test('name in constructor should set terminal.name', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(terminal, term); + try { + equal(terminal, term); + } catch (e) { + done(e); + } terminal.dispose(); reg1.dispose(); const reg2 = window.onDidCloseTerminal(() => { @@ -66,12 +86,20 @@ suite('window namespace tests', () => { }); }); const terminal = window.createTerminal('a'); - equal(terminal.name, 'a'); + try { + equal(terminal.name, 'a'); + } catch (e) { + done(e); + } }); test('onDidOpenTerminal should fire when a terminal is created', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(term.name, 'b'); + try { + equal(term.name, 'b'); + } catch (e) { + done(e); + } reg1.dispose(); const reg2 = window.onDidCloseTerminal(() => { reg2.dispose(); @@ -155,9 +183,13 @@ suite('window namespace tests', () => { test('should be available to terminals API', done => { const terminal = window.createTerminal({ name: 'bg', hideFromUser: true }); window.onDidOpenTerminal(t => { - equal(t, terminal); - equal(t.name, 'bg'); - ok(window.terminals.indexOf(terminal) !== -1); + try { + equal(t, terminal); + equal(t.name, 'bg'); + ok(window.terminals.indexOf(terminal) !== -1); + } catch (e) { + done(e); + } const reg3 = window.onDidCloseTerminal(() => { reg3.dispose(); done(); @@ -185,19 +217,23 @@ suite('window namespace tests', () => { const reg3 = window.onDidCloseTerminal(e => { closeEvents.push(e.name); - if (closeEvents.length === 1) { - deepEqual(openEvents, ['test1']); - deepEqual(dataEvents, [{ name: 'test1', data: 'write1' }]); - deepEqual(closeEvents, ['test1']); - } else if (closeEvents.length === 2) { - deepEqual(openEvents, ['test1', 'test2']); - deepEqual(dataEvents, [{ name: 'test1', data: 'write1' }, { name: 'test2', data: 'write2' }]); - deepEqual(closeEvents, ['test1', 'test2']); + try { + if (closeEvents.length === 1) { + deepEqual(openEvents, ['test1']); + deepEqual(dataEvents, [{ name: 'test1', data: 'write1' }]); + deepEqual(closeEvents, ['test1']); + } else if (closeEvents.length === 2) { + deepEqual(openEvents, ['test1', 'test2']); + deepEqual(dataEvents, [{ name: 'test1', data: 'write1' }, { name: 'test2', data: 'write2' }]); + deepEqual(closeEvents, ['test1', 'test2']); - reg1.dispose(); - reg2.dispose(); - reg3.dispose(); - done(); + reg1.dispose(); + reg2.dispose(); + reg3.dispose(); + done(); + } + } catch (e) { + done(e); } }); @@ -237,7 +273,11 @@ suite('window namespace tests', () => { suite('Extension pty terminals', () => { test('should fire onDidOpenTerminal and onDidCloseTerminal', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(term.name, 'c'); + try { + equal(term.name, 'c'); + } catch (e) { + done(e); + } reg1.dispose(); const reg2 = window.onDidCloseTerminal(() => { reg2.dispose(); @@ -302,13 +342,21 @@ suite('window namespace tests', () => { test('should respect dimension overrides', (done) => { const reg1 = window.onDidOpenTerminal(term => { - equal(terminal, term); + try { + equal(terminal, term); + } catch (e) { + done(e); + } reg1.dispose(); term.show(); const reg2 = window.onDidChangeTerminalDimensions(e => { - equal(e.dimensions.columns, 10); - equal(e.dimensions.rows, 5); - equal(e.terminal, terminal); + try { + equal(e.dimensions.columns, 10); + equal(e.dimensions.rows, 5); + equal(e.terminal, terminal); + } catch (e) { + done(e); + } reg2.dispose(); const reg3 = window.onDidCloseTerminal(() => { reg3.dispose(); diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts index 525b5a9318c..e4344f0d58a 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts @@ -23,7 +23,11 @@ suite('workspace-namespace', () => { reg1.dispose(); const reg2 = vscode.window.onDidWriteTerminalData(e => { reg2.dispose(); - assert.equal(e.data, 'testing\r\n'); + try { + assert.equal(e.data, 'testing\r\n'); + } catch (e) { + done(e); + } term.dispose(); }); }); From fe2e842fee3f1f6ec878c4e56017410f174718fb Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 31 Oct 2019 11:21:18 -0700 Subject: [PATCH 3/4] Ensure terminal/task tests don't leak listeners across tests --- .../src/singlefolder-tests/terminal.test.ts | 108 +++++++----------- .../workspace.tasks.test.ts | 63 ++++++---- 2 files changed, 80 insertions(+), 91 deletions(-) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index e25292bf250..512fd8e646e 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import { window, Pseudoterminal, EventEmitter, TerminalDimensions, workspace, ConfigurationTarget } from 'vscode'; +import { window, Pseudoterminal, EventEmitter, TerminalDimensions, workspace, ConfigurationTarget, Disposable } from 'vscode'; import { doesNotThrow, equal, ok, deepEqual } from 'assert'; suite('window namespace tests', () => { @@ -12,49 +12,47 @@ suite('window namespace tests', () => { await workspace.getConfiguration('terminal.integrated').update('windowsEnableConpty', false, ConfigurationTarget.Global); }); suite('Terminal', () => { + let disposables: Disposable[] = []; + + teardown(() => { + disposables.forEach(d => d.dispose()); + disposables.length = 0; + }); + test('sendText immediately after createTerminal should not throw', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(terminal, term); } catch (e) { done(e); } terminal.dispose(); - reg1.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); - }); + disposables.push(window.onDidCloseTerminal(() => done())); + })); const terminal = window.createTerminal(); doesNotThrow(terminal.sendText.bind(terminal, 'echo "foo"')); }); test('onDidCloseTerminal event fires when terminal is disposed', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(terminal, term); } catch (e) { done(e); } terminal.dispose(); - reg1.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); - }); + disposables.push(window.onDidCloseTerminal(() => done())); + })); const terminal = window.createTerminal(); }); test('processId immediately after createTerminal should fetch the pid', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(terminal, term); } catch (e) { done(e); } - reg1.dispose(); terminal.processId.then(id => { try { ok(id > 0); @@ -62,29 +60,22 @@ suite('window namespace tests', () => { done(e); } terminal.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); + disposables.push(window.onDidCloseTerminal(() => done())); }); - }); + })); const terminal = window.createTerminal(); }); test('name in constructor should set terminal.name', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(terminal, term); } catch (e) { done(e); } terminal.dispose(); - reg1.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); - }); + disposables.push(window.onDidCloseTerminal(() => done())); + })); const terminal = window.createTerminal('a'); try { equal(terminal.name, 'a'); @@ -94,19 +85,15 @@ suite('window namespace tests', () => { }); test('onDidOpenTerminal should fire when a terminal is created', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(term.name, 'b'); } catch (e) { done(e); } - reg1.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); + disposables.push(window.onDidCloseTerminal(() => done())); terminal.dispose(); - }); + })); const terminal = window.createTerminal('b'); }); // test('onDidChangeActiveTerminal should fire when new terminals are created', (done) => { @@ -182,7 +169,7 @@ suite('window namespace tests', () => { suite('hideFromUser', () => { test('should be available to terminals API', done => { const terminal = window.createTerminal({ name: 'bg', hideFromUser: true }); - window.onDidOpenTerminal(t => { + disposables.push(window.onDidOpenTerminal(t => { try { equal(t, terminal); equal(t.name, 'bg'); @@ -190,12 +177,12 @@ suite('window namespace tests', () => { } catch (e) { done(e); } - const reg3 = window.onDidCloseTerminal(() => { - reg3.dispose(); + disposables.push(window.onDidCloseTerminal(() => { + // reg3.dispose(); done(); - }); + })); terminal.dispose(); - }); + })); }); }); @@ -204,17 +191,17 @@ suite('window namespace tests', () => { const openEvents: string[] = []; const dataEvents: { name: string, data: string }[] = []; const closeEvents: string[] = []; - const reg1 = window.onDidOpenTerminal(e => openEvents.push(e.name)); + disposables.push(window.onDidOpenTerminal(e => openEvents.push(e.name))); let resolveOnceDataWritten: (() => void) | undefined; - const reg2 = window.onDidWriteTerminalData(e => { + disposables.push(window.onDidWriteTerminalData(e => { dataEvents.push({ name: e.terminal.name, data: e.data }); resolveOnceDataWritten!(); - }); + })); - const reg3 = window.onDidCloseTerminal(e => { + disposables.push(window.onDidCloseTerminal(e => { closeEvents.push(e.name); try { @@ -226,16 +213,12 @@ suite('window namespace tests', () => { deepEqual(openEvents, ['test1', 'test2']); deepEqual(dataEvents, [{ name: 'test1', data: 'write1' }, { name: 'test2', data: 'write2' }]); deepEqual(closeEvents, ['test1', 'test2']); - - reg1.dispose(); - reg2.dispose(); - reg3.dispose(); done(); } } catch (e) { done(e); } - }); + })); const term1Write = new EventEmitter(); const term1Close = new EventEmitter(); @@ -272,19 +255,15 @@ suite('window namespace tests', () => { suite('Extension pty terminals', () => { test('should fire onDidOpenTerminal and onDidCloseTerminal', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(term.name, 'c'); } catch (e) { done(e); } - reg1.dispose(); - const reg2 = window.onDidCloseTerminal(() => { - reg2.dispose(); - done(); - }); + disposables.push(window.onDidCloseTerminal(() => done())); term.dispose(); - }); + })); const pty: Pseudoterminal = { onDidWrite: new EventEmitter().event, open: () => { }, @@ -341,15 +320,14 @@ suite('window namespace tests', () => { // }); test('should respect dimension overrides', (done) => { - const reg1 = window.onDidOpenTerminal(term => { + disposables.push(window.onDidOpenTerminal(term => { try { equal(terminal, term); } catch (e) { done(e); } - reg1.dispose(); term.show(); - const reg2 = window.onDidChangeTerminalDimensions(e => { + disposables.push(window.onDidChangeTerminalDimensions(e => { try { equal(e.dimensions.columns, 10); equal(e.dimensions.rows, 5); @@ -357,14 +335,10 @@ suite('window namespace tests', () => { } catch (e) { done(e); } - reg2.dispose(); - const reg3 = window.onDidCloseTerminal(() => { - reg3.dispose(); - done(); - }); + disposables.push(window.onDidCloseTerminal(() => done())); terminal.dispose(); - }); - }); + })); + })); const writeEmitter = new EventEmitter(); const overrideDimensionsEmitter = new EventEmitter(); const pty: Pseudoterminal = { diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts index e4344f0d58a..edb404b8653 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/workspace.tasks.test.ts @@ -4,14 +4,20 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; -import * as vscode from 'vscode'; +import { window, tasks, Disposable, TaskDefinition, Task, EventEmitter, CustomExecution, Pseudoterminal, TaskScope, commands, Task2 } from 'vscode'; suite('workspace-namespace', () => { suite('Tasks', () => { + let disposables: Disposable[] = []; + + teardown(() => { + disposables.forEach(d => d.dispose()); + disposables.length = 0; + }); test('CustomExecution task should start and shutdown successfully', (done) => { - interface CustomTestingTaskDefinition extends vscode.TaskDefinition { + interface CustomTestingTaskDefinition extends TaskDefinition { /** * One of the task properties. This can be used to customize the task in the tasks.json */ @@ -19,49 +25,58 @@ suite('workspace-namespace', () => { } const taskType: string = 'customTesting'; const taskName = 'First custom task'; - const reg1 = vscode.window.onDidOpenTerminal(term => { - reg1.dispose(); - const reg2 = vscode.window.onDidWriteTerminalData(e => { - reg2.dispose(); + let isPseudoterminalClosed = false; + disposables.push(window.onDidOpenTerminal(term => { + disposables.push(window.onDidWriteTerminalData(e => { try { assert.equal(e.data, 'testing\r\n'); } catch (e) { done(e); } + disposables.push(window.onDidCloseTerminal(() => { + try { + // Pseudoterminal.close should have fired by now, additionally we want + // to make sure all events are flushed before continuing with more tests + assert.ok(isPseudoterminalClosed); + } catch (e) { + done(e); + return; + } + done(); + })); term.dispose(); - }); - }); - const taskProvider = vscode.tasks.registerTaskProvider(taskType, { + })); + })); + disposables.push(tasks.registerTaskProvider(taskType, { provideTasks: () => { - const result: vscode.Task[] = []; + const result: Task[] = []; const kind: CustomTestingTaskDefinition = { type: taskType, customProp1: 'testing task one' }; - const writeEmitter = new vscode.EventEmitter(); - const execution = new vscode.CustomExecution((): Thenable => { - const pty: vscode.Pseudoterminal = { + const writeEmitter = new EventEmitter(); + const execution = new CustomExecution((): Thenable => { + const pty: Pseudoterminal = { onDidWrite: writeEmitter.event, - open: () => { - writeEmitter.fire('testing\r\n'); - }, - close: () => { - taskProvider.dispose(); - done(); - } + open: () => writeEmitter.fire('testing\r\n'), + close: () => isPseudoterminalClosed = true }; return Promise.resolve(pty); }); - const task = new vscode.Task2(kind, vscode.TaskScope.Workspace, taskName, taskType, execution); + const task = new Task2(kind, TaskScope.Workspace, taskName, taskType, execution); result.push(task); return result; }, - resolveTask(_task: vscode.Task): vscode.Task | undefined { + resolveTask(_task: Task): Task | undefined { + try { assert.fail('resolveTask should not trigger during the test'); + } catch (e) { + done(e); + } return undefined; } - }); - vscode.commands.executeCommand('workbench.action.tasks.runTask', `${taskType}: ${taskName}`); + })); + commands.executeCommand('workbench.action.tasks.runTask', `${taskType}: ${taskName}`); }); }); }); From f5cb824330806744a03945e0dc6dee6f53667b61 Mon Sep 17 00:00:00 2001 From: Daniel Imms Date: Thu, 31 Oct 2019 11:39:03 -0700 Subject: [PATCH 4/4] Ignore 0,0 event to fix override dimensions pty test See #83778 for follow up (when we're not in endgame) --- .../vscode-api-tests/src/singlefolder-tests/terminal.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts index 24332cd4d30..3df611b17b4 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -96,6 +96,7 @@ suite('window namespace tests', () => { })); const terminal = window.createTerminal('b'); }); + // test('onDidChangeActiveTerminal should fire when new terminals are created', (done) => { // const reg1 = window.onDidChangeActiveTerminal((active: Terminal | undefined) => { // equal(active, terminal); @@ -341,6 +342,10 @@ suite('window namespace tests', () => { } term.show(); disposables.push(window.onDidChangeTerminalDimensions(e => { + if (e.dimensions.columns === 0 || e.dimensions.rows === 0) { + // HACK: Ignore the event if dimension(s) are zero (#83778) + return; + } try { equal(e.dimensions.columns, 10); equal(e.dimensions.rows, 5);