diff --git a/package.json b/package.json index f70ee99f2b92..f8f943752477 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "clean:src": "jlpm run clean", "clean:test": "lerna run clean --scope \"@jupyterlab/test-*\"", "clean:utils": "cd buildutils && jlpm run clean", - "coverage": "lerna run coverage --scope \"@jupyterlab/test-*\" --stream --concurrency 1", + "coverage": "lerna run coverage --scope \"@jupyterlab/test-*\" --stream --concurrency 1 --no-bail", "create:package": "node buildutils/lib/create-package.js", "create:test": "node buildutils/lib/create-test-package.js", "create:theme": "node buildutils/lib/create-theme.js", diff --git a/packages/apputils/src/clientsession.tsx b/packages/apputils/src/clientsession.tsx index 2ad1679586f5..a590d7cb73b2 100644 --- a/packages/apputils/src/clientsession.tsx +++ b/packages/apputils/src/clientsession.tsx @@ -878,26 +878,25 @@ export namespace ClientSession { * * Returns a promise resolving with whether the kernel was restarted. */ - export function restartKernel( + export async function restartKernel( kernel: Kernel.IKernelConnection ): Promise { let restartBtn = Dialog.warnButton({ label: 'RESTART ' }); - return showDialog({ + const result = await showDialog({ title: 'Restart Kernel?', body: 'Do you want to restart the current kernel? All variables will be lost.', buttons: [Dialog.cancelButton(), restartBtn] - }).then(result => { - if (kernel.isDisposed) { - return Promise.resolve(false); - } - if (result.button.accept) { - return kernel.restart().then(() => { - return true; - }); - } - return false; }); + + if (kernel.isDisposed) { + return false; + } + if (result.button.accept) { + await kernel.restart(); + return true; + } + return false; } /** diff --git a/packages/apputils/src/toolbar.tsx b/packages/apputils/src/toolbar.tsx index d82c3953fd17..5f110afe7652 100644 --- a/packages/apputils/src/toolbar.tsx +++ b/packages/apputils/src/toolbar.tsx @@ -3,6 +3,8 @@ import { UseSignal, ReactWidget } from './vdom'; +import { Kernel } from '@jupyterlab/services'; + import { Button } from '@jupyterlab/ui-components'; import { IIterator, find, map, some } from '@phosphor/algorithm'; @@ -400,8 +402,7 @@ export namespace Toolbar { * Create a kernel status indicator item. * * #### Notes - * It show display a busy status if the kernel status is - * not idle. + * It will show a busy status if the kernel status is busy. * It will show the current status in the node title. * It can handle a change to the context or the kernel. */ @@ -679,10 +680,20 @@ namespace Private { return; } let status = session.status; - this.toggleClass(TOOLBAR_IDLE_CLASS, status === 'idle'); - this.toggleClass(TOOLBAR_BUSY_CLASS, status !== 'idle'); + const busy = this._isBusy(status); + this.toggleClass(TOOLBAR_BUSY_CLASS, busy); + this.toggleClass(TOOLBAR_IDLE_CLASS, !busy); let title = 'Kernel ' + status[0].toUpperCase() + status.slice(1); this.node.title = title; } + + /** + * Check if status should be shown as busy. + */ + private _isBusy(status: Kernel.Status): boolean { + return ( + status === 'busy' || status === 'starting' || status === 'restarting' + ); + } } } diff --git a/packages/console/src/widget.ts b/packages/console/src/widget.ts index d10cb725a1cc..90b2d4847b5d 100644 --- a/packages/console/src/widget.ts +++ b/packages/console/src/widget.ts @@ -784,8 +784,7 @@ export class CodeConsole extends Widget { if (!kernel) { return; } - kernel - .requestKernelInfo() + kernel.ready .then(() => { if (this.isDisposed || !kernel || !kernel.info) { return; @@ -797,6 +796,7 @@ export class CodeConsole extends Widget { }); } else if (this.session.status === 'restarting') { this.addBanner(); + this._handleInfo(this.session.kernel.info); } } diff --git a/packages/notebook/src/panel.ts b/packages/notebook/src/panel.ts index 32182ae5cc18..cc309fdf9cf7 100644 --- a/packages/notebook/src/panel.ts +++ b/packages/notebook/src/panel.ts @@ -9,7 +9,12 @@ import { Message } from '@phosphor/messaging'; import { ISignal, Signal } from '@phosphor/signaling'; -import { IClientSession, Printing } from '@jupyterlab/apputils'; +import { + IClientSession, + Printing, + showDialog, + Dialog +} from '@jupyterlab/apputils'; import { DocumentWidget } from '@jupyterlab/docregistry'; @@ -51,6 +56,10 @@ export class NotebookPanel extends DocumentWidget { // Set up things related to the context this.content.model = this.context.model; this.context.session.kernelChanged.connect(this._onKernelChanged, this); + this.context.session.statusChanged.connect( + this._onSessionStatusChanged, + this + ); void this.revealed.then(() => { // Set the document edit mode on initial open if it looks like a new document. @@ -184,13 +193,40 @@ export class NotebookPanel extends DocumentWidget { } let { newValue } = args; void newValue.ready.then(() => { - if (this.model) { + if (this.model && this.context.session.kernel === newValue) { this._updateLanguage(newValue.info.language_info); } }); void this._updateSpec(newValue); } + private _onSessionStatusChanged( + sender: IClientSession, + status: Kernel.Status + ) { + // If the status is autorestarting, and we aren't already in a series of + // autorestarts, show the dialog. + if (status === 'autorestarting' && !this._autorestarting) { + // The kernel died and the server is restarting it. We notify the user so + // they know why their kernel state is gone. + void showDialog({ + title: 'Kernel Restarting', + body: `The kernel for ${ + this.session.path + } appears to have died. It will restart automatically.`, + buttons: [Dialog.okButton()] + }); + this._autorestarting = true; + } else if (status === 'restarting') { + // Another autorestart attempt will first change the status to + // restarting, then to autorestarting again, so we don't reset the + // autorestarting status if the status is 'restarting'. + /* no-op */ + } else { + this._autorestarting = false; + } + } + /** * Update the kernel language. */ @@ -215,6 +251,12 @@ export class NotebookPanel extends DocumentWidget { } private _activated = new Signal(this); + + /** + * Whether we are currently in a series of autorestarts we have already + * notified the user about. + */ + private _autorestarting = false; } /** diff --git a/packages/services/src/kernel/default.ts b/packages/services/src/kernel/default.ts index 58f3262cf01f..679d98568117 100644 --- a/packages/services/src/kernel/default.ts +++ b/packages/services/src/kernel/default.ts @@ -57,8 +57,11 @@ export class DefaultKernel implements Kernel.IKernel { options.serverSettings || ServerConnection.makeSettings(); this._clientId = options.clientId || UUID.uuid4(); this._username = options.username || ''; - this._futures = new Map(); - this._comms = new Map(); + + void this._readyPromise.promise.then(() => { + this._sendPending(); + }); + this._createSocket(); Private.runningKernels.push(this); } @@ -187,7 +190,7 @@ export class DefaultKernel implements Kernel.IKernel { * A promise that is fulfilled when the kernel is ready. */ get ready(): Promise { - return this._connectionPromise.promise; + return this._readyPromise.promise; } /** @@ -227,10 +230,9 @@ export class DefaultKernel implements Kernel.IKernel { return; } this._isDisposed = true; - this._terminated.emit(void 0); + this._terminated.emit(); this._status = 'dead'; - // TODO: Kernel status rework should avoid doing - // anything asynchronous in the disposal. + // Trigger the async _clearState, but do not wait for it. void this._clearState(); this._clearSocket(); this._kernelSession = ''; @@ -349,7 +351,6 @@ export class DefaultKernel implements Kernel.IKernel { async handleRestart(): Promise { await this._clearState(); this._updateStatus('restarting'); - this._clearSocket(); } /** @@ -362,7 +363,7 @@ export class DefaultKernel implements Kernel.IKernel { this._clearSocket(); this._updateStatus('reconnecting'); this._createSocket(); - return this._connectionPromise.promise; + return this._readyPromise.promise; } /** @@ -376,8 +377,8 @@ export class DefaultKernel implements Kernel.IKernel { * On a valid response, closes the websocket and disposes of the kernel * object, and fulfills the promise. * - * The promise will be rejected if the kernel status is `Dead` or if the - * request fails or the response is invalid. + * If the kernel is already `dead`, it closes the websocket and returns + * without a server request. */ async shutdown(): Promise { if (this.status === 'dead') { @@ -398,10 +399,6 @@ export class DefaultKernel implements Kernel.IKernel { * * Fulfills with the `kernel_info_response` content when the shell reply is * received and validated. - * - * TODO: this should be automatically run every time our kernel restarts, - * before we say the kernel is ready, and cache the info and the kernel - * session id. Further calls to this should returned the cached results. */ async requestKernelInfo(): Promise { let options: KernelMessage.IOptions = { @@ -787,10 +784,13 @@ export class DefaultKernel implements Kernel.IKernel { /** * Clear the socket state. + * + * #### Notes + * When calling this, you should also set the status to something like + * 'reconnecting' to reset the kernel ready state. */ private _clearSocket(): void { this._wsStopped = true; - this._isReady = false; if (this._ws !== null) { // Clear the websocket event handlers and the socket itself. this._ws.onopen = this._noOp; @@ -807,16 +807,44 @@ export class DefaultKernel implements Kernel.IKernel { */ private _updateStatus(status: Kernel.Status): void { switch (status) { - case 'starting': case 'idle': case 'busy': - case 'connected': - this._isReady = true; + if (!this._isReady && this._initialized) { + this._isReady = true; + this._readyPromise.resolve(); + } break; case 'restarting': + // Send a kernel_info_request to get to a known kernel state. + void this.requestKernelInfo().catch(this._noOp); + break; + case 'starting': + case 'autorestarting': + // 'starting' can happen at initialization or 'restarting'. + // 'autorestarting' is always preceded by 'restarting'. In either case, + // the 'restarting' handler above is fine, so we do nothing here. + /* no-op */ + break; + case 'connected': + // requestKernelInfo is sent by the onWSOpen + break; case 'reconnecting': + if (this._isReady) { + this._isReady = false; + this._readyPromise = new PromiseDelegate(); + void this._readyPromise.promise.then(() => { + // when we are ready again, send any pending messages. + this._sendPending(); + }); + } + break; case 'dead': - this._isReady = false; + if (this._isReady) { + this._isReady = false; + this._readyPromise = new PromiseDelegate(); + } + void this._readyPromise.promise.catch(this._noOp); + this._readyPromise.reject('Kernel is dead'); break; default: console.error('invalid kernel status:', status); @@ -830,9 +858,6 @@ export class DefaultKernel implements Kernel.IKernel { this.dispose(); } } - if (this._isReady) { - this._sendPending(); - } } /** @@ -853,12 +878,11 @@ export class DefaultKernel implements Kernel.IKernel { * Clear the internal state. */ private async _clearState(): Promise { - this._isReady = false; this._pendingMessages = []; - const futuresResolved: Promise[] = []; + const futuresResolved: Promise[] = []; this._futures.forEach(future => { + futuresResolved.push(future.done.then(this._noOp, this._noOp)); future.dispose(); - futuresResolved.push(future.done); }); this._comms.forEach(comm => { comm.dispose(); @@ -870,9 +894,7 @@ export class DefaultKernel implements Kernel.IKernel { this._displayIdToParentIds.clear(); this._msgIdToDisplayIds.clear(); - await Promise.all(futuresResolved).catch(() => { - /* no-op */ - }); + await Promise.all(futuresResolved); } /** @@ -1006,7 +1028,6 @@ export class DefaultKernel implements Kernel.IKernel { url = url + `&token=${encodeURIComponent(token)}`; } - this._connectionPromise = new PromiseDelegate(); this._wsStopped = false; this._ws = new settings.WebSocket(url); @@ -1024,19 +1045,26 @@ export class DefaultKernel implements Kernel.IKernel { */ private _onWSOpen = (evt: Event) => { this._reconnectAttempt = 0; - // Allow the message to get through. - this._isReady = true; - // Update our status to connected. this._updateStatus('connected'); + + // We temporarily set the ready status to true so our kernel info request + // below will go through. + this._isReady = true; + // Get the kernel info, signaling that the kernel is ready. - // TODO: requestKernelInfo shouldn't make a request, but should return cached info? this.requestKernelInfo() .then(() => { - this._connectionPromise.resolve(void 0); + this._initialized = true; + this._isReady = true; + this._readyPromise.resolve(); }) .catch(err => { - this._connectionPromise.reject(err); + this._initialized = true; + this._readyPromise.reject(err); }); + + // Reset the isReady status after we sent our message so others wait for + // the kernel info request to come back. this._isReady = false; }; @@ -1121,9 +1149,22 @@ export class DefaultKernel implements Kernel.IKernel { switch (msg.header.msg_type) { case 'status': // Updating the status is synchronous, and we call no async user code - this._updateStatus( - (msg as KernelMessage.IStatusMsg).content.execution_state - ); + let executionState = (msg as KernelMessage.IStatusMsg).content + .execution_state; + this._updateStatus(executionState); + if (executionState === 'restarting') { + // After processing for this message is completely done, we want to + // handle this restart, so we don't await, but instead schedule the + // work as a microtask. We schedule this here so that it comes + // before any microtasks scheduled in the signal emission below. + void Promise.resolve().then(async () => { + // handleRestart changes the status to 'restarting', so we call it + // first so that the status won't flip back and forth between + // 'restarting' and 'autorestarting'. + await this.handleRestart(); + this._updateStatus('autorestarting'); + }); + } break; case 'comm_open': await this._handleCommOpen(msg as KernelMessage.ICommOpenMsg); @@ -1166,9 +1207,6 @@ export class DefaultKernel implements Kernel.IKernel { this._reconnectAttempt += 1; } else { this._updateStatus('dead'); - this._connectionPromise.reject( - new Error('Could not establish connection') - ); } }; @@ -1184,8 +1222,10 @@ export class DefaultKernel implements Kernel.IKernel { private _reconnectLimit = 7; private _reconnectAttempt = 0; private _isReady = false; - private _futures: Map; - private _comms: Map; + private _readyPromise = new PromiseDelegate(); + private _initialized = false; + private _futures = new Map(); + private _comms = new Map(); private _targetRegistry: { [key: string]: ( comm: Kernel.IComm, @@ -1194,7 +1234,6 @@ export class DefaultKernel implements Kernel.IKernel { } = Object.create(null); private _info: KernelMessage.IInfoReply | null = null; private _pendingMessages: KernelMessage.IMessage[] = []; - private _connectionPromise: PromiseDelegate; private _specPromise: Promise; private _statusChanged = new Signal(this); private _iopubMessage = new Signal(this); @@ -1530,11 +1569,6 @@ namespace Private { ); let init = { method: 'POST' }; - // TODO: If we handleRestart before making the server request, we sever the - // communication link before the shutdown_reply message comes, so we end up - // getting the shutdown_reply messages after we reconnect, which is weird. - // We might want to move the handleRestart to after we get the response back - // Handle the restart on all of the kernels with the same id. await Promise.all( runningKernels.filter(k => k.id === kernel.id).map(k => k.handleRestart()) @@ -1545,13 +1579,6 @@ namespace Private { } let data = await response.json(); validate.validateModel(data); - // Reconnect the other kernels asynchronously, but don't wait for them. - each(runningKernels, k => { - if (k !== kernel && k.id === kernel.id) { - void k.reconnect(); - } - }); - await kernel.reconnect(); } /** diff --git a/packages/services/src/kernel/kernel.ts b/packages/services/src/kernel/kernel.ts index 8ba427ac25b6..9ec0639a7935 100644 --- a/packages/services/src/kernel/kernel.ts +++ b/packages/services/src/kernel/kernel.ts @@ -929,6 +929,7 @@ export namespace Kernel { | 'idle' | 'busy' | 'restarting' + | 'autorestarting' | 'dead' | 'connected'; diff --git a/tests/test-apputils/src/clientsession.spec.ts b/tests/test-apputils/src/clientsession.spec.ts index fb9a99ba513d..69564309fc4b 100644 --- a/tests/test-apputils/src/clientsession.spec.ts +++ b/tests/test-apputils/src/clientsession.spec.ts @@ -9,7 +9,11 @@ import { ClientSession, Dialog, IClientSession } from '@jupyterlab/apputils'; import { UUID } from '@phosphor/coreutils'; -import { acceptDialog, dismissDialog } from '@jupyterlab/testutils'; +import { + acceptDialog, + dismissDialog, + testEmission +} from '@jupyterlab/testutils'; describe('@jupyterlab/apputils', () => { describe('ClientSession', () => { @@ -313,21 +317,19 @@ describe('@jupyterlab/apputils', () => { describe('#restart()', () => { it('should restart if the user accepts the dialog', async () => { - let called = false; - - await session.initialize(); - session.statusChanged.connect((sender, args) => { - if (args === 'restarting') { - called = true; - } + const emission = testEmission(session.statusChanged, { + find: (_, args) => args === 'restarting' }); - + await session.initialize(); + await session.kernel.ready; const restart = session.restart(); await acceptDialog(); expect(await restart).to.equal(true); - expect(called).to.equal(true); - }, 30000); + await emission; + // Wait for the restarted kernel to be ready + await session.kernel.ready; + }); it('should not restart if the user rejects the dialog', async () => { let called = false; @@ -388,28 +390,30 @@ describe('@jupyterlab/apputils', () => { } }); await session.initialize(); + await session.kernel.ready; const restart = ClientSession.restartKernel(session.kernel); await acceptDialog(); - await restart; + expect(await restart).to.equal(true); expect(called).to.equal(true); }, 30000); // Allow for slower CI it('should not restart if the user rejects the dialog', async () => { let called = false; - await session.initialize(); session.statusChanged.connect((sender, args) => { if (args === 'restarting') { called = true; } }); + await session.initialize(); + await session.kernel.ready; const restart = ClientSession.restartKernel(session.kernel); await dismissDialog(); - await restart; + expect(await restart).to.equal(false); expect(called).to.equal(false); }, 30000); // Allow for slower CI }); diff --git a/tests/test-apputils/src/toolbar.spec.ts b/tests/test-apputils/src/toolbar.spec.ts index 161de6eab1fd..6f079b9852be 100644 --- a/tests/test-apputils/src/toolbar.spec.ts +++ b/tests/test-apputils/src/toolbar.spec.ts @@ -14,30 +14,23 @@ import { toArray } from '@phosphor/algorithm'; import { CommandRegistry } from '@phosphor/commands'; -// import { ReadonlyJSONObject } from '@phosphor/coreutils'; - -// import { Message } from '@phosphor/messaging'; +import { ReadonlyJSONObject } from '@phosphor/coreutils'; import { Widget } from '@phosphor/widgets'; import { simulate } from 'simulate-event'; import { createClientSession, framePromise } from '@jupyterlab/testutils'; -import { ReadonlyJSONObject } from '@phosphor/coreutils'; describe('@jupyterlab/apputils', () => { let widget: Toolbar; - let session: ClientSession; beforeEach(async () => { widget = new Toolbar(); - session = await createClientSession(); }); afterEach(async () => { widget.dispose(); - await session.shutdown(); - session.dispose(); }); describe('Toolbar', () => { @@ -281,87 +274,102 @@ describe('@jupyterlab/apputils', () => { }); }); - describe('.createInterruptButton()', () => { - it("should have the `'jp-StopIcon'` class", async () => { - const button = Toolbar.createInterruptButton(session); - Widget.attach(button, document.body); - await framePromise(); - expect(button.node.querySelector('.jp-StopIcon')).to.exist; + describe('Kernel buttons', () => { + let session: ClientSession; + beforeEach(async () => { + session = await createClientSession(); }); - }); - describe('.createRestartButton()', () => { - it("should have the `'jp-RefreshIcon'` class", async () => { - const button = Toolbar.createRestartButton(session); - Widget.attach(button, document.body); - await framePromise(); - expect(button.node.querySelector('.jp-RefreshIcon')).to.exist; + afterEach(async () => { + await session.shutdown(); + session.dispose(); }); - }); - describe('.createKernelNameItem()', () => { - it("should display the `'display_name'` of the kernel", async () => { - const item = Toolbar.createKernelNameItem(session); - await session.initialize(); - Widget.attach(item, document.body); - await framePromise(); - expect( - (item.node.firstChild.lastChild as HTMLElement).textContent - ).to.equal(session.kernelDisplayName); + describe('.createInterruptButton()', () => { + it("should have the `'jp-StopIcon'` class", async () => { + const button = Toolbar.createInterruptButton(session); + Widget.attach(button, document.body); + await framePromise(); + expect(button.node.querySelector('.jp-StopIcon')).to.exist; + }); }); - it("should display `'No Kernel!'` if there is no kernel", async () => { - const item = Toolbar.createKernelNameItem(session); - Widget.attach(item, document.body); - await framePromise(); - expect( - (item.node.firstChild.lastChild as HTMLElement).textContent - ).to.equal('No Kernel!'); + describe('.createRestartButton()', () => { + it("should have the `'jp-RefreshIcon'` class", async () => { + const button = Toolbar.createRestartButton(session); + Widget.attach(button, document.body); + await framePromise(); + expect(button.node.querySelector('.jp-RefreshIcon')).to.exist; + }); }); - }); - describe('.createKernelStatusItem()', () => { - beforeEach(async () => { - await session.initialize(); - await session.kernel.ready; - }); - - it('should display a busy status if the kernel status is not idle', async () => { - const item = Toolbar.createKernelStatusItem(session); - let called = false; - const future = session.kernel.requestExecute({ code: 'a = 1' }); - future.onIOPub = msg => { - if (session.status === 'busy') { - expect(item.hasClass('jp-FilledCircleIcon')).to.equal(true); - called = true; - } - }; - await future.done; - expect(called).to.equal(true); - }); - - it('should show the current status in the node title', async () => { - const item = Toolbar.createKernelStatusItem(session); - const status = session.status; - expect(item.node.title.toLowerCase()).to.contain(status); - let called = false; - const future = session.kernel.requestExecute({ code: 'a = 1' }); - future.onIOPub = msg => { - if (session.status === 'busy') { - expect(item.node.title.toLowerCase()).to.contain('busy'); - called = true; - } - }; - await future.done; - expect(called).to.equal(true); - }); - - it('should handle a starting session', async () => { - await session.shutdown(); - session = await createClientSession(); - const item = Toolbar.createKernelStatusItem(session); - expect(item.node.title).to.equal('Kernel Starting'); - expect(item.hasClass('jp-FilledCircleIcon')).to.equal(true); + describe('.createKernelNameItem()', () => { + it("should display the `'display_name'` of the kernel", async () => { + const item = Toolbar.createKernelNameItem(session); + await session.initialize(); + Widget.attach(item, document.body); + await framePromise(); + expect( + (item.node.firstChild.lastChild as HTMLElement).textContent + ).to.equal(session.kernelDisplayName); + }); + + it("should display `'No Kernel!'` if there is no kernel", async () => { + const item = Toolbar.createKernelNameItem(session); + Widget.attach(item, document.body); + await framePromise(); + expect( + (item.node.firstChild.lastChild as HTMLElement).textContent + ).to.equal('No Kernel!'); + }); + }); + + describe('.createKernelStatusItem()', () => { + beforeEach(async () => { + await session.initialize(); + await session.kernel.ready; + }); + + it('should display a busy status if the kernel status is busy', async () => { + const item = Toolbar.createKernelStatusItem(session); + let called = false; + session.statusChanged.connect((_, status) => { + if (status === 'busy') { + expect(item.hasClass('jp-FilledCircleIcon')).to.equal(true); + called = true; + } + }); + const future = session.kernel.requestExecute({ code: 'a = 109\na' }); + await future.done; + expect(called).to.equal(true); + }); + + it('should show the current status in the node title', async () => { + const item = Toolbar.createKernelStatusItem(session); + const status = session.status; + expect(item.node.title.toLowerCase()).to.contain(status); + let called = false; + const future = session.kernel.requestExecute({ code: 'a = 1' }); + future.onIOPub = msg => { + if (session.status === 'busy') { + expect(item.node.title.toLowerCase()).to.contain('busy'); + called = true; + } + }; + await future.done; + expect(called).to.equal(true); + }); + + it('should handle a starting session', async () => { + await session.kernel.ready; + await session.shutdown(); + session = await createClientSession(); + const item = Toolbar.createKernelStatusItem(session); + expect(item.node.title).to.equal('Kernel Starting'); + expect(item.hasClass('jp-FilledCircleIcon')).to.equal(true); + await session.initialize(); + await session.kernel.ready; + }); }); }); }); diff --git a/tests/test-docregistry/src/mimedocument.spec.ts b/tests/test-docregistry/src/mimedocument.spec.ts index 6824277ccdfe..d99c1d66182d 100644 --- a/tests/test-docregistry/src/mimedocument.spec.ts +++ b/tests/test-docregistry/src/mimedocument.spec.ts @@ -17,7 +17,11 @@ import { import { RenderedText, IRenderMime } from '@jupyterlab/rendermime'; -import { createFileContext, defaultRenderMime } from '@jupyterlab/testutils'; +import { + createFileContext, + defaultRenderMime, + testEmission +} from '@jupyterlab/testutils'; const RENDERMIME = defaultRenderMime(); @@ -111,10 +115,10 @@ describe('docregistry/mimedocument', () => { it('should change the document contents', async () => { RENDERMIME.addFactory(fooFactory); await dContext.initialize(true); - let called = false; - dContext.model.contentChanged.connect(() => { - expect(dContext.model.toString()).to.equal('bar'); - called = true; + const emission = testEmission(dContext.model.contentChanged, { + test: () => { + expect(dContext.model.toString()).to.equal('bar'); + } }); const renderer = RENDERMIME.createRenderer('text/foo'); const widget = new LogRenderer({ @@ -125,7 +129,7 @@ describe('docregistry/mimedocument', () => { dataType: 'string' }); await widget.ready; - expect(called).to.equal(true); + await emission; }); }); }); diff --git a/tests/test-services/src/contents/index.spec.ts b/tests/test-services/src/contents/index.spec.ts index 1ff24a4deacc..63ee387b6c9c 100644 --- a/tests/test-services/src/contents/index.spec.ts +++ b/tests/test-services/src/contents/index.spec.ts @@ -782,7 +782,7 @@ describe('drive', () => { }); }); - describe('#newUntitled()', async () => { + describe('#newUntitled()', () => { it('should create a file', async () => { const drive = new Drive(); handleRequest(drive, 201, DEFAULT_FILE); diff --git a/tests/test-services/src/kernel/comm.spec.ts b/tests/test-services/src/kernel/comm.spec.ts index 94bb73720e34..9daf0846afe4 100644 --- a/tests/test-services/src/kernel/comm.spec.ts +++ b/tests/test-services/src/kernel/comm.spec.ts @@ -20,12 +20,14 @@ comm.close("goodbye") `; const RECEIVE = ` -def _recv(msg): - data = msg["content"]["data"] - if data == "quit": - comm.close("goodbye") - else: - comm.send(data) +def create_recv(comm): + def _recv(msg): + data = msg["content"]["data"] + if data == "quit": + comm.close("goodbye") + else: + comm.send(data) + return _recv `; const SEND = ` @@ -33,13 +35,13 @@ ${RECEIVE} from ipykernel.comm import Comm comm = Comm(target_name="test", data="hello") comm.send(data="hello") -comm.on_msg(_recv) +comm.on_msg(create_recv(comm)) `; const TARGET = ` ${RECEIVE} def target_func(comm, msg): - comm.on_msg(_recv) + comm.on_msg(create_recv(comm)) get_ipython().kernel.comm_manager.register_target("test", target_func) `; diff --git a/tests/test-services/src/kernel/ikernel.spec.ts b/tests/test-services/src/kernel/ikernel.spec.ts index 09694dc1256e..03e2235b6fd0 100644 --- a/tests/test-services/src/kernel/ikernel.spec.ts +++ b/tests/test-services/src/kernel/ikernel.spec.ts @@ -68,7 +68,7 @@ describe('Kernel.IKernel', () => { }); }); - describe('#iopubMessage', async () => { + describe('#iopubMessage', () => { it('should be emitted for an iopub message', async () => { let called = false; defaultKernel.iopubMessage.connect((k, msg) => { @@ -303,9 +303,7 @@ describe('Kernel.IKernel', () => { await emission; }); - // TODO: seems to be sporadically timing out if we await the restart. See - // https://github.com/jupyter/notebook/issues/3705. - it.skip('should get a restarting status', async () => { + it('should get a restarting status', async () => { const emission = testEmission(defaultKernel.statusChanged, { find: () => defaultKernel.status === 'restarting' }); @@ -612,9 +610,7 @@ describe('Kernel.IKernel', () => { }); describe('#restart()', () => { - // TODO: seems to be sporadically timing out if we await the restart. See - // https://github.com/jupyter/notebook/issues/3705. - it.skip('should restart and resolve with a valid server response', async () => { + it('should restart and resolve with a valid server response', async () => { await defaultKernel.restart(); await defaultKernel.ready; }); @@ -646,9 +642,7 @@ describe('Kernel.IKernel', () => { await expectFailure(restart); }); - // TODO: seems to be sporadically timing out if we await the restart. See - // https://github.com/jupyter/notebook/issues/3705. - it.skip('should dispose of existing comm and future objects', async () => { + it('should dispose of existing comm and future objects', async () => { const kernel = defaultKernel; const comm = kernel.connectToComm('test'); const future = kernel.requestExecute({ code: 'foo' }); diff --git a/tests/test-services/src/terminal/manager.spec.ts b/tests/test-services/src/terminal/manager.spec.ts index a9ac38621cd4..f92f1ec87937 100644 --- a/tests/test-services/src/terminal/manager.spec.ts +++ b/tests/test-services/src/terminal/manager.spec.ts @@ -10,6 +10,7 @@ import { TerminalSession, TerminalManager } from '@jupyterlab/services'; +import { testEmission } from '../utils'; describe('terminal', () => { let manager: TerminalSession.IManager; @@ -123,27 +124,27 @@ describe('terminal', () => { }); it('should emit a runningChanged signal', async () => { - let called = false; session = await manager.startNew(); - manager.runningChanged.connect((sender, args) => { - expect(session.isDisposed).to.equal(false); - called = true; + const emission = testEmission(manager.runningChanged, { + test: () => { + expect(session.isDisposed).to.equal(false); + } }); await manager.shutdown(session.name); - expect(called).to.equal(true); + await emission; }); }); describe('#runningChanged', () => { it('should be emitted when the running terminals changed', async () => { - let called = false; - manager.runningChanged.connect((sender, args) => { - expect(sender).to.equal(manager); - expect(toArray(args).length).to.be.greaterThan(0); - called = true; + const emission = testEmission(manager.runningChanged, { + test: (sender, args) => { + expect(sender).to.equal(manager); + expect(toArray(args).length).to.be.greaterThan(0); + } }); await manager.startNew(); - expect(called).to.equal(true); + await emission; }); }); diff --git a/tests/test-services/src/workspace/manager.spec.ts b/tests/test-services/src/workspace/manager.spec.ts index 437b23b6bafa..189ad5873ef4 100644 --- a/tests/test-services/src/workspace/manager.spec.ts +++ b/tests/test-services/src/workspace/manager.spec.ts @@ -49,7 +49,7 @@ describe('workspace', () => { }); }); - describe('#list()', async () => { + describe('#list()', () => { it('should fetch a workspace list supporting arbitrary IDs', async () => { const ids = ['foo', 'bar', 'baz', 'f/o/o', 'b/a/r', 'b/a/z']; const promises = ids.map(id => diff --git a/testutils/src/index.ts b/testutils/src/index.ts index 1f9595be7109..1a6fefe89edf 100644 --- a/testutils/src/index.ts +++ b/testutils/src/index.ts @@ -216,40 +216,28 @@ export async function createNotebookContext( /** * Wait for a dialog to be attached to an element. */ -export function waitForDialog( - host?: HTMLElement, - timeout?: number +export async function waitForDialog( + host: HTMLElement = document.body, + timeout: number = 250 ): Promise { - return new Promise((resolve, reject) => { - let counter = 0; - const interval = 25; - const limit = Math.floor((timeout || 250) / interval); - const seek = () => { - if (++counter === limit) { - reject(new Error('Dialog not found')); - return; - } - - if ((host || document.body).getElementsByClassName('jp-Dialog')[0]) { - resolve(undefined); - return; - } - - setTimeout(seek, interval); - }; - - seek(); - }); + const interval = 25; + const limit = Math.floor(timeout / interval); + for (let counter = 0; counter < limit; counter++) { + if (host.getElementsByClassName('jp-Dialog')[0]) { + return; + } + await sleep(interval); + } + throw new Error('Dialog not found'); } /** * Accept a dialog after it is attached by accepting the default button. */ export async function acceptDialog( - host?: HTMLElement, - timeout?: number + host: HTMLElement = document.body, + timeout: number = 250 ): Promise { - host = host || document.body; await waitForDialog(host, timeout); const node = host.getElementsByClassName('jp-Dialog')[0]; @@ -266,11 +254,9 @@ export async function acceptDialog( * This promise will always resolve successfully. */ export async function dismissDialog( - host?: HTMLElement, - timeout?: number + host: HTMLElement = document.body, + timeout: number = 250 ): Promise { - host = host || document.body; - try { await waitForDialog(host, timeout); } catch (error) {