Skip to content

Fixes to for escaping of output in native notebooks #14159

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 1 commit into from
Sep 29, 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
16 changes: 2 additions & 14 deletions src/client/datascience/jupyter/kernels/cellExecution.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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