Skip to content

Commit

Permalink
Fixes to for escaping of output in native notebooks (microsoft#14159)
Browse files Browse the repository at this point in the history
  • Loading branch information
Don Jayamanne committed Sep 29, 2020
1 parent 8e0e622 commit 50c3fd3
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 16 deletions.
16 changes: 2 additions & 14 deletions src/client/datascience/jupyter/kernels/cellExecution.ts
Expand Up @@ -38,8 +38,6 @@ import {
import { IKernel } from './types';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');
// tslint:disable-next-line: no-require-imports
import escape = require('lodash/escape');

export class CellExecutionFactory {
constructor(
Expand Down Expand Up @@ -471,11 +469,6 @@ export class CellExecution {
// See this for docs on the messages:
// https://jupyter-client.readthedocs.io/en/latest/messaging.html#messaging-in-jupyter
private async handleExecuteResult(msg: KernelMessage.IExecuteResultMsg, clearState: RefBool) {
// Escape text output
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string);
}

await this.addToCellData(
{
output_type: 'execute_result',
Expand All @@ -500,7 +493,7 @@ export class CellExecution {
// Mark as stream output so the text is formatted because it likely has ansi codes in it.
output_type: 'stream',
// tslint:disable-next-line: no-any
text: escape((o.data as any)['text/plain'].toString()),
text: (o.data as any)['text/plain'].toString(),
metadata: {},
execution_count: reply.execution_count
},
Expand Down Expand Up @@ -544,7 +537,7 @@ export class CellExecution {
);
edit.replaceCellOutput(this.cellIndex, [...exitingCellOutput]); // This is necessary to get VS code to update (for now)
} else {
const originalText = formatStreamText(concatMultilineString(escape(msg.content.text)));
const originalText = formatStreamText(concatMultilineString(msg.content.text));
// Create a new stream entry
const output: nbformat.IStream = {
output_type: 'stream',
Expand All @@ -557,11 +550,6 @@ export class CellExecution {
}

private async handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool) {
// Escape text output
if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) {
msg.content.data['text/plain'] = escape(msg.content.data['text/plain'] as string);
}

const output: nbformat.IDisplayData = {
output_type: 'display_data',
data: msg.content.data,
Expand Down
48 changes: 48 additions & 0 deletions src/test/datascience/notebook/executionService.ds.test.ts
Expand Up @@ -343,4 +343,52 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () {
'Incorrect output'
);
});
test('Verify escaping of output', async () => {
await insertPythonCellAndWait('1');
await insertPythonCellAndWait(dedent`
a="<a href=f>"
a`);
await insertPythonCellAndWait(dedent`
a="<a href=f>"
print(a)`);
await insertPythonCellAndWait('raise Exception("<whatever>")');
const cells = vscodeNotebook.activeNotebookEditor?.document.cells!;

await executeActiveDocument();

// Wait till execution count changes and status is error.
await waitForCondition(
async () => assertHasExecutionCompletedWithErrors(cells[3]),
15_000,
'Cell did not get executed'
);

for (const cell of cells) {
assert.lengthOf(cell.outputs, 1, 'Incorrect output');
}
assert.equal(
cells[0].outputs[0].outputKind,
vscodeNotebookEnums.CellOutputKind.Rich,
'Incorrect output for first cell'
);
assert.equal(
cells[1].outputs[0].outputKind,
vscodeNotebookEnums.CellOutputKind.Rich,
'Incorrect output for first cell'
);
assert.equal(
cells[2].outputs[0].outputKind,
vscodeNotebookEnums.CellOutputKind.Rich,
'Incorrect output for first cell'
);
assertHasTextOutputInVSCode(cells[0], '1');
assertHasTextOutputInVSCode(cells[1], '<a href=f>', 0, false);
assertHasTextOutputInVSCode(cells[2], '<a href=f>', 0, false);
const errorOutput = cells[3].outputs[0] as CellErrorOutput;
assert.equal(errorOutput.outputKind, vscodeNotebookEnums.CellOutputKind.Error, 'Incorrect output');
assert.equal(errorOutput.ename, 'Exception', 'Incorrect ename'); // As status contains ename, we don't want this displayed again.
assert.equal(errorOutput.evalue, '<whatever>', 'Incorrect evalue'); // As status contains ename, we don't want this displayed again.
assert.isNotEmpty(errorOutput.traceback, 'Incorrect traceback');
assert.include(errorOutput.traceback.join(''), '<whatever>');
});
});
4 changes: 2 additions & 2 deletions src/test/datascience/notebook/interrupRestart.ds.test.ts
Expand Up @@ -81,7 +81,7 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
await closeNotebooksAndCleanUpAfterTests(disposables.concat(suiteDisposables));
});

test('Cancelling token will cancel cell executionxxx', async () => {
test('Cancelling token will cancel cell execution', async () => {
await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0);
const cell = vscEditor.document.cells[0];
const appShell = api.serviceContainer.get<IApplicationShell>(IApplicationShell);
Expand Down Expand Up @@ -115,7 +115,7 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
assertVSCCellHasErrors(cell);
}
});
test('Restarting kernel will cancel cell execution & we can re-run a cellxxx', async () => {
test('Restarting kernel will cancel cell execution & we can re-run a cell', async () => {
await insertPythonCellAndWait('import time\nfor i in range(10000):\n print(i)\n time.sleep(0.1)', 0);
const cell = vscEditor.document.cells[0];

Expand Down

0 comments on commit 50c3fd3

Please sign in to comment.