From 03a844a2e5517ae69d8df8764c3e402cf8c1e7e7 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 14 Nov 2021 23:39:46 -0800 Subject: [PATCH 1/4] Use async await in pty terminal test Part of #137155 --- .../src/singlefolder-tests/terminal.test.ts | 49 +++++++++---------- 1 file changed, 23 insertions(+), 26 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 c88fa9619d6..dafd6ca3010 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -28,7 +28,7 @@ import { assertNoRpc } from '../utils'; await config.update('environmentChangesRelaunch', false, ConfigurationTarget.Global); }); - suite('Terminal', () => { + suite.only('Terminal', () => { let disposables: Disposable[] = []; teardown(() => { @@ -491,31 +491,7 @@ import { assertNoRpc } from '../utils'; // const terminal = window.createTerminal({ name: 'foo', pty }); // }); - test('should respect dimension overrides', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - } catch (e) { - done(e); - return; - } - term.show(); - disposables.push(window.onDidChangeTerminalDimensions(e => { - // The default pty dimensions have a chance to appear here since override - // dimensions happens after the terminal is created. If so just ignore and - // wait for the right dimensions - if (e.dimensions.columns === 10 || e.dimensions.rows === 5) { - try { - equal(e.terminal, terminal); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(() => done())); - terminal.dispose(); - } - })); - })); + test('should respect dimension overrides', async () => { const writeEmitter = new EventEmitter(); const overrideDimensionsEmitter = new EventEmitter(); const pty: Pseudoterminal = { @@ -525,6 +501,27 @@ import { assertNoRpc } from '../utils'; close: () => { } }; const terminal = window.createTerminal({ name: 'foo', pty }); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t => { + if (t === terminal) { + r(); + } + })); + }); + await new Promise(r => { + disposables.push(window.onDidChangeTerminalDimensions(e => { + strictEqual(e.terminal, terminal); + // The default pty dimensions have a chance to appear here since override + // dimensions happens after the terminal is created. If so just ignore and + // wait for the right dimensions + console.log('onDidChangeTerminalDimensions', e); + if (e.dimensions.columns === 10 || e.dimensions.rows === 5) { + disposables.push(window.onDidCloseTerminal(() => r())); + terminal.dispose(); + } + })); + terminal.show(); + }); }); test('should change terminal name', (done) => { From b934d0934672fe975b5376551342f20eff67ca6d Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 14 Nov 2021 23:53:12 -0800 Subject: [PATCH 2/4] Move other extension terminal tests to async await Part of #137155 --- .../src/singlefolder-tests/terminal.test.ts | 210 ++++++++---------- 1 file changed, 90 insertions(+), 120 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 dafd6ca3010..d7139de67a4 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -28,7 +28,7 @@ import { assertNoRpc } from '../utils'; await config.update('environmentChangesRelaunch', false, ConfigurationTarget.Global); }); - suite.only('Terminal', () => { + suite('Terminal', () => { let disposables: Disposable[] = []; teardown(() => { @@ -425,23 +425,24 @@ import { assertNoRpc } from '../utils'; }); suite('Extension pty terminals', () => { - test('should fire onDidOpenTerminal and onDidCloseTerminal', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(term.name, 'c'); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(() => done())); - term.dispose(); - })); + test('should fire onDidOpenTerminal and onDidCloseTerminal', async () => { const pty: Pseudoterminal = { onDidWrite: new EventEmitter().event, open: () => { }, close: () => { } }; - window.createTerminal({ name: 'c', pty }); + const terminal = await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t => { + if (t.name === 'c') { + r(t); + } + })); + window.createTerminal({ name: 'c', pty }); + }); + await new Promise(r => { + disposables.push(window.onDidCloseTerminal(() => r())); + terminal.dispose(); + }); }); // The below tests depend on global UI state and each other @@ -500,13 +501,13 @@ import { assertNoRpc } from '../utils'; open: () => overrideDimensionsEmitter.fire({ columns: 10, rows: 5 }), close: () => { } }; - const terminal = window.createTerminal({ name: 'foo', pty }); - await new Promise(r => { + const terminal = await new Promise(r => { disposables.push(window.onDidOpenTerminal(t => { - if (t === terminal) { - r(); + if (t === created) { + r(t); } })); + const created = window.createTerminal({ name: 'foo', pty }); }); await new Promise(r => { disposables.push(window.onDidChangeTerminalDimensions(e => { @@ -524,26 +525,7 @@ import { assertNoRpc } from '../utils'; }); }); - test('should change terminal name', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - equal(terminal.name, 'foo'); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(t => { - try { - equal(terminal, t); - equal(terminal.name, 'bar'); - } catch (e) { - done(e); - return; - } - done(); - })); - })); + test('should change terminal name', async () => { const changeNameEmitter = new EventEmitter(); const closeEmitter = new EventEmitter(); const pty: Pseudoterminal = { @@ -556,29 +538,22 @@ import { assertNoRpc } from '../utils'; }, close: () => { } }; - const terminal = window.createTerminal({ name: 'foo', pty }); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t1 => { + if (t1 === created) { + disposables.push(window.onDidCloseTerminal(t2 => { + if (t2 === created) { + strictEqual(t1.name, 'bar'); + r(); + } + })); + } + })); + const created = window.createTerminal({ name: 'foo', pty }); + }); }); - test('exitStatus.code should be set to the exit code (undefined)', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - equal(terminal.exitStatus, undefined); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(t => { - try { - equal(terminal, t); - deepEqual(terminal.exitStatus, { code: undefined }); - } catch (e) { - done(e); - return; - } - done(); - })); - })); + test('exitStatus.code should be set to the exit code (undefined)', async () => { const writeEmitter = new EventEmitter(); const closeEmitter = new EventEmitter(); const pty: Pseudoterminal = { @@ -587,29 +562,23 @@ import { assertNoRpc } from '../utils'; open: () => closeEmitter.fire(undefined), close: () => { } }; - const terminal = window.createTerminal({ name: 'foo', pty }); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t1 => { + if (t1 === created) { + strictEqual(created.exitStatus, undefined); + disposables.push(window.onDidCloseTerminal(t2 => { + if (t2 === created) { + deepStrictEqual(created.exitStatus, { code: undefined }); + r(); + } + })); + } + })); + const created = window.createTerminal({ name: 'foo', pty }); + }); }); - test('exitStatus.code should be set to the exit code (zero)', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - equal(terminal.exitStatus, undefined); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(t => { - try { - equal(terminal, t); - deepEqual(terminal.exitStatus, { code: 0 }); - } catch (e) { - done(e); - return; - } - done(); - })); - })); + test('exitStatus.code should be set to the exit code (zero)', async () => { const writeEmitter = new EventEmitter(); const closeEmitter = new EventEmitter(); const pty: Pseudoterminal = { @@ -618,29 +587,23 @@ import { assertNoRpc } from '../utils'; open: () => closeEmitter.fire(0), close: () => { } }; - const terminal = window.createTerminal({ name: 'foo', pty }); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t1 => { + if (t1 === created) { + strictEqual(created.exitStatus, undefined); + disposables.push(window.onDidCloseTerminal(t2 => { + if (t2 === created) { + deepStrictEqual(created.exitStatus, { code: 0 }); + r(); + } + })); + } + })); + const created = window.createTerminal({ name: 'foo', pty }); + }); }); - test('exitStatus.code should be set to the exit code (non-zero)', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - equal(terminal.exitStatus, undefined); - } catch (e) { - done(e); - return; - } - disposables.push(window.onDidCloseTerminal(t => { - try { - equal(terminal, t); - deepEqual(terminal.exitStatus, { code: 22 }); - } catch (e) { - done(e); - return; - } - done(); - })); - })); + test('exitStatus.code should be set to the exit code (non-zero)', async () => { const writeEmitter = new EventEmitter(); const closeEmitter = new EventEmitter(); const pty: Pseudoterminal = { @@ -654,20 +617,23 @@ import { assertNoRpc } from '../utils'; }, close: () => { } }; - const terminal = window.createTerminal({ name: 'foo', pty }); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(t1 => { + if (t1 === created) { + strictEqual(created.exitStatus, undefined); + disposables.push(window.onDidCloseTerminal(t2 => { + if (t2 === created) { + deepStrictEqual(created.exitStatus, { code: 22 }); + r(); + } + })); + } + })); + const created = window.createTerminal({ name: 'foo', pty }); + }); }); - test('creationOptions should be set and readonly for ExtensionTerminalOptions terminals', (done) => { - disposables.push(window.onDidOpenTerminal(term => { - try { - equal(terminal, term); - } catch (e) { - done(e); - return; - } - terminal.dispose(); - disposables.push(window.onDidCloseTerminal(() => done())); - })); + test('creationOptions should be set and readonly for ExtensionTerminalOptions terminals', async () => { const writeEmitter = new EventEmitter(); const pty: Pseudoterminal = { onDidWrite: writeEmitter.event, @@ -675,16 +641,20 @@ import { assertNoRpc } from '../utils'; close: () => { } }; const options = { name: 'foo', pty }; - const terminal = window.createTerminal(options); - try { - equal(terminal.name, 'foo'); + await new Promise(r => { + disposables.push(window.onDidOpenTerminal(term => { + if (term === terminal) { + terminal.dispose(); + disposables.push(window.onDidCloseTerminal(() => r())); + } + })); + const terminal = window.createTerminal(options); + strictEqual(terminal.name, 'foo'); const terminalOptions = terminal.creationOptions as ExtensionTerminalOptions; - equal(terminalOptions.name, 'foo'); - equal(terminalOptions.pty, pty); + strictEqual(terminalOptions.name, 'foo'); + strictEqual(terminalOptions.pty, pty); throws(() => terminalOptions.name = 'bad', 'creationOptions should be readonly at runtime'); - } catch (e) { - done(e); - } + }); }); }); From 9366786ec63e80d6132b257e0a2cb44c6cb583e1 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Sun, 14 Nov 2021 23:55:29 -0800 Subject: [PATCH 3/4] Use optional chaining in ExtHostPseudoterminal --- .../api/common/extHostTerminalService.ts | 40 +++++++------------ 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 6eb760ba348..15e4b33753a 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -275,15 +275,11 @@ export class ExtHostPseudoterminal implements ITerminalChildProcess { } input(data: string): void { - if (this._pty.handleInput) { - this._pty.handleInput(data); - } + this._pty.handleInput?.(data); } resize(cols: number, rows: number): void { - if (this._pty.setDimensions) { - this._pty.setDimensions({ columns: cols, rows }); - } + this._pty.setDimensions?.({ columns: cols, rows }); } async processBinary(data: string): Promise { @@ -314,28 +310,22 @@ export class ExtHostPseudoterminal implements ITerminalChildProcess { startSendingEvents(initialDimensions: ITerminalDimensionsDto | undefined): void { // Attach the listeners this._pty.onDidWrite(e => this._onProcessData.fire(e)); - if (this._pty.onDidClose) { - this._pty.onDidClose((e: number | void = undefined) => { - this._onProcessExit.fire(e === void 0 ? undefined : e); - }); - } - if (this._pty.onDidOverrideDimensions) { - this._pty.onDidOverrideDimensions(e => { - if (e) { - this._onDidChangeProperty.fire({ type: ProcessPropertyType.OverrideDimensions, value: { cols: e.columns, rows: e.rows } }); - } - }); - } - if (this._pty.onDidChangeName) { - this._pty.onDidChangeName(title => { - this._onDidChangeProperty.fire({ type: ProcessPropertyType.Title, value: title }); - }); - } + this._pty.onDidClose?.((e: number | void = undefined) => { + this._onProcessExit.fire(e === void 0 ? undefined : e); + }); + this._pty.onDidOverrideDimensions?.(e => { + if (e) { + this._onDidChangeProperty.fire({ type: ProcessPropertyType.OverrideDimensions, value: { cols: e.columns, rows: e.rows } }); + } + }); + this._pty.onDidChangeName?.(title => { + this._onDidChangeProperty.fire({ type: ProcessPropertyType.Title, value: title }); + }); this._pty.open(initialDimensions ? initialDimensions : undefined); - if (this._pty.setDimensions && initialDimensions) { - this._pty.setDimensions(initialDimensions); + if (initialDimensions) { + this._pty.setDimensions?.(initialDimensions); } this._onProcessReady.fire({ pid: -1, cwd: '', capabilities: this._capabilities }); From f88820646b3d4e7f37f870461f7139b4f8d51cc1 Mon Sep 17 00:00:00 2001 From: Daniel Imms <2193314+Tyriar@users.noreply.github.com> Date: Mon, 15 Nov 2021 00:14:06 -0800 Subject: [PATCH 4/4] Tidy up --- .../vscode-api-tests/src/singlefolder-tests/terminal.test.ts | 1 - src/vs/workbench/api/common/extHostTerminalService.ts | 2 +- 2 files changed, 1 insertion(+), 2 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 d7139de67a4..b27f729f46d 100644 --- a/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts +++ b/extensions/vscode-api-tests/src/singlefolder-tests/terminal.test.ts @@ -515,7 +515,6 @@ import { assertNoRpc } from '../utils'; // The default pty dimensions have a chance to appear here since override // dimensions happens after the terminal is created. If so just ignore and // wait for the right dimensions - console.log('onDidChangeTerminalDimensions', e); if (e.dimensions.columns === 10 || e.dimensions.rows === 5) { disposables.push(window.onDidCloseTerminal(() => r())); terminal.dispose(); diff --git a/src/vs/workbench/api/common/extHostTerminalService.ts b/src/vs/workbench/api/common/extHostTerminalService.ts index 15e4b33753a..47452b0060b 100644 --- a/src/vs/workbench/api/common/extHostTerminalService.ts +++ b/src/vs/workbench/api/common/extHostTerminalService.ts @@ -576,7 +576,7 @@ export abstract class BaseExtHostTerminalService extends Disposable implements I protected _setupExtHostProcessListeners(id: number, p: ITerminalChildProcess): IDisposable { const disposables = new DisposableStore(); - disposables.add(p.onProcessReady((e: { pid: number, cwd: string }) => this._proxy.$sendProcessReady(id, e.pid, e.cwd))); + disposables.add(p.onProcessReady(e => this._proxy.$sendProcessReady(id, e.pid, e.cwd))); disposables.add(p.onDidChangeProperty(property => this._proxy.$sendProcessProperty(id, property))); // Buffer data events to reduce the amount of messages going to the renderer