Skip to content

Mock jupyter apis such that they can be faked to have tests run more reliably #3853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Rich Chiodo (rchiodo) merged 5 commits into from
Jan 3, 2019
Merged

Mock jupyter apis such that they can be faked to have tests run more reliably #3853

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f4efac
Implement jupyter session wrapping
rchiodo Jan 2, 2019
e97b145
Fix remote test to not leave a dangling session
rchiodo Jan 3, 2019
3acc3c7
Fix dispose problems in functional tests
rchiodo Jan 3, 2019
c051915
Add news entry
rchiodo Jan 3, 2019
0ae57fc
Fix guid regex
rchiodo Jan 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/3 Code Health/3556.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add mock of Jupyter API to allow functional tests to run more quickly and more consistently.
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/client/datascience/editor-integration/codewatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import * as localize from '../../common/utils/localize';
import { captureTelemetry } from '../../telemetry';
import { generateCellRanges } from '../cellFactory';
import { Commands, Telemetry } from '../constants';
import { JupyterInstallError } from '../jupyterInstallError';
import { JupyterInstallError } from '../jupyter/jupyterInstallError';
import { ICodeWatcher, IHistoryProvider } from '../types';

@injectable()
Expand Down
14 changes: 13 additions & 1 deletion src/client/datascience/history.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,12 @@ import { EXTENSION_ROOT_DIR } from '../common/constants';
import { ContextKey } from '../common/contextKey';
import { IFileSystem } from '../common/platform/types';
import { IConfigurationService, IDisposableRegistry, ILogger } from '../common/types';
import { createDeferred } from '../common/utils/async';
import * as localize from '../common/utils/localize';
import { IInterpreterService } from '../interpreter/contracts';
import { captureTelemetry, sendTelemetryEvent } from '../telemetry';
import { EditorContexts, HistoryMessages, Settings, Telemetry } from './constants';
import { JupyterInstallError } from './jupyterInstallError';
import { JupyterInstallError } from './jupyter/jupyterInstallError';
import {
CellState,
ICell,
Expand Down Expand Up @@ -112,6 +113,14 @@ export class History implements IWebPanelMessageListener, IHistory {
// Start a status item
const status = this.setStatus(localize.DataScience.executingCode());

// Create a deferred object that will wait until the status is disposed
const finishedAddingCode = createDeferred<void>();
const actualDispose = status.dispose;
status.dispose = () => {
finishedAddingCode.resolve();
actualDispose();
};

try {

// Make sure we're loaded first.
Expand Down Expand Up @@ -151,6 +160,9 @@ export class History implements IWebPanelMessageListener, IHistory {
// Indicate executing until this cell is done.
status.dispose();
});

// Wait for the cell to finish
await finishedAddingCode.promise;
}
} catch (err) {
status.dispose();
Expand Down
15 changes: 8 additions & 7 deletions src/client/datascience/historycommandlistener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,14 @@ export class HistoryCommandListener implements IDataScienceCommandListener {

// When all done, show a notice that it completed.
const openQuestion = localize.DataScience.exportOpenQuestion();
this.applicationShell.showInformationMessage(localize.DataScience.exportDialogComplete().format(uri.fsPath), openQuestion).then((str: string | undefined) => {
if (str === openQuestion) {
// If the user wants to, open the notebook they just generated.
this.jupyterExecution.spawnNotebook(uri.fsPath).ignoreErrors();
}
});

if (uri && uri.fsPath) {
this.applicationShell.showInformationMessage(localize.DataScience.exportDialogComplete().format(uri.fsPath), openQuestion).then((str: string | undefined) => {
if (str === openQuestion) {
// If the user wants to, open the notebook they just generated.
this.jupyterExecution.spawnNotebook(uri.fsPath).ignoreErrors();
}
});
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion ...client/datascience/jupyterConnectError.ts → ...atascience/jupyter/jupyterConnectError.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
import '../common/extensions';
import '../../common/extensions';

export class JupyterConnectError extends Error {
constructor(message: string, stderr?: string) {
Expand Down
16 changes: 8 additions & 8 deletions src/client/datascience/jupyterConnection.ts → .../datascience/jupyter/jupyterConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
import * as path from 'path';
import { CancellationToken, Disposable } from 'vscode-jsonrpc';

import { CancellationError } from '../common/cancellation';
import { IFileSystem } from '../common/platform/types';
import { ObservableExecutionResult, Output } from '../common/process/types';
import { IConfigurationService, ILogger } from '../common/types';
import { createDeferred, Deferred } from '../common/utils/async';
import * as localize from '../common/utils/localize';
import { IServiceContainer } from '../ioc/types';
import { CancellationError } from '../../common/cancellation';
import { IFileSystem } from '../../common/platform/types';
import { ObservableExecutionResult, Output } from '../../common/process/types';
import { IConfigurationService, ILogger } from '../../common/types';
import { createDeferred, Deferred } from '../../common/utils/async';
import * as localize from '../../common/utils/localize';
import { IServiceContainer } from '../../ioc/types';
import { JupyterConnectError } from './jupyterConnectError';
import { IConnection } from './types';
import { IConnection } from '../types';

const UrlPatternRegEx = /(https?:\/\/[^\s]+)/ ;
const HttpPattern = /https?:\/\//;
Expand Down
117 changes: 34 additions & 83 deletions src/client/datascience/jupyterExecution.ts → ...t/datascience/jupyter/jupyterExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,33 +10,34 @@ import { URL } from 'url';
import * as uuid from 'uuid/v4';
import { CancellationToken, Disposable } from 'vscode-jsonrpc';

import { Cancellation, CancellationError } from '../common/cancellation';
import { IS_WINDOWS } from '../common/platform/constants';
import { IFileSystem, TemporaryDirectory } from '../common/platform/types';
import { Cancellation, CancellationError } from '../../common/cancellation';
import { IS_WINDOWS } from '../../common/platform/constants';
import { IFileSystem, TemporaryDirectory } from '../../common/platform/types';
import {
ExecutionResult,
IProcessService,
IProcessServiceFactory,
IPythonExecutionFactory,
ObservableExecutionResult,
SpawnOptions
} from '../common/process/types';
import { IAsyncDisposableRegistry, IDisposableRegistry, ILogger } from '../common/types';
import * as localize from '../common/utils/localize';
import { noop } from '../common/utils/misc';
import { EXTENSION_ROOT_DIR } from '../constants';
} from '../../common/process/types';
import { IAsyncDisposableRegistry, IDisposableRegistry, ILogger } from '../../common/types';
import * as localize from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { EXTENSION_ROOT_DIR } from '../../constants';
import {
ICondaService,
IInterpreterService,
IKnownSearchPathsForInterpreters,
InterpreterType,
PythonInterpreter
} from '../interpreter/contracts';
import { IServiceContainer } from '../ioc/types';
import { captureTelemetry } from '../telemetry';
import { Telemetry } from './constants';
} from '../../interpreter/contracts';
import { IServiceContainer } from '../../ioc/types';
import { captureTelemetry } from '../../telemetry';
import { Telemetry } from '../constants';
import { JupyterConnection, JupyterServerInfo } from './jupyterConnection';
import { IConnection, IJupyterExecution, IJupyterKernelSpec, INotebookServer } from './types';
import { IConnection, IJupyterExecution, IJupyterKernelSpec, INotebookServer, IJupyterSessionManager } from '../types';
import { JupyterKernelSpec } from './jupyterKernelSpec';

const CheckJupyterRegEx = IS_WINDOWS ? /^jupyter?\.exe$/ : /^jupyter?$/;
const NotebookCommand = 'notebook';
Expand All @@ -45,33 +46,6 @@ const KernelSpecCommand = 'kernelspec';
const KernelCreateCommand = 'ipykernel';
const PyKernelOutputRegEx = /.*\s+(.+)$/m;
const KernelSpecOutputRegEx = /^\s*(\S+)\s+(\S+)$/;
const IsGuidRegEx = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;

class JupyterKernelSpec implements IJupyterKernelSpec {
public name: string;
public language: string;
public path: string;
public specFile: string | undefined;
constructor(specModel: Kernel.ISpecModel, file?: string) {
this.name = specModel.name;
this.language = specModel.language;
this.path = specModel.argv.length > 0 ? specModel.argv[0] : '';
this.specFile = file;
}
public dispose = async () => {
if (this.specFile &&
IsGuidRegEx.test(path.basename(path.dirname(this.specFile)))) {
// There is more than one location for the spec file directory
// to be cleaned up. If one fails, the other likely deleted it already.
try {
await fs.remove(path.dirname(this.specFile));
} catch {
noop();
}
this.specFile = undefined;
}
}
}

// JupyterCommand objects represent some process that can be launched that should be guaranteed to work because it
// was found by testing it previously
Expand Down Expand Up @@ -155,21 +129,22 @@ export class JupyterExecution implements IJupyterExecution, Disposable {
private usablePythonInterpreter: PythonInterpreter | undefined;

constructor(@inject(IPythonExecutionFactory) private executionFactory: IPythonExecutionFactory,
@inject(ICondaService) private condaService: ICondaService,
@inject(IInterpreterService) private interpreterService: IInterpreterService,
@inject(IProcessServiceFactory) private processServiceFactory: IProcessServiceFactory,
@inject(IKnownSearchPathsForInterpreters) private knownSearchPaths: IKnownSearchPathsForInterpreters,
@inject(ILogger) private logger: ILogger,
@inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry,
@inject(IAsyncDisposableRegistry) private asyncRegistry: IAsyncDisposableRegistry,
@inject(IFileSystem) private fileSystem: IFileSystem,
@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
@inject(ICondaService) private condaService: ICondaService,
@inject(IInterpreterService) private interpreterService: IInterpreterService,
@inject(IProcessServiceFactory) private processServiceFactory: IProcessServiceFactory,
@inject(IKnownSearchPathsForInterpreters) private knownSearchPaths: IKnownSearchPathsForInterpreters,
@inject(ILogger) private logger: ILogger,
@inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry,
@inject(IAsyncDisposableRegistry) private asyncRegistry: IAsyncDisposableRegistry,
@inject(IFileSystem) private fileSystem: IFileSystem,
@inject(IJupyterSessionManager) private sessionManager: IJupyterSessionManager,
@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
this.processServicePromise = this.processServiceFactory.create();
this.disposableRegistry.push(this.interpreterService.onDidChangeInterpreter(this.onSettingsChanged));
this.disposableRegistry.push(this.interpreterService.onDidChangeInterpreter(() => this.onSettingsChanged()));
this.disposableRegistry.push(this);
}

public dispose = () => {
public dispose() {
// Clear our usableJupyterInterpreter
this.usablePythonInterpreter = undefined;
this.commands = {};
Expand Down Expand Up @@ -241,7 +216,6 @@ export class JupyterExecution implements IJupyterExecution, Disposable {

// Try to connect to our jupyter process
const result = this.serviceContainer.get<INotebookServer>(INotebookServer);
this.disposableRegistry.push(result);
await result.connect(connection, kernelSpec, cancelToken, workingDir);
return result;
} catch (err) {
Expand Down Expand Up @@ -298,7 +272,7 @@ export class JupyterExecution implements IJupyterExecution, Disposable {
}

// Now enumerate them again
const enumerator = connection ? () => this.getActiveKernelSpecs(connection) : () => this.enumerateSpecs(cancelToken);
const enumerator = connection ? () => this.sessionManager.getActiveKernelSpecs(connection) : () => this.enumerateSpecs(cancelToken);

// Then find our match
return this.findSpecMatch(enumerator);
Expand Down Expand Up @@ -365,8 +339,10 @@ export class JupyterExecution implements IJupyterExecution, Disposable {
const launchResult = await notebookCommand.execObservable(args, { throwOnStdErr: false, encoding: 'utf8', token: cancelToken });

// Make sure this process gets cleaned up. We might be canceled before the connection finishes.
if (launchResult) {
this.asyncRegistry.push({ dispose: () => Promise.resolve(launchResult.dispose()) });
if (launchResult && cancelToken) {
cancelToken.onCancellationRequested(() => {
launchResult.dispose();
});
}

// Wait for the connection information on this result
Expand Down Expand Up @@ -475,10 +451,10 @@ export class JupyterExecution implements IJupyterExecution, Disposable {
return undefined;
}

private onSettingsChanged = (): Promise<void> => {
private onSettingsChanged() {
// Do the same thing as dispose so that we regenerate
// all of our commands
return Promise.resolve(this.dispose());
this.dispose();

@rchiodo Rich Chiodo (rchiodo) Jan 3, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this [](start = 8, length = 4)

I think this might be incorrect. It should wait for the dispose. #Resolved

@rchiodo Rich Chiodo (rchiodo) Jan 3, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually the dispose() in this class doesn't return a promise, so this is correct.

In reply to: 245071855 [](ancestors = 245071855)

}

private async addMatchingSpec(bestInterpreter: PythonInterpreter, cancelToken?: CancellationToken): Promise<void> {
Expand Down Expand Up @@ -684,32 +660,7 @@ export class JupyterExecution implements IJupyterExecution, Disposable {
return bestSpec;
}

private getActiveKernelSpecs = async (connection: IConnection): Promise<IJupyterKernelSpec[]> => {
// Use our connection to create a session manager
const serverSettings = ServerConnection.makeSettings(
{
baseUrl: connection.baseUrl,
token: connection.token,
pageUrl: '',
// A web socket is required to allow token authentication (what if there is no token authentication?)
wsUrl: connection.baseUrl.replace('http', 'ws'),
init: { cache: 'no-store', credentials: 'same-origin' }
});
const sessionManager = new SessionManager({ serverSettings: serverSettings });

// Ask the session manager to refresh its list of kernel specs.
await sessionManager.refreshSpecs();

// Enumerate all of the kernel specs, turning each into a JupyterKernelSpec
const kernelspecs = sessionManager.specs && sessionManager.specs.kernelspecs ? sessionManager.specs.kernelspecs : {};
const keys = Object.keys(kernelspecs);
return keys.map(k => {
const spec = kernelspecs[k];
return new JupyterKernelSpec(spec);
});
}

private async readSpec(kernelSpecOutputLine: string): Promise<JupyterKernelSpec | undefined> {
private async readSpec(kernelSpecOutputLine: string) : Promise<JupyterKernelSpec | undefined> {
const match = KernelSpecOutputRegEx.exec(kernelSpecOutputLine);
if (match && match !== null && match.length > 2) {
// Second match should be our path to the kernel spec
Expand Down
14 changes: 7 additions & 7 deletions src/client/datascience/jupyterExporter.ts → ...nt/datascience/jupyter/jupyterExporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import * as uuid from 'uuid/v4';

import * as os from 'os';
import * as path from 'path';
import { IWorkspaceService } from '../common/application/types';
import { IFileSystem } from '../common/platform/types';
import { ILogger } from '../common/types';
import * as localize from '../common/utils/localize';
import { noop } from '../common/utils/misc';
import { CodeSnippits, RegExpValues } from './constants';
import { CellState, ICell, IJupyterExecution, INotebookExporter, ISysInfo } from './types';
import { IWorkspaceService } from '../../common/application/types';
import { IFileSystem } from '../../common/platform/types';
import { ILogger } from '../../common/types';
import * as localize from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { CodeSnippits, RegExpValues } from '../constants';
import { CellState, ICell, IJupyterExecution, INotebookExporter, ISysInfo } from '../types';

@injectable()
export class JupyterExporter implements INotebookExporter {
Expand Down
Loading