Navigation Menu

Skip to content

Commit

Permalink
Refactor ipywidget tests to use real kernel to wait for idle (microso…
Browse files Browse the repository at this point in the history 
…ft#12695)
  • Loading branch information
@rchiodo
rchiodo committed Jul 1, 2020
1 parent 7c6d40a commit 3a926e2
Show file tree
Hide file tree
Showing 6 changed files with 114 additions and 98 deletions.
79 changes: 77 additions & 2 deletions src/client/datascience/baseJupyterSession.ts
View file
Expand Up @@ -10,12 +10,14 @@ import { Event, EventEmitter } from 'vscode';
import { CancellationToken } from 'vscode-jsonrpc';
import { ServerStatus } from '../../datascience-ui/interactive-common/mainState';
import { traceError, traceInfo, traceWarning } from '../common/logger';
import { waitForPromise } from '../common/utils/async';
import { sleep, waitForPromise } from '../common/utils/async';
import * as localize from '../common/utils/localize';
import { noop } from '../common/utils/misc';
import { PythonInterpreter } from '../pythonEnvironments/info';
import { sendTelemetryEvent } from '../telemetry';
import { Telemetry } from './constants';
import { Identifiers, Telemetry } from './constants';
import { JupyterInvalidKernelError } from './jupyter/jupyterInvalidKernelError';
import { JupyterWaitForIdleError } from './jupyter/jupyterWaitForIdleError';
import { JupyterKernelPromiseFailedError } from './jupyter/kernels/jupyterKernelPromiseFailedError';
import { KernelSelector } from './jupyter/kernels/kernelSelector';
import { LiveKernelModel } from './jupyter/kernels/types';
Expand Down Expand Up @@ -324,6 +326,79 @@ export abstract class BaseJupyterSession implements IJupyterSession {
interpreter?: PythonInterpreter
): Promise<ISessionWithSocket>;

protected async waitForIdleOnSession(session: ISessionWithSocket | undefined, timeout: number): Promise<void> {
if (session && session.kernel) {
traceInfo(`Waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`);
// tslint:disable-next-line: no-any
const statusHandler = (resolve: () => void, reject: (exc: any) => void, e: Kernel.Status | undefined) => {
if (e === 'idle') {
resolve();
} else if (e === 'dead') {
traceError('Kernel died while waiting for idle');
// If we throw an exception, make sure to shutdown the session as it's not usable anymore
this.shutdownSession(session, this.statusHandler).ignoreErrors();
reject(
new JupyterInvalidKernelError({
...session.kernel,
lastActivityTime: new Date(),
numberOfConnections: 0,
session: session.model
})
);
}
};

let statusChangeHandler: Slot<ISessionWithSocket, Kernel.Status> | undefined;
const kernelStatusChangedPromise = new Promise((resolve, reject) => {
statusChangeHandler = (_: ISessionWithSocket, e: Kernel.Status) => statusHandler(resolve, reject, e);
session.statusChanged.connect(statusChangeHandler);
});
let kernelChangedHandler: Slot<ISessionWithSocket, Session.IKernelChangedArgs> | undefined;
const statusChangedPromise = new Promise((resolve, reject) => {
kernelChangedHandler = (_: ISessionWithSocket, e: Session.IKernelChangedArgs) =>
statusHandler(resolve, reject, e.newValue?.status);
session.kernelChanged.connect(kernelChangedHandler);
});
const checkStatusPromise = new Promise(async (resolve) => {
// This function seems to cause CI builds to timeout randomly on
// different tests. Waiting for status to go idle doesn't seem to work and
// in the past, waiting on the ready promise doesn't work either. Check status with a maximum of 5 seconds
const startTime = Date.now();
while (
session &&
session.kernel &&
session.kernel.status !== 'idle' &&
Date.now() - startTime < timeout
) {
await sleep(100);
}
resolve();
});
await Promise.race([kernelStatusChangedPromise, statusChangedPromise, checkStatusPromise]);
traceInfo(`Finished waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`);

if (statusChangeHandler && session && session.statusChanged) {
session.statusChanged.disconnect(statusChangeHandler);
}
if (kernelChangedHandler && session && session.kernelChanged) {
session.kernelChanged.disconnect(kernelChangedHandler);
}

// If we didn't make it out in ten seconds, indicate an error
if (session.kernel && session.kernel.status === 'idle') {
// So that we don't have problems with ipywidgets, always register the default ipywidgets comm target.
// Restart sessions and retries might make this hard to do correctly otherwise.
session.kernel.registerCommTarget(Identifiers.DefaultCommTarget, noop);

return;
}

// If we throw an exception, make sure to shutdown the session as it's not usable anymore
this.shutdownSession(session, this.statusHandler).ignoreErrors();
throw new JupyterWaitForIdleError(localize.DataScience.jupyterLaunchTimedOut());
}
}

// Changes the current session.
protected setSession(session: ISessionWithSocket | undefined) {
const oldSession = this._session;
Expand Down
93 changes: 5 additions & 88 deletions src/client/datascience/jupyter/jupyterSession.ts
View file
@@ -1,32 +1,22 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import type {
Contents,
ContentsManager,
Kernel,
ServerConnection,
Session,
SessionManager
} from '@jupyterlab/services';
import type { Slot } from '@phosphor/signaling';
import type { Contents, ContentsManager, ServerConnection, Session, SessionManager } from '@jupyterlab/services';
import * as uuid from 'uuid/v4';
import { CancellationToken } from 'vscode-jsonrpc';
import { Cancellation } from '../../common/cancellation';
import { traceError, traceInfo } from '../../common/logger';
import { IOutputChannel } from '../../common/types';
import { sleep } from '../../common/utils/async';
import * as localize from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { PythonInterpreter } from '../../pythonEnvironments/info';
import { captureTelemetry } from '../../telemetry';
import { BaseJupyterSession, JupyterSessionStartError } from '../baseJupyterSession';
import { Identifiers, Telemetry } from '../constants';
import { Telemetry } from '../constants';
import { reportAction } from '../progress/decorator';
import { ReportableAction } from '../progress/types';
import { IJupyterConnection, IJupyterKernelSpec, ISessionWithSocket } from '../types';
import { JupyterInvalidKernelError } from './jupyterInvalidKernelError';
import { JupyterWaitForIdleError } from './jupyterWaitForIdleError';
import { JupyterWebSockets } from './jupyterWebSocket';
import { KernelSelector } from './kernels/kernelSelector';
import { LiveKernelModel } from './kernels/types';
Expand Down Expand Up @@ -63,9 +53,10 @@ export class JupyterSession extends BaseJupyterSession {
}

@reportAction(ReportableAction.JupyterSessionWaitForIdleSession)
public async waitForIdle(timeout: number): Promise<void> {
@captureTelemetry(Telemetry.WaitForIdleJupyter, undefined, true)
public waitForIdle(timeout: number): Promise<void> {
// Wait for idle on this session
await this.waitForIdleOnSession(this.session, timeout);
return this.waitForIdleOnSession(this.session, timeout);
}

public async connect(cancelToken?: CancellationToken): Promise<void> {
Expand Down Expand Up @@ -157,80 +148,6 @@ export class JupyterSession extends BaseJupyterSession {
}
}

@captureTelemetry(Telemetry.WaitForIdleJupyter, undefined, true)
private async waitForIdleOnSession(session: ISessionWithSocket | undefined, timeout: number): Promise<void> {
if (session && session.kernel) {
traceInfo(`Waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`);
// tslint:disable-next-line: no-any
const statusHandler = (resolve: () => void, reject: (exc: any) => void, e: Kernel.Status | undefined) => {
if (e === 'idle') {
resolve();
} else if (e === 'dead') {
traceError('Kernel died while waiting for idle');
// If we throw an exception, make sure to shutdown the session as it's not usable anymore
this.shutdownSession(session, this.statusHandler).ignoreErrors();
reject(
new JupyterInvalidKernelError({
...session.kernel,
lastActivityTime: new Date(),
numberOfConnections: 0,
session: session.model
})
);
}
};

let statusChangeHandler: Slot<ISessionWithSocket, Kernel.Status> | undefined;
const kernelStatusChangedPromise = new Promise((resolve, reject) => {
statusChangeHandler = (_: ISessionWithSocket, e: Kernel.Status) => statusHandler(resolve, reject, e);
session.statusChanged.connect(statusChangeHandler);
});
let kernelChangedHandler: Slot<ISessionWithSocket, Session.IKernelChangedArgs> | undefined;
const statusChangedPromise = new Promise((resolve, reject) => {
kernelChangedHandler = (_: ISessionWithSocket, e: Session.IKernelChangedArgs) =>
statusHandler(resolve, reject, e.newValue?.status);
session.kernelChanged.connect(kernelChangedHandler);
});
const checkStatusPromise = new Promise(async (resolve) => {
// This function seems to cause CI builds to timeout randomly on
// different tests. Waiting for status to go idle doesn't seem to work and
// in the past, waiting on the ready promise doesn't work either. Check status with a maximum of 5 seconds
const startTime = Date.now();
while (
session &&
session.kernel &&
session.kernel.status !== 'idle' &&
Date.now() - startTime < timeout
) {
await sleep(100);
}
resolve();
});
await Promise.race([kernelStatusChangedPromise, statusChangedPromise, checkStatusPromise]);
traceInfo(`Finished waiting for idle on (kernel): ${session.kernel.id} -> ${session.kernel.status}`);

if (statusChangeHandler && session && session.statusChanged) {
session.statusChanged.disconnect(statusChangeHandler);
}
if (kernelChangedHandler && session && session.kernelChanged) {
session.kernelChanged.disconnect(kernelChangedHandler);
}

// If we didn't make it out in ten seconds, indicate an error
if (session.kernel && session.kernel.status === 'idle') {
// So that we don't have problems with ipywidgets, always register the default ipywidgets comm target.
// Restart sessions and retries might make this hard to do correctly otherwise.
session.kernel.registerCommTarget(Identifiers.DefaultCommTarget, noop);

return;
}

// If we throw an exception, make sure to shutdown the session as it's not usable anymore
this.shutdownSession(session, this.statusHandler).ignoreErrors();
throw new JupyterWaitForIdleError(localize.DataScience.jupyterLaunchTimedOut());
}
}

private async createSession(
serverSettings: ServerConnection.ISettings,
kernelSpec: IJupyterKernelSpec | LiveKernelModel | undefined,
Expand Down
8 changes: 6 additions & 2 deletions src/client/datascience/raw-kernel/rawJupyterSession.ts
View file
Expand Up @@ -49,8 +49,12 @@ export class RawJupyterSession extends BaseJupyterSession {
}

@reportAction(ReportableAction.JupyterSessionWaitForIdleSession)
public async waitForIdle(_timeout: number): Promise<void> {
// RawKernels are good to go right away
public async waitForIdle(timeout: number): Promise<void> {
// Wait until status says idle.
if (this.session) {
return this.waitForIdleOnSession(this.session, timeout);
}
return Promise.resolve();
}
public async dispose(): Promise<void> {
this._disposables.forEach((d) => d.dispose());
Expand Down
8 changes: 5 additions & 3 deletions src/client/datascience/raw-kernel/rawSession.ts
View file
Expand Up @@ -26,13 +26,15 @@ export class RawSession implements ISessionWithSocket {
private _clientID: string;
private _kernel: RawKernel;
private readonly _statusChanged: Signal<this, Kernel.Status>;
private readonly _kernelChanged: Signal<this, Session.IKernelChangedArgs>;
private readonly exitHandler: IDisposable;

// RawSession owns the lifetime of the kernel process and will dispose it
constructor(public kernelProcess: IKernelProcess) {
// tslint:disable-next-line: no-require-imports
const singaling = require('@phosphor/signaling') as typeof import('@phosphor/signaling');
this._statusChanged = new singaling.Signal<this, Kernel.Status>(this);
const signaling = require('@phosphor/signaling') as typeof import('@phosphor/signaling');
this._statusChanged = new signaling.Signal<this, Kernel.Status>(this);
this._kernelChanged = new signaling.Signal<this, Session.IKernelChangedArgs>(this);
// Unique ID for this session instance
this._id = uuid();

Expand Down Expand Up @@ -93,7 +95,7 @@ export class RawSession implements ISessionWithSocket {
throw new Error('Not yet implemented');
}
get kernelChanged(): ISignal<this, Session.IKernelChangedArgs> {
throw new Error('Not yet implemented');
return this._kernelChanged;
}
get propertyChanged(): ISignal<this, 'path' | 'name' | 'type'> {
throw new Error('Not yet implemented');
Expand Down
7 changes: 4 additions & 3 deletions src/test/datascience/uiTests/notebookHelpers.ts
View file
Expand Up @@ -11,7 +11,6 @@ import * as TypeMoq from 'typemoq';
import { EventEmitter, Uri, ViewColumn, WebviewPanel } from 'vscode';
import { traceInfo } from '../../../client/common/logger';
import { noop } from '../../../client/common/utils/misc';
import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes';
import { INotebookEditor, INotebookEditorProvider } from '../../../client/datascience/types';
import { createTemporaryFile } from '../../utils/fs';
import { mockedVSCodeNamespaces } from '../../vscode-mock';
Expand Down Expand Up @@ -118,14 +117,16 @@ export async function openNotebook(
const webViewPanel = createWebViewPanel();
traceInfo(`Notebook UI Tests: about to open editor`);

const kernelIdle = notebookUI.waitForMessageAfterServer(InteractiveWindowMessages.KernelIdle);
const notebookEditor = await openNotebookEditor(ioc, notebookFileContents, notebookFile);
traceInfo(`Notebook UI Tests: have editor`);
await uiLoaded;
traceInfo(`Notebook UI Tests: UI complete`);

// Wait for kernel to be idle before finishing. (Prevents early shutdown problems in tests)
await kernelIdle;
await notebookEditor.notebook?.waitForIdle(60_000);

// Tell the notebook UI about the editor (it needs it for execution)
notebookUI._setEditor(notebookEditor);

return { notebookEditor, webViewPanel, notebookUI };
}
17 changes: 17 additions & 0 deletions src/test/datascience/uiTests/notebookUi.ts
View file
Expand Up @@ -6,6 +6,7 @@
import { assert } from 'chai';
import { ElementHandle } from 'playwright-chromium';
import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes';
import { INotebookEditor } from '../../../client/datascience/types';
import { BaseWebUI } from './helpers';

enum CellToolbarButton {
Expand All @@ -17,6 +18,10 @@ enum MainToolbarButton {
}

export class NotebookEditorUI extends BaseWebUI {
private _editor: INotebookEditor | undefined;
public _setEditor(editor: INotebookEditor) {
this._editor = editor;
}
public async getCellCount(): Promise<number> {
const items = await this.page!.$$('.cell-wrapper');
return items.length;
Expand All @@ -29,6 +34,10 @@ export class NotebookEditorUI extends BaseWebUI {

public async executeCell(cellIndex: number): Promise<void> {
const renderedPromise = this.waitForMessage(InteractiveWindowMessages.ExecutionRendered);
// Make sure to wait for idle so that the button is clickable.
await this.waitForIdle();

// Click the run button.
const runButton = await this.getToolbarButton(cellIndex, CellToolbarButton.run);
await Promise.all([runButton.click({ button: 'left', force: true, timeout: 0 }), renderedPromise]);
}
Expand Down Expand Up @@ -58,6 +67,14 @@ export class NotebookEditorUI extends BaseWebUI {
const items = await this.page!.$$('.cell-wrapper');
return items[cellIndex];
}

private waitForIdle(): Promise<void> {
if (this._editor && this._editor.notebook) {
return this._editor.notebook.waitForIdle(60_000);
}
return Promise.resolve();
}

private async getMainToolbarButton(button: MainToolbarButton): Promise<ElementHandle<Element>> {
// First wait for the toolbar button to be visible.
await this.page!.waitForFunction(
Expand Down

0 comments on commit 3a926e2

Please sign in to comment.