Skip to content

More functional tests for the notebook editor #7804

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
merged 4 commits into from
Oct 8, 2019
Merged

More functional tests for the notebook editor #7804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7eaa9ff
New tests
rchiodo Oct 7, 2019
961603a
Merge remote-tracking branch 'origin/master' into rchiodo/startup_tests
rchiodo Oct 7, 2019
f7f0b8a
Add some more functional tests for the notebook editor
rchiodo Oct 7, 2019
27674c9
Review feedback
rchiodo Oct 7, 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/7372.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
More functional tests for the notebook editor.
4 changes: 2 additions & 2 deletions src/client/datascience/editor-integration/codewatcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ export class CodeWatcher implements ICodeWatcher {
}
this.sendPerceivedCellExecute(stopWatch);
} catch (err) {
this.dataScienceErrorHandler.handleError(err).ignoreErrors();
await this.dataScienceErrorHandler.handleError(err);
}

return result;
Expand All @@ -347,7 +347,7 @@ export class CodeWatcher implements ICodeWatcher {
const activeInteractiveWindow = await this.interactiveWindowProvider.getOrCreateActive();
return activeInteractiveWindow.addMessage(message);
} catch (err) {
this.dataScienceErrorHandler.handleError(err).ignoreErrors();
await this.dataScienceErrorHandler.handleError(err);
}
}
}
Expand Down
42 changes: 19 additions & 23 deletions src/client/datascience/errorHandler/errorHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,34 +20,30 @@ export class DataScienceErrorHandler implements IDataScienceErrorHandler {

public async handleError(err: Error): Promise<void> {
if (err instanceof JupyterInstallError) {
this.applicationShell.showInformationMessage(
const response = await this.applicationShell.showInformationMessage(
err.message,
localize.DataScience.jupyterInstall(),
localize.DataScience.notebookCheckForImportNo(),
err.actionTitle)
.then(response => {
if (response === localize.DataScience.jupyterInstall()) {
return this.channels.getInstallationChannels()
.then(installers => {
if (installers) {
// If Conda is available, always pick it as the user must have a Conda Environment
const installer = installers.find(ins => ins.name === 'Conda');
const product = ProductNames.get(Product.jupyter);
err.actionTitle);
if (response === localize.DataScience.jupyterInstall()) {
const installers = await this.channels.getInstallationChannels();
if (installers) {
// If Conda is available, always pick it as the user must have a Conda Environment
const installer = installers.find(ins => ins.name === 'Conda');
const product = ProductNames.get(Product.jupyter);

if (installer && product) {
installer.installModule(product)
.catch(e => this.applicationShell.showErrorMessage(e.message, localize.DataScience.pythonInteractiveHelpLink()));
} else if (installers[0] && product) {
installers[0].installModule(product)
.catch(e => this.applicationShell.showErrorMessage(e.message, localize.DataScience.pythonInteractiveHelpLink()));
}
}
});
} else if (response === err.actionTitle) {
// This is a special error that shows a link to open for more help
this.applicationShell.openUrl(err.action);
if (installer && product) {
installer.installModule(product)
.catch(e => this.applicationShell.showErrorMessage(e.message, localize.DataScience.pythonInteractiveHelpLink()));
} else if (installers[0] && product) {
installers[0].installModule(product)
.catch(e => this.applicationShell.showErrorMessage(e.message, localize.DataScience.pythonInteractiveHelpLink()));
}
});
}
} else if (response === err.actionTitle) {
// This is a special error that shows a link to open for more help
this.applicationShell.openUrl(err.action);
}
} else if (err instanceof JupyterSelfCertsError) {
// Don't show the message for self cert errors
noop();
Expand Down
44 changes: 23 additions & 21 deletions src/client/datascience/interactive-common/interactiveBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
await this.ipynbProvider.open(Uri.file(file), contents);
}
} catch (e) {
this.errorHandler.handleError(e).ignoreErrors();
await this.errorHandler.handleError(e);
}
});
} catch (exc) {
Expand Down Expand Up @@ -721,7 +721,6 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
if (!usable) {
// Not loading anymore
status.dispose();
this.dispose();

// Indicate failing.
throw new JupyterInstallError(localize.DataScience.jupyterNotSupported().format(await this.jupyterExecution.getNotebookError()), localize.DataScience.pythonInteractiveHelpLink());
Expand All @@ -741,7 +740,7 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
sendTelemetryEvent(Telemetry.SelfCertsMessageClose);
}
// Don't leave our Interactive Window open in a non-connected state
this.dispose();
this.closeBecauseOfFailure(e).ignoreErrors();
});
throw e;
} else {
Expand Down Expand Up @@ -1139,27 +1138,30 @@ export abstract class InteractiveBase extends WebViewHost<IInteractiveWindowMapp
private async checkUsable(): Promise<boolean> {
let activeInterpreter: PythonInterpreter | undefined;
try {
activeInterpreter = await this.interpreterService.getActiveInterpreter();
const usableInterpreter = await this.jupyterExecution.getUsableJupyterPython();
if (usableInterpreter) {
// See if the usable interpreter is not our active one. If so, show a warning
// Only do this if not the guest in a liveshare session
const api = await this.liveShare.getApi();
if (!api || (api.session && api.session.role !== vsls.Role.Guest)) {
const active = await this.interpreterService.getActiveInterpreter();
const activeDisplayName = active ? active.displayName : undefined;
const activePath = active ? active.path : undefined;
const usableDisplayName = usableInterpreter ? usableInterpreter.displayName : undefined;
const usablePath = usableInterpreter ? usableInterpreter.path : undefined;
const notebookError = await this.jupyterExecution.getNotebookError();
if (activePath && usablePath && !this.fileSystem.arePathsSame(activePath, usablePath) && activeDisplayName && usableDisplayName) {
this.applicationShell.showWarningMessage(localize.DataScience.jupyterKernelNotSupportedOnActive().format(activeDisplayName, usableDisplayName, notebookError));
const options = await this.getNotebookOptions();
if (options && !options.uri) {
activeInterpreter = await this.interpreterService.getActiveInterpreter();
const usableInterpreter = await this.jupyterExecution.getUsableJupyterPython();
if (usableInterpreter) {
// See if the usable interpreter is not our active one. If so, show a warning
// Only do this if not the guest in a liveshare session
const api = await this.liveShare.getApi();
if (!api || (api.session && api.session.role !== vsls.Role.Guest)) {
const active = await this.interpreterService.getActiveInterpreter();
const activeDisplayName = active ? active.displayName : undefined;
const activePath = active ? active.path : undefined;
const usableDisplayName = usableInterpreter ? usableInterpreter.displayName : undefined;
const usablePath = usableInterpreter ? usableInterpreter.path : undefined;
const notebookError = await this.jupyterExecution.getNotebookError();
if (activePath && usablePath && !this.fileSystem.arePathsSame(activePath, usablePath) && activeDisplayName && usableDisplayName) {
this.applicationShell.showWarningMessage(localize.DataScience.jupyterKernelNotSupportedOnActive().format(activeDisplayName, usableDisplayName, notebookError));
}
}
}
return usableInterpreter ? true : false;
} else {
return true;
}

return usableInterpreter ? true : false;

} catch (e) {
// Can't find a usable interpreter, show the error.
if (activeInterpreter) {
Expand Down
27 changes: 25 additions & 2 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
// If that works, send the cells to the web view
return this.postMessage(InteractiveWindowMessages.LoadAllCells, { cells });
} catch (e) {
this.errorHandler.handleError(e).ignoreErrors();
return this.errorHandler.handleError(e);
}
}

Expand Down Expand Up @@ -280,10 +280,33 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
this.shareMessage(InteractiveWindowMessages.RemoteReexecuteCode, { code: info.code, file: Identifiers.EmptyFileName, line: 0, id: info.id, originator: this.id, debug: false });
}
} catch (exc) {
await this.errorHandler.handleError(exc);
// Make this error our cell output
this.sendCellsToWebView([
Copy link

Choose a reason for hiding this comment

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

Why not send the error message to the view then display the error message (e.g. errorHandler.handleError could end up displaying another UI, and its good to have the errors already visible in the native editor at that point.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea.

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

{
data: {
source: info.code,
cell_type: 'code',
outputs: [{
output_type: 'error',
evalue: exc.toString()
}],
metadata: {},
execution_count: null
},
id: info.id,
file: Identifiers.EmptyFileName,
line: 0,
state: CellState.error,
type: 'execute'
}
]);

// Tell the other side we restarted the kernel. This will stop all executions
this.postMessage(InteractiveWindowMessages.RestartKernel).ignoreErrors();

// Handle an error
await this.errorHandler.handleError(exc);

}
}

Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/interactive-ipynb/nativeEditorCommandListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export class NativeEditorCommandListener implements IDataScienceCommandListener
// Then take the contents and load it.
await this.provider.open(file, contents);
} catch (e) {
this.dataScienceErrorHandler.handleError(e).ignoreErrors();
return this.dataScienceErrorHandler.handleError(e);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/interactive-ipynb/nativeEditorProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
const command = 'workbench.action.closeActiveEditor';
await this.cmdManager.executeCommand(command);
} catch (e) {
this.dataScienceErrorHandler.handleError(e).ignoreErrors();
return this.dataScienceErrorHandler.handleError(e);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/client/datascience/interactive-window/interactiveWindowCommandListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,10 @@ export class InteractiveWindowCommandListener implements IDataScienceCommandList
}
}
} else {
this.dataScienceErrorHandler.handleError(
await this.dataScienceErrorHandler.handleError(
new JupyterInstallError(
localize.DataScience.jupyterNotSupported().format(await this.jupyterExecution.getNotebookError()),
localize.DataScience.pythonInteractiveHelpLink())).ignoreErrors();
localize.DataScience.pythonInteractiveHelpLink()));
}
}

Expand Down
26 changes: 13 additions & 13 deletions src/client/datascience/jupyter/jupyterExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
INotebookServerLaunchInfo,
INotebookServerOptions
} from '../types';
import {IFindCommandResult, JupyterCommandFinder} from './jupyterCommandFinder';
import { IFindCommandResult, JupyterCommandFinder } from './jupyterCommandFinder';
import { JupyterConnection, JupyterServerInfo } from './jupyterConnection';
import { JupyterInstallError } from './jupyterInstallError';
import { JupyterKernelSpec } from './jupyterKernelSpec';
Expand Down Expand Up @@ -94,7 +94,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
}

public async getNotebookError(): Promise<string> {
const notebook = await this.commandFinder.findBestCommand(JupyterCommands.NotebookCommand);
const notebook = await this.findBestCommand(JupyterCommands.NotebookCommand);
return notebook.error ? notebook.error : localize.DataScience.notebookNotFound();
}

Expand Down Expand Up @@ -200,7 +200,7 @@ export class JupyterExecutionBase implements IJupyterExecution {

public async spawnNotebook(file: string): Promise<void> {
// First we find a way to start a notebook server
const notebookCommand = await this.findBestCommandTimed(JupyterCommands.NotebookCommand);
const notebookCommand = await this.findBestCommand(JupyterCommands.NotebookCommand);
this.checkNotebookCommand(notebookCommand);

const args: string[] = [`--NotebookApp.file_to_run=${file}`];
Expand All @@ -211,7 +211,7 @@ export class JupyterExecutionBase implements IJupyterExecution {

public async importNotebook(file: string, template: string | undefined): Promise<string> {
// First we find a way to start a nbconvert
const convert = await this.findBestCommandTimed(JupyterCommands.ConvertCommand);
const convert = await this.findBestCommand(JupyterCommands.ConvertCommand);
if (!convert.command) {
throw new Error(localize.DataScience.jupyterNbConvertNotSupported());
}
Expand Down Expand Up @@ -270,6 +270,10 @@ export class JupyterExecutionBase implements IJupyterExecution {
}
}

protected async findBestCommand(command: JupyterCommands, cancelToken?: CancellationToken): Promise<IFindCommandResult> {
return this.commandFinder.findBestCommand(command, cancelToken);
}

private checkNotebookCommand(notebook: IFindCommandResult) {
if (!notebook.command) {
const errorMessage = notebook.error ? notebook.error : localize.DataScience.notebookNotFound();
Expand Down Expand Up @@ -345,7 +349,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
@captureTelemetry(Telemetry.StartJupyter)
private async startNotebookServer(useDefaultConfig: boolean, cancelToken?: CancellationToken): Promise<{ connection: IConnection; kernelSpec: IJupyterKernelSpec | undefined }> {
// First we find a way to start a notebook server
const notebookCommand = await this.findBestCommandTimed(JupyterCommands.NotebookCommand, cancelToken);
const notebookCommand = await this.findBestCommand(JupyterCommands.NotebookCommand, cancelToken);
this.checkNotebookCommand(notebookCommand);

// Now actually launch it
Expand Down Expand Up @@ -452,7 +456,7 @@ export class JupyterExecutionBase implements IJupyterExecution {

private getUsableJupyterPythonImpl = async (cancelToken?: CancellationToken): Promise<PythonInterpreter | undefined> => {
// This should be the best interpreter for notebooks
const found = await this.findBestCommandTimed(JupyterCommands.NotebookCommand, cancelToken);
const found = await this.findBestCommand(JupyterCommands.NotebookCommand, cancelToken);
if (found && found.command) {
return found.command.interpreter();
}
Expand Down Expand Up @@ -490,7 +494,7 @@ export class JupyterExecutionBase implements IJupyterExecution {

private async addMatchingSpec(bestInterpreter: PythonInterpreter, cancelToken?: CancellationToken): Promise<void> {
const displayName = localize.DataScience.historyTitle();
const ipykernelCommand = await this.findBestCommandTimed(JupyterCommands.KernelCreateCommand, cancelToken);
const ipykernelCommand = await this.findBestCommand(JupyterCommands.KernelCreateCommand, cancelToken);

// If this fails, then we just skip this spec
try {
Expand Down Expand Up @@ -571,7 +575,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
private isCommandSupported = async (command: JupyterCommands, cancelToken?: CancellationToken): Promise<boolean> => {
// See if we can find the command
try {
const result = await this.findBestCommandTimed(command, cancelToken);
const result = await this.findBestCommand(command, cancelToken);
return result.command !== undefined;
} catch (err) {
this.logger.logWarning(err);
Expand Down Expand Up @@ -735,7 +739,7 @@ export class JupyterExecutionBase implements IJupyterExecution {

private enumerateSpecs = async (_cancelToken?: CancellationToken): Promise<(JupyterKernelSpec | undefined)[]> => {
if (await this.isKernelSpecSupported()) {
const kernelSpecCommand = await this.findBestCommandTimed(JupyterCommands.KernelSpecCommand);
const kernelSpecCommand = await this.findBestCommand(JupyterCommands.KernelSpecCommand);

if (kernelSpecCommand.command) {
try {
Expand Down Expand Up @@ -768,8 +772,4 @@ export class JupyterExecutionBase implements IJupyterExecution {

return [];
}

private async findBestCommandTimed(command: JupyterCommands, cancelToken?: CancellationToken): Promise<IFindCommandResult> {
return this.commandFinder.findBestCommand(command, cancelToken);
}
}
6 changes: 6 additions & 0 deletions src/datascience-ui/native-editor/nativeEditorStateController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ export class NativeEditorStateController extends MainStateController {
return super.handleMessage(msg, payload);
}

// This method is used by tests to prepare this react control for loading again.
public reset() {
this.waitingForLoadRender = false;
Copy link

Choose a reason for hiding this comment

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

What does this method do?

Copy link
Author

Choose a reason for hiding this comment

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

Rests the state of loading so that the tests can reload the same control over again. I'll add a comment. reset sounds like a better name

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

this.setState({ busy: true });
}

public canMoveUp = (cellId?: string) => {
const index = this.getState().cellVMs.findIndex(cvm => cvm.cell.id === cellId);
return (index > 0);
Expand Down
Loading