Skip to content

Treat Native notebook tests as VS Code tests #14282

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.

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 7 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 9 additions & 9 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ src/test/common/insidersBuild/insidersExtensionService.unit.test.ts
src/test/pythonFiles/formatting/dummy.ts

src/test/format/extension.dispatch.test.ts
src/test/format/extension.format.ds.test.ts
src/test/format/extension.format.native.vscode.test.ts
src/test/format/extension.onTypeFormat.test.ts
src/test/format/extension.lineFormatter.test.ts
src/test/format/extension.sort.test.ts
Expand Down Expand Up @@ -574,20 +574,20 @@ src/test/datascience/notebook.functional.test.ts
src/test/datascience/mockLanguageClient.ts
src/test/datascience/errorHandler.functional.test.tsx
src/test/datascience/notebook/notebookStorage.unit.test.ts
src/test/datascience/notebook/notebookTrust.ds.test.ts
src/test/datascience/notebook/notebookTrust.native.vscode.test.ts
src/test/datascience/notebook/rendererExtensionDownloader.unit.test.ts
src/test/datascience/notebook/survey.unit.test.ts
src/test/datascience/notebook/interrupRestart.ds.test.ts
src/test/datascience/notebook/contentProvider.ds.test.ts
src/test/datascience/notebook/interrupRestart.native.vscode.test.ts
src/test/datascience/notebook/contentProvider.native.vscode.test.ts
src/test/datascience/notebook/helper.ts
src/test/datascience/notebook/contentProvider.unit.test.ts
src/test/datascience/notebook/edit.ds.test.ts
src/test/datascience/notebook/edit.native.vscode.test.ts
src/test/datascience/notebook/rendererExension.unit.test.ts
src/test/datascience/notebook/saving.ds.test.ts
src/test/datascience/notebook/notebookEditorProvider.ds.test.ts
src/test/datascience/notebook/saving.native.vscode.test.ts
src/test/datascience/notebook/notebookEditorProvider.native.vscode.test.ts
src/test/datascience/notebook/helpers.unit.test.ts
src/test/datascience/notebook/executionService.ds.test.ts
src/test/datascience/notebook/cellOutput.ds.test.ts
src/test/datascience/notebook/executionService.native.vscode.test.ts
src/test/datascience/notebook/cellOutput.native.vscode.test.ts
src/test/datascience/interactiveWindowTestHelpers.tsx
src/test/datascience/export/exportUtil.test.ts
src/test/datascience/export/exportToHTML.test.ts
Expand Down
4 changes: 2 additions & 2 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@
"skipFiles": ["<node_internals>/**"]
},
{
"name": "Tests (DataScience, *.ds.test.ts)",
"name": "Tests (DataScience, *.native.vscode.test.ts)",
"type": "extensionHost",
"request": "launch",
"runtimeExecutable": "${execPath}",
Expand All @@ -169,7 +169,7 @@
"CI_PYTHON_PATH": "<PythonPath>", // Initialize this to invert the grep (exclude tests with value defined in grep).

"VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE": "true",
"TEST_FILES_SUFFIX": "ds.test"
"TEST_FILES_SUFFIX": "native.vscode.test"
},
"stopOnEntry": false,
"sourceMaps": true,
Expand Down
2 changes: 1 addition & 1 deletion build/ci/templates/test_phases.yml
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ steps:
DISPLAY: :10
VSC_PYTHON_CI_TEST_VSC_CHANNEL: 'insiders'
VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE: 'true'
TEST_FILES_SUFFIX: 'ds.test'
TEST_FILES_SUFFIX: 'native.vscode.test'

# Run the single workspace tests for DS in VS Code Stable.
- script: |
Expand Down
4 changes: 4 additions & 0 deletions build/ci/vscode-python-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ stages:
'Debugger':
TestsToRun: 'testDebugger'
NeedsPythonTestReqs: true
'DataScience in VSCode':
TestsToRun: 'testDataScienceInVSCode'
NeedsPythonTestReqs: true
NeedsPythonFunctionalReqs: true
'Smoke':
TestsToRun: 'testSmoke'
NeedsPythonTestReqs: true
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3471,7 +3471,7 @@
"preTestJediLSP": "node ./out/test/languageServers/jedi/lspSetup.js",
"testJediLSP": "node ./out/test/languageServers/jedi/lspSetup.js && cross-env CODE_TESTS_WORKSPACE=src/test VSC_PYTHON_CI_TEST_GREP='Language Server:' node ./out/test/testBootstrap.js ./out/test/standardTest.js && node ./out/test/languageServers/jedi/lspTeardown.js",
"pretestDataScience": "node ./out/test/datascience/dsTestSetup.js",
"testDataScience": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_PYTHON_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=ds.test VSC_PYTHON_FORCE_LOGGING=1 VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE=true node ./out/test/testBootstrap.js ./out/test/standardTest.js",
"testDataScience": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_PYTHON_CI_TEST_VSC_CHANNEL=insiders TEST_FILES_SUFFIX=native.vscode.test VSC_PYTHON_FORCE_LOGGING=1 VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE=true node ./out/test/testBootstrap.js ./out/test/standardTest.js",
"pretestDataScienceInVSCode": "node ./out/test/datascience/dsTestSetup.js",
"testDataScienceInVSCode": "cross-env CODE_TESTS_WORKSPACE=src/test/datascience VSC_PYTHON_CI_TEST_VSC_CHANNEL=stable TEST_FILES_SUFFIX=vscode.test VSC_PYTHON_FORCE_LOGGING=1 VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE=true node ./out/test/testBootstrap.js ./out/test/standardTest.js",
"testMultiWorkspace": "node ./out/test/testBootstrap.js ./out/test/multiRootTest.js",
Expand Down
119 changes: 70 additions & 49 deletions src/client/datascience/jupyter/kernels/cellExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export class CellExecution {
*/
private previousUpdatedToCellHasCompleted = Promise.resolve();
private disposables: IDisposable[] = [];
private cancelHandled = false;

private constructor(
public readonly editor: VSCNotebookEditor,
Expand Down Expand Up @@ -146,6 +147,9 @@ export class CellExecution {
* If execution has commenced, then interrupt (via cancellation token) else dequeue from execution.
*/
public async cancel() {
if (this.cancelHandled) {
return;
}
await this.initPromise;
// We need to notify cancellation only if execution is in progress,
// coz if not, we can safely reset the states.
Expand All @@ -156,7 +160,7 @@ export class CellExecution {
if (!this.started) {
await this.dequeue();
}
this._result.resolve(this.cell.metadata.runState);
await this.completedDurToCancellation();
Copy link
Author

Choose a reason for hiding this comment

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

Due to changes in API, we need to ensure cell status is updated.

this.dispose();
}
private dispose() {
Expand Down Expand Up @@ -213,6 +217,24 @@ export class CellExecution {
this._result.resolve(this.cell.metadata.runState);
}

private async completedDurToCancellation() {
await updateCellExecutionTimes(this.editor, this.cell, {
startTime: this.cell.metadata.runStartTime,
lastRunDuration: this.stopWatch.elapsedTime
});

await this.editor.edit((edit) =>
edit.replaceCellMetadata(this.cell.index, {
...this.cell.metadata,
runState: vscodeNotebookEnums.NotebookCellRunState.Idle,
statusMessage: ''
})
);

this._completed = true;
this._result.resolve(this.cell.metadata.runState);
}

/**
* Notify other parts of extension about the cell execution.
*/
Expand Down Expand Up @@ -270,7 +292,7 @@ export class CellExecution {
}
}

private execute(session: IJupyterSession, loggers: INotebookExecutionLogger[]) {
private async execute(session: IJupyterSession, loggers: INotebookExecutionLogger[]) {
// Generate metadata from our cell (some kernels expect this.)
const metadata = {
...this.cell.metadata,
Expand All @@ -281,59 +303,58 @@ export class CellExecution {
const code = this.cell.document.getText();

// Skip if no code to execute
if (code.trim().length > 0) {
const request = session.requestExecute(
{
code,
silent: false,
stop_on_error: false,
allow_stdin: true,
store_history: true // Silent actually means don't output anything. Store_history is what affects execution_count
},
false,
metadata
);
if (code.trim().length === 0) {
return this.completedSuccessfully().then(noop, noop);
}

// Listen to messages and update our cell execution state appropriately
const request = session.requestExecute(
{
code,
silent: false,
stop_on_error: false,
allow_stdin: true,
store_history: true // Silent actually means don't output anything. Store_history is what affects execution_count
},
false,
metadata
);

// Keep track of our clear state
const clearState = new RefBool(false);
// Listen to messages and update our cell execution state appropriately

// Listen to the reponse messages and update state as we go
if (request) {
// Stop handling the request if the subscriber is canceled.
const cancelDisposable = this.token.onCancellationRequested(() => {
request.onIOPub = noop;
request.onStdin = noop;
request.onReply = noop;
});
// Keep track of our clear state
const clearState = new RefBool(false);

// Listen to messages.
request.onIOPub = this.handleIOPub.bind(this, clearState, loggers);
request.onStdin = this.handleInputRequest.bind(this, session);
request.onReply = this.handleReply.bind(this, clearState);

// When the request finishes we are done
request.done
.then(() => this.completedSuccessfully())
.catch(async (e) => {
// @jupyterlab/services throws a `Canceled` error when the kernel is interrupted.
// Such an error must be ignored.
if (e && e instanceof Error && e.message === 'Canceled') {
await this.completedSuccessfully();
} else {
await this.completedWithErrors(e);
}
})
.finally(() => {
cancelDisposable.dispose();
})
.ignoreErrors();
// Listen to the reponse messages and update state as we go
if (!request) {
return this.completedWithErrors(new Error('Session cannot generate requests')).then(noop, noop);
}

// Stop handling the request if the subscriber is canceled.
const cancelDisposable = this.token.onCancellationRequested(() => {
request.onIOPub = noop;
request.onStdin = noop;
request.onReply = noop;
});

// Listen to messages.
request.onIOPub = this.handleIOPub.bind(this, clearState, loggers);
request.onStdin = this.handleInputRequest.bind(this, session);
request.onReply = this.handleReply.bind(this, clearState);

// When the request finishes we are done
try {
await request.done;
await this.completedSuccessfully();
} catch (ex) {
// @jupyterlab/services throws a `Canceled` error when the kernel is interrupted.
// Such an error must be ignored.
if (ex && ex instanceof Error && ex.message === 'Canceled') {
await this.completedSuccessfully();
} else {
this.completedWithErrors(new Error('Session cannot generate requrests')).then(noop, noop);
await this.completedWithErrors(ex);
}
} else {
this.completedSuccessfully().then(noop, noop);
} finally {
cancelDisposable.dispose();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ suite('DataScience - Notebook Commands', () => {
)
).never();
});
test('Should switch kernel using the provided notebookxxx', async () => {
test('Should switch kernel using the provided notebook', async () => {
const notebook = createNotebookMock();
when((notebook as any).then).thenReturn(undefined);
const uri = Uri.file('test.ipynb');
Expand Down
5 changes: 4 additions & 1 deletion src/test/datascience/dsTestSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ function updateTestsForOldNotebooks() {

if (!IS_CI_SERVER) {
// Noop.
} else if (process.env.VSC_PYTHON_CI_TEST_VSC_CHANNEL === 'insiders') {
} else if (
process.env.VSC_PYTHON_CI_TEST_VSC_CHANNEL === 'insiders' &&
process.env.TEST_FILES_SUFFIX === 'native.vscode.test'
) {
updateTestsForNativeNotebooks();
} else {
updateTestsForOldNotebooks();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { IApplicationShell } from '../../../../client/common/application/types';
import { ProductNames } from '../../../../client/common/installer/productNames';
import { BufferDecoder } from '../../../../client/common/process/decoder';
import { ProcessService } from '../../../../client/common/process/proc';
import { IInstaller, InstallerResponse, Product } from '../../../../client/common/types';
import { IDisposable, IInstaller, InstallerResponse, Product } from '../../../../client/common/types';
import { createDeferred } from '../../../../client/common/utils/async';
import { Common, DataScience } from '../../../../client/common/utils/localize';
import { INotebookEditorProvider } from '../../../../client/datascience/types';
Expand All @@ -22,6 +22,7 @@ import { closeNotebooksAndCleanUpAfterTests } from '../../notebook/helper';

// tslint:disable: no-invalid-this max-func-body-length no-function-expression no-any
suite('DataScience Install IPyKernel (slow) (install)', () => {
const disposables: IDisposable[] = [];
const nbFile = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src/test/datascience/jupyter/kernels/nbWithKernel.ipynb');
const executable = getOSType() === OSType.Windows ? 'Scripts/python.exe' : 'bin/python'; // If running locally on Windows box.
const venvPythonPath = path.join(EXTENSION_ROOT_DIR_FOR_TESTS, 'src/test/datascience/.venvnokernel', executable);
Expand Down Expand Up @@ -55,7 +56,7 @@ suite('DataScience Install IPyKernel (slow) (install)', () => {
});

setup(closeActiveWindows);
teardown(closeNotebooksAndCleanUpAfterTests);
teardown(() => closeNotebooksAndCleanUpAfterTests(disposables));

test('Test Install IPyKernel prompt message', async () => {
// Confirm the message has not changed.
Expand All @@ -72,7 +73,7 @@ suite('DataScience Install IPyKernel (slow) (install)', () => {
const installed = createDeferred();

// Confirm it is installed.
sinon.stub(installer, 'install').callsFake(async function (product: Product) {
const showInformationMessage = sinon.stub(installer, 'install').callsFake(async function (product: Product) {
Copy link
Author

Choose a reason for hiding this comment

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

We need to revert the stubs in each test.

// Call original method
const result: InstallerResponse = await ((installer.install as any).wrappedMethod.apply(
installer,
Expand All @@ -83,6 +84,7 @@ suite('DataScience Install IPyKernel (slow) (install)', () => {
}
return result;
});
disposables.push({ dispose: () => showInformationMessage.restore() });

// Confirm message is displayed & we click 'Install` button.
sinon.stub(appShell, 'showErrorMessage').callsFake(function (message: string) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import { assert } from 'chai';
import * as sinon from 'sinon';
import { commands, NotebookEditor as VSCNotebookEditor } from 'vscode';
import { IApplicationShell, IVSCodeNotebook } from '../../../client/common/application/types';
import { IConfigurationService, IDataScienceSettings, IDisposable } from '../../../client/common/types';
import { IDisposable } from '../../../client/common/types';
import { createDeferredFromPromise } from '../../../client/common/utils/async';
import { DataScience } from '../../../client/common/utils/localize';
import { noop } from '../../../client/common/utils/misc';
import { IKernelProvider } from '../../../client/datascience/jupyter/kernels/types';
import { INotebookEditorProvider } from '../../../client/datascience/types';
Expand All @@ -27,9 +28,8 @@ import {
trustAllNotebooks,
waitForTextOutputInVSCode
} from './helper';
// tslint:disable-next-line: no-var-requires no-require-imports

// tslint:disable: no-any no-invalid-this
// tslint:disable: no-any no-invalid-this no-function-expression
/*
* This test focuses on interrupting, restarting kernels.
* We will not use actual kernels, just ensure the appropriate methods are invoked on the appropriate classes.
Expand All @@ -45,8 +45,6 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
let vscEditor: VSCNotebookEditor;
let vscodeNotebook: IVSCodeNotebook;
const suiteDisposables: IDisposable[] = [];
let oldAskForRestart: boolean | undefined;
let dsSettings: IDataScienceSettings;
suiteSetup(async function () {
this.timeout(60_000);
api = await initialize();
Expand All @@ -59,11 +57,6 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
editorProvider = api.serviceContainer.get<INotebookEditorProvider>(INotebookEditorProvider);
editorProvider = api.serviceContainer.get<INotebookEditorProvider>(INotebookEditorProvider);
kernelProvider = api.serviceContainer.get<IKernelProvider>(IKernelProvider);
dsSettings = api.serviceContainer.get<IConfigurationService>(IConfigurationService).getSettings(undefined)
.datascience;
oldAskForRestart = dsSettings.askForKernelRestart;
// Disable the prompt (when attempting to restart kernel).
dsSettings.askForKernelRestart = false;
});
setup(async () => {
sinon.restore();
Expand All @@ -74,12 +67,7 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
vscEditor = vscodeNotebook.activeNotebookEditor!;
});
teardown(() => closeNotebooks(disposables));
suiteTeardown(async () => {
oldAskForRestart = dsSettings.askForKernelRestart;
// Restore.
dsSettings.askForKernelRestart = oldAskForRestart;
await closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables));
});
suiteTeardown(async () => closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables)));

test('Cancelling token will cancel cell execution', async () => {
await insertPythonCell('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0);
Expand Down Expand Up @@ -118,7 +106,20 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
test('Restarting kernel will cancel cell execution & we can re-run a cell', async () => {
await insertPythonCell('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0);
const cell = vscEditor.document.cells[0];
// Ensure we click `Yes` when prompted to restart the kernel.
const appShell = api.serviceContainer.get<IApplicationShell>(IApplicationShell);
const showInformationMessage = sinon
.stub(appShell, 'showInformationMessage')
.callsFake(function (message: string) {
Copy link
Author

Choose a reason for hiding this comment

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

Better & easier test, than updating the settings

Copy link
Author

Choose a reason for hiding this comment

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

Also can be used to confirm prompt is displayed.

if (message === DataScience.restartKernelMessage()) {
// User clicked ok to restart it.
return DataScience.restartKernelMessageYes();
}
return (appShell.showInformationMessage as any).wrappedMethod.apply(appShell, arguments);
});
disposables.push({ dispose: () => showInformationMessage.restore() });

(editorProvider.activeEditor as any).shouldAskForRestart = () => Promise.resolve(false);
await executeActiveDocument();

// Wait for cell to get busy.
Expand Down
Loading