From 9bc647a2d83b2a0ab847aa95d33b405b8762ffb6 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 14:52:32 -0700 Subject: [PATCH 1/9] Move escaping to just output --- .../editor-integration/cellhashprovider.ts | 14 ++++++--- .../datascience/jupyter/jupyterDebugger.ts | 6 +--- .../datascience/jupyter/jupyterNotebook.ts | 31 ++++++++----------- .../datascience/jupyter/kernelVariables.ts | 6 ++-- .../jupyter/oldJupyterVariables.ts | 6 ++-- .../interactive-common/cellOutput.tsx | 20 +++++++++--- 6 files changed, 43 insertions(+), 40 deletions(-) diff --git a/src/client/datascience/editor-integration/cellhashprovider.ts b/src/client/datascience/editor-integration/cellhashprovider.ts index 7f9f14a9de8e..afc123713c84 100644 --- a/src/client/datascience/editor-integration/cellhashprovider.ts +++ b/src/client/datascience/editor-integration/cellhashprovider.ts @@ -29,7 +29,9 @@ import { // tslint:disable-next-line:no-require-imports no-var-requires const _escapeRegExp = require('lodash/escapeRegExp') as typeof import('lodash/escapeRegExp'); // NOSONAR -const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)/g; +// tslint:disable-next-line: no-require-imports no-var-requires +const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR +const LineNumberMatchRegex = /(;32m[ ->]*?)(\d+)(.*)/g; interface IRangedCellHash extends ICellHash { code: string; @@ -133,7 +135,7 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo ...msg, content: { ...msg.content, - traceback: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR + transient: this.modifyTraceback(msg as KernelMessage.IErrorMsg) // NOSONAR } }; } @@ -423,14 +425,16 @@ export class CellHashProvider implements ICellHashProvider, INotebookExecutionLo // Now attempt to find a cell that matches these source lines const offset = this.findCellOffset(this.hashes.get(match[0]), sourceLines); if (offset !== undefined) { - return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num) => { + return traceFrame.replace(LineNumberMatchRegex, (_s, prefix, num, suffix) => { const n = parseInt(num, 10); const newLine = offset + n - 1; - return `${prefix}${newLine + 1}`; + return `${_escape(prefix)}${newLine + 1}${_escape( + suffix + )}`; }); } } - return traceFrame; + return _escape(traceFrame); } } diff --git a/src/client/datascience/jupyter/jupyterDebugger.ts b/src/client/datascience/jupyter/jupyterDebugger.ts index 796dd18ab771..afa7df942137 100644 --- a/src/client/datascience/jupyter/jupyterDebugger.ts +++ b/src/client/datascience/jupyter/jupyterDebugger.ts @@ -3,8 +3,6 @@ 'use strict'; import type { nbformat } from '@jupyterlab/coreutils'; import { inject, injectable, named } from 'inversify'; -// tslint:disable-next-line: no-require-imports -import unescape = require('lodash/unescape'); import * as path from 'path'; import * as uuid from 'uuid/v4'; import { DebugConfiguration, Disposable } from 'vscode'; @@ -475,10 +473,8 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener { if (outputs.length > 0) { const data = outputs[0].data; if (data && data.hasOwnProperty('text/plain')) { - // Plain text should be escaped by our execution engine. Unescape it so - // we can parse it. // tslint:disable-next-line:no-any - return unescape((data as any)['text/plain']); + return (data as any)['text/plain']; } if (outputs[0].output_type === 'stream') { const stream = outputs[0] as nbformat.IStream; diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index ab1a015065e3..242e3c687952 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -40,10 +40,6 @@ import { KernelConnectionMetadata } from './kernels/types'; // tslint:disable-next-line: no-require-imports import cloneDeep = require('lodash/cloneDeep'); -// tslint:disable-next-line: no-require-imports -import escape = require('lodash/escape'); -// tslint:disable-next-line: no-require-imports -import unescape = require('lodash/unescape'); import { concatMultilineString, formatStreamText } from '../../../datascience-ui/common'; import { RefBool } from '../../common/refBool'; import { PythonEnvironment } from '../../pythonEnvironments/info'; @@ -787,12 +783,12 @@ export class JupyterNotebookBase implements INotebook { outputs.forEach((o) => { if (o.output_type === 'stream') { const stream = o as nbformat.IStream; - result = result.concat(formatStreamText(unescape(concatMultilineString(stream.text, true)))); + result = result.concat(formatStreamText(concatMultilineString(stream.text, true))); } else { const data = o.data; if (data && data.hasOwnProperty('text/plain')) { // tslint:disable-next-line:no-any - result = result.concat(unescape((data as any)['text/plain'])); + result = result.concat((data as any)['text/plain']); } } }); @@ -1237,7 +1233,7 @@ export class JupyterNotebookBase implements INotebook { ) { // Check our length on text output if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { - msg.content.data['text/plain'] = escape(trimFunc(msg.content.data['text/plain'] as string)); + msg.content.data['text/plain'] = trimFunc(msg.content.data['text/plain'] as string); } this.addToCellData( @@ -1266,7 +1262,7 @@ export class JupyterNotebookBase implements INotebook { if (o.data && o.data.hasOwnProperty('text/plain')) { // tslint:disable-next-line: no-any const str = (o.data as any)['text/plain'].toString(); - const data = escape(trimFunc(str)) as string; + const data = trimFunc(str) as string; this.addToCellData( cell, { @@ -1314,13 +1310,13 @@ export class JupyterNotebookBase implements INotebook { : undefined; if (existing) { // tslint:disable-next-line:restrict-plus-operands - existing.text = existing.text + escape(msg.content.text); + existing.text = existing.text + msg.content.text; const originalText = formatStreamText(concatMultilineString(existing.text)); originalTextLength = originalText.length; existing.text = trimFunc(originalText); trimmedTextLength = existing.text.length; } else { - const originalText = formatStreamText(concatMultilineString(escape(msg.content.text))); + const originalText = formatStreamText(concatMultilineString(msg.content.text)); originalTextLength = originalText.length; // Create a new stream entry const output: nbformat.IStream = { @@ -1350,11 +1346,6 @@ export class JupyterNotebookBase implements INotebook { } private handleDisplayData(msg: KernelMessage.IDisplayDataMsg, clearState: RefBool, cell: ICell) { - // 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, @@ -1402,10 +1393,14 @@ export class JupyterNotebookBase implements INotebook { private handleError(msg: KernelMessage.IErrorMsg, clearState: RefBool, cell: ICell) { const output: nbformat.IError = { output_type: 'error', - ename: escape(msg.content.ename), - evalue: escape(msg.content.evalue), - traceback: msg.content.traceback.map(escape) + ename: msg.content.ename, + evalue: msg.content.evalue, + traceback: msg.content.traceback }; + if (msg.content.hasOwnProperty('transient')) { + // tslint:disable-next-line: no-any + output.transient = (msg.content as any).transient; + } this.addToCellData(cell, output, clearState); cell.state = CellState.error; diff --git a/src/client/datascience/jupyter/kernelVariables.ts b/src/client/datascience/jupyter/kernelVariables.ts index 501b3f25ef17..7d3d1b004bd3 100644 --- a/src/client/datascience/jupyter/kernelVariables.ts +++ b/src/client/datascience/jupyter/kernelVariables.ts @@ -6,8 +6,6 @@ import { inject, injectable } from 'inversify'; import stripAnsi from 'strip-ansi'; import * as uuid from 'uuid/v4'; -// tslint:disable-next-line: no-require-imports -import unescape = require('lodash/unescape'); import { CancellationToken, Event, EventEmitter, Uri } from 'vscode'; import { PYTHON_LANGUAGE } from '../../common/constants'; import { traceError } from '../../common/logger'; @@ -248,7 +246,7 @@ export class KernelVariables implements IJupyterVariables { // Pull our text result out of the Jupyter cell private deserializeJupyterResult(cells: ICell[]): T { - const text = unescape(this.extractJupyterResultText(cells)); + const text = this.extractJupyterResultText(cells); return JSON.parse(text) as T; } @@ -373,7 +371,7 @@ export class KernelVariables implements IJupyterVariables { // Now execute the query if (notebook && query) { const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), token, true); - const text = unescape(this.extractJupyterResultText(cells)); + const text = this.extractJupyterResultText(cells); // Apply the expression to it const matches = this.getAllMatches(query.parser, text); diff --git a/src/client/datascience/jupyter/oldJupyterVariables.ts b/src/client/datascience/jupyter/oldJupyterVariables.ts index 6c25a3a3e547..7c1d77de7135 100644 --- a/src/client/datascience/jupyter/oldJupyterVariables.ts +++ b/src/client/datascience/jupyter/oldJupyterVariables.ts @@ -11,8 +11,6 @@ import { Event, EventEmitter, Uri } from 'vscode'; import { PYTHON_LANGUAGE } from '../../common/constants'; import { traceError } from '../../common/logger'; -// tslint:disable-next-line: no-require-imports -import unescape = require('lodash/unescape'); import { IConfigurationService } from '../../common/types'; import * as localize from '../../common/utils/localize'; import { EXTENSION_ROOT_DIR } from '../../constants'; @@ -234,7 +232,7 @@ export class OldJupyterVariables implements IJupyterVariables { // Pull our text result out of the Jupyter cell private deserializeJupyterResult(cells: ICell[]): T { - const text = unescape(this.extractJupyterResultText(cells)); + const text = this.extractJupyterResultText(cells); return JSON.parse(text) as T; } @@ -359,7 +357,7 @@ export class OldJupyterVariables implements IJupyterVariables { // Now execute the query if (notebook && query) { const cells = await notebook.execute(query.query, Identifiers.EmptyFileName, 0, uuid(), undefined, true); - const text = unescape(this.extractJupyterResultText(cells)); + const text = this.extractJupyterResultText(cells); // Apply the expression to it const matches = this.getAllMatches(query.parser, text); diff --git a/src/datascience-ui/interactive-common/cellOutput.tsx b/src/datascience-ui/interactive-common/cellOutput.tsx index 8b545eac7086..109e828eac96 100644 --- a/src/datascience-ui/interactive-common/cellOutput.tsx +++ b/src/datascience-ui/interactive-common/cellOutput.tsx @@ -20,6 +20,8 @@ import { getRichestMimetype, getTransform, isIPyWidgetOutput, isMimeTypeSupporte // tslint:disable-next-line: no-var-requires no-require-imports const ansiToHtml = require('ansi-to-html'); +// tslint:disable-next-line: no-var-requires no-require-imports +const lodashEscape = require('lodash/escape'); // tslint:disable-next-line: no-require-imports no-var-requires const cloneDeep = require('lodash/cloneDeep'); @@ -328,7 +330,7 @@ export class CellOutput extends React.Component { // tslint:disable-next-line: no-any const text = (input as any)['text/plain']; input = { - 'text/html': text // XML tags should have already been escaped. + 'text/html': lodashEscape(text) }; } else if (output.output_type === 'stream') { mimeType = 'text/html'; @@ -337,7 +339,7 @@ export class CellOutput extends React.Component { renderWithScrollbars = true; // Sonar is wrong, TS won't compile without this AS const stream = output as nbformat.IStream; // NOSONAR - const concatted = concatMultilineString(stream.text); + const concatted = lodashEscape(concatMultilineString(stream.text)); input = { 'text/html': concatted // XML tags should have already been escaped. }; @@ -363,14 +365,18 @@ export class CellOutput extends React.Component { const error = output as nbformat.IError; // NOSONAR try { const converter = new CellOutput.ansiToHtmlClass(CellOutput.getAnsiToHtmlOptions()); - const trace = error.traceback.length ? converter.toHtml(error.traceback.join('\n')) : error.evalue; + // Modified traceback may exist. If so use that instead. It's only at run time though + const traceback: string[] = error.transient + ? (error.transient as string[]) + : error.traceback.map(lodashEscape); + const trace = traceback ? converter.toHtml(traceback.join('\n')) : error.evalue; input = { 'text/html': trace }; } catch { // This can fail during unit tests, just use the raw data input = { - 'text/html': error.evalue + 'text/html': lodashEscape(error.evalue) }; } } else if (input) { @@ -395,6 +401,12 @@ export class CellOutput extends React.Component { data = fixMarkdown(concatMultilineString(data as nbformat.MultilineString, true), true); } + // Make sure text output is escaped (nteract texttransform won't) + if (mimeType === 'text/plain') { + data = lodashEscape(data.toString()); + mimeType = 'text/html'; + } + return { isText, isError, From 8ca1b523f68347ce28280cc9d218366a5f72f9f3 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 15:28:10 -0700 Subject: [PATCH 2/9] Add some tests to verify round tripping --- .../interactiveWindow.functional.test.tsx | 8 ++ .../nativeEditor.functional.test.tsx | 128 ++++++++++++++++++ .../datascience/notebook.functional.test.ts | 6 +- src/test/datascience/testHelpers.tsx | 3 +- 4 files changed, 139 insertions(+), 6 deletions(-) diff --git a/src/test/datascience/interactiveWindow.functional.test.tsx b/src/test/datascience/interactiveWindow.functional.test.tsx index d40cb82906ac..243a970b1aba 100644 --- a/src/test/datascience/interactiveWindow.functional.test.tsx +++ b/src/test/datascience/interactiveWindow.functional.test.tsx @@ -63,6 +63,8 @@ import { verifyLastCellInputState } from './testHelpers'; import { ITestInteractiveWindowProvider } from './testInteractiveWindowProvider'; +// tslint:disable-next-line: no-require-imports no-var-requires +const _escape = require('lodash/escape') as typeof import('lodash/escape'); // NOSONAR // tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string suite('DataScience Interactive Window output tests', () => { @@ -385,6 +387,9 @@ for _ in range(50): time.sleep(0.1) sys.stdout.write('\\r')`; + const exception = 'raise Exception("")'; + addMockData(ioc, exception, `""`, 'text/html', 'error'); + addMockData(ioc, badPanda, `pandas has no attribute 'read'`, 'text/html', 'error'); addMockData(ioc, goodPanda, `A table`, 'text/html'); addMockData(ioc, matPlotLib, matPlotLibResults, 'text/html'); @@ -401,6 +406,9 @@ for _ in range(50): return Promise.resolve({ result: result, haveMore: loops > 0 }); }); + await addCode(ioc, exception, true); + verifyHtmlOnInteractiveCell(_escape(``), CellPosition.Last); + await addCode(ioc, badPanda, true); verifyHtmlOnInteractiveCell(`has no attribute 'read'`, CellPosition.Last); diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 82e4f21ad91a..75fd36a8b500 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -34,6 +34,7 @@ import { KeyPrefix } from '../../client/datascience/notebookStorage/nativeEditor import { ICell, IDataScienceErrorHandler, + IDataScienceFileSystem, IJupyterExecution, INotebookEditor, INotebookEditorProvider, @@ -880,6 +881,133 @@ df.head()`; verifyHtmlOnCell(ne.mount.wrapper, 'NativeCell', `3`, 2); }); + runMountedTest('Roundtrip with jupyter', async () => { + // Write out a temporary file + const baseFile = ` +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 1, + "metadata": { + "collapsed": true + }, + "outputs": [ + { + "data": { + "text/plain": [ + "1" + ] + }, + "execution_count": 1, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "a='<1>'\\n", + "a" + ] + }, + { + "cell_type": "code", + "execution_count": 2, + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "2" + ] + }, + "execution_count": 2, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "b=2\\n", + "b" + ] + }, + { + "cell_type": "code", + "execution_count": 3, + "metadata": {}, + "outputs": [ + { + "data": { + "text/plain": [ + "3" + ] + }, + "execution_count": 3, + "metadata": {}, + "output_type": "execute_result" + } + ], + "source": [ + "c=3\\n", + "c" + ] + } + ], + "metadata": { + "file_extension": ".py", + "kernelspec": { + "display_name": "Python 3", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.7.4" + }, + "mimetype": "text/x-python", + "name": "python", + "npconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": 3 + }, + "nbformat": 4, + "nbformat_minor": 2 +}`; + addMockData(ioc, `a='<1>'\na`, `'<1>'`); + const dsfs = ioc.get(IDataScienceFileSystem); + const tf = await dsfs.createTemporaryLocalFile('.ipynb'); + try { + await dsfs.writeLocalFile(tf.filePath, baseFile); + + // File should exist. Open and run all cells + const n = await openEditor(ioc, '', tf.filePath); + const threeCellsUpdated = n.mount.waitForMessage(InteractiveWindowMessages.ExecutionRendered, { + numberOfTimes: 3 + }); + n.editor.runAllCells(); + await threeCellsUpdated; + + // Save the file + const saveButton = findButton(n.mount.wrapper, NativeEditor, 8); + const saved = waitForMessage(ioc, InteractiveWindowMessages.NotebookClean); + saveButton!.simulate('click'); + await saved; + + // Read in the file contents. Should match the original + const savedContents = await dsfs.readLocalFile(tf.filePath); + assert.equal(savedContents, baseFile, 'File contents were changed by execution'); + } finally { + tf.dispose(); + } + }); + runMountedTest('Startup and shutdown', async () => { // Turn off raw kernel for this test as it's testing jupyterserver start / shutdown ioc.setExperimentState(LocalZMQKernel.experiment, false); diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index b8ff1d720cbd..5a00e31c716c 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -6,8 +6,6 @@ import { assert } from 'chai'; import { ChildProcess } from 'child_process'; import * as fs from 'fs-extra'; import { injectable } from 'inversify'; -// tslint:disable-next-line: no-require-imports -import escape = require('lodash/escape'); import * as os from 'os'; import * as path from 'path'; import { SemVer } from 'semver'; @@ -1031,7 +1029,7 @@ a`, mimeType: 'text/plain', cellType: 'code', result: ``, - verifyValue: (d) => assert.ok(d.includes(escape(``)), 'XML not escaped') + verifyValue: (d) => assert.ok(d.includes(``), 'Should not escape at the notebook level') }, { markdownRegEx: undefined, @@ -1043,7 +1041,7 @@ df.head()`, cellType: 'error', // tslint:disable-next-line:quotemark verifyValue: (d) => - assert.ok((d as string).includes(escape("has no attribute 'read'")), 'Unexpected error result') + assert.ok((d as string).includes("has no attribute 'read'"), 'Unexpected error result') }, { markdownRegEx: undefined, diff --git a/src/test/datascience/testHelpers.tsx b/src/test/datascience/testHelpers.tsx index 88798f4334f9..fbd453e00ad4 100644 --- a/src/test/datascience/testHelpers.tsx +++ b/src/test/datascience/testHelpers.tsx @@ -7,7 +7,6 @@ import { min } from 'lodash'; import * as path from 'path'; import * as React from 'react'; import { Provider } from 'react-redux'; -import { isString } from 'util'; import { CancellationToken } from 'vscode'; import { EXTENSION_ROOT_DIR } from '../../client/common/constants'; @@ -273,7 +272,7 @@ export function verifyHtmlOnCell( output = targetCell!.find('div.markdown-cell-output'); } const outputHtml = output.length > 0 ? output.html() : undefined; - if (html && isString(html)) { + if (html && typeof html === 'string') { // Extract only the first 100 chars from the input string const sliced = html.substr(0, min([html.length, 100])); assert.ok(output.length > 0, 'No output cell found'); From 3b56ba20a630e0f007629768474f0441131cb2b2 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 16:18:52 -0700 Subject: [PATCH 3/9] Fixup test for rountrip and make roundtripping actually work --- .../datascience/jupyter/jupyterNotebook.ts | 31 +++++++++---------- .../nativeEditor.functional.test.tsx | 17 ++++++++-- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index 242e3c687952..4993780db0f9 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -40,7 +40,7 @@ import { KernelConnectionMetadata } from './kernels/types'; // tslint:disable-next-line: no-require-imports import cloneDeep = require('lodash/cloneDeep'); -import { concatMultilineString, formatStreamText } from '../../../datascience-ui/common'; +import { concatMultilineString, formatStreamText, splitMultilineString } from '../../../datascience-ui/common'; import { RefBool } from '../../common/refBool'; import { PythonEnvironment } from '../../pythonEnvironments/info'; import { getInterpreterFromKernelConnectionMetadata, isPythonKernelConnection } from './kernels/helpers'; @@ -1202,12 +1202,7 @@ export class JupyterNotebookBase implements INotebook { private addToCellData = ( cell: ICell, - output: - | nbformat.IUnrecognizedOutput - | nbformat.IExecuteResult - | nbformat.IDisplayData - | nbformat.IStream - | nbformat.IError, + output: nbformat.IExecuteResult | nbformat.IDisplayData | nbformat.IStream | nbformat.IError, clearState: RefBool ) => { const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell; @@ -1233,7 +1228,10 @@ export class JupyterNotebookBase implements INotebook { ) { // Check our length on text output if (msg.content.data && msg.content.data.hasOwnProperty('text/plain')) { - msg.content.data['text/plain'] = trimFunc(msg.content.data['text/plain'] as string); + msg.content.data['text/plain'] = splitMultilineString( + // tslint:disable-next-line: no-any + trimFunc(concatMultilineString(msg.content.data['text/plain'] as any)) + ); } this.addToCellData( @@ -1261,14 +1259,15 @@ export class JupyterNotebookBase implements INotebook { reply.payload.forEach((o) => { if (o.data && o.data.hasOwnProperty('text/plain')) { // tslint:disable-next-line: no-any - const str = (o.data as any)['text/plain'].toString(); - const data = trimFunc(str) as string; + const str = concatMultilineString((o.data as any)['text/plain']); + const data = trimFunc(str); this.addToCellData( cell, { // Mark as stream output so the text is formatted because it likely has ansi codes in it. output_type: 'stream', - text: data, + text: splitMultilineString(data), + name: 'stdout', metadata: {}, execution_count: reply.execution_count }, @@ -1310,11 +1309,11 @@ export class JupyterNotebookBase implements INotebook { : undefined; if (existing) { // tslint:disable-next-line:restrict-plus-operands - existing.text = existing.text + msg.content.text; - const originalText = formatStreamText(concatMultilineString(existing.text)); + const originalText = formatStreamText(concatMultilineString(existing.text + msg.content.text)); originalTextLength = originalText.length; - existing.text = trimFunc(originalText); - trimmedTextLength = existing.text.length; + const newText = trimFunc(originalText); + trimmedTextLength = newText.length; + existing.text = splitMultilineString(newText); } else { const originalText = formatStreamText(concatMultilineString(msg.content.text)); originalTextLength = originalText.length; @@ -1322,7 +1321,7 @@ export class JupyterNotebookBase implements INotebook { const output: nbformat.IStream = { output_type: 'stream', name: msg.content.name, - text: trimFunc(originalText) + text: [trimFunc(originalText)] }; data.outputs = [...data.outputs, output]; trimmedTextLength = output.text.length; diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 75fd36a8b500..1114417ddad2 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -896,7 +896,7 @@ df.head()`; { "data": { "text/plain": [ - "1" + "'<1>'" ] }, "execution_count": 1, @@ -981,6 +981,8 @@ df.head()`; "nbformat_minor": 2 }`; addMockData(ioc, `a='<1>'\na`, `'<1>'`); + addMockData(ioc, 'b=2\nb', 2); + addMockData(ioc, 'c=3\nc', 3); const dsfs = ioc.get(IDataScienceFileSystem); const tf = await dsfs.createTemporaryLocalFile('.ipynb'); try { @@ -1002,7 +1004,18 @@ df.head()`; // Read in the file contents. Should match the original const savedContents = await dsfs.readLocalFile(tf.filePath); - assert.equal(savedContents, baseFile, 'File contents were changed by execution'); + const savedJSON = JSON.parse(savedContents); + const baseJSON = JSON.parse(baseFile); + + // Don't compare kernelspec names + delete savedJSON.metadata.kernelspec.display_name; + delete baseJSON.metadata.kernelspec.display_name; + + // Don't compare python versions + delete savedJSON.metadata.language_info.version; + delete baseJSON.metadata.language_info.version; + + assert.deepEqual(savedJSON, baseJSON, 'File contents were changed by execution'); } finally { tf.dispose(); } From d73c75982449d4f7ec6e933b779d2692ff5d9556 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 16:21:29 -0700 Subject: [PATCH 4/9] Add news entry --- news/2 Fixes/14182.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/14182.md diff --git a/news/2 Fixes/14182.md b/news/2 Fixes/14182.md new file mode 100644 index 000000000000..53103b73ee62 --- /dev/null +++ b/news/2 Fixes/14182.md @@ -0,0 +1 @@ +Do not escape output in the actual ipynb file. \ No newline at end of file From 60fc1a3d51fce4573072239af607e34d4873e94e Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 16:28:28 -0700 Subject: [PATCH 5/9] Add to manual test file --- src/test/datascience/manualTestFiles/manualTestFile.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/datascience/manualTestFiles/manualTestFile.py b/src/test/datascience/manualTestFiles/manualTestFile.py index b6009d6f04d0..6655a33ae856 100644 --- a/src/test/datascience/manualTestFiles/manualTestFile.py +++ b/src/test/datascience/manualTestFiles/manualTestFile.py @@ -10,6 +10,9 @@ plt.plot(x, np.sin(x)) plt.show() +#%% Test exception +raise Exception("") + # %% Bokeh Plot from bokeh.io import output_notebook, show from bokeh.plotting import figure From d95937834231cc006ce45c93e0c6b3fa7d5e6d03 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 16:54:59 -0700 Subject: [PATCH 6/9] Fix streaming problem and add more to the test --- .../datascience/jupyter/jupyterNotebook.ts | 20 +++++++------- .../nativeEditor.functional.test.tsx | 27 +++++++++++-------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/client/datascience/jupyter/jupyterNotebook.ts b/src/client/datascience/jupyter/jupyterNotebook.ts index 4993780db0f9..d931e3a7b015 100644 --- a/src/client/datascience/jupyter/jupyterNotebook.ts +++ b/src/client/datascience/jupyter/jupyterNotebook.ts @@ -1259,7 +1259,7 @@ export class JupyterNotebookBase implements INotebook { reply.payload.forEach((o) => { if (o.data && o.data.hasOwnProperty('text/plain')) { // tslint:disable-next-line: no-any - const str = concatMultilineString((o.data as any)['text/plain']); + const str = concatMultilineString((o.data as any)['text/plain']); // NOSONAR const data = trimFunc(str); this.addToCellData( cell, @@ -1308,8 +1308,10 @@ export class JupyterNotebookBase implements INotebook { ? data.outputs[data.outputs.length - 1] : undefined; if (existing) { - // tslint:disable-next-line:restrict-plus-operands - const originalText = formatStreamText(concatMultilineString(existing.text + msg.content.text)); + const originalText = formatStreamText( + // tslint:disable-next-line: no-any + `${concatMultilineString(existing.text as any)}${concatMultilineString(msg.content.text)}` + ); originalTextLength = originalText.length; const newText = trimFunc(originalText); trimmedTextLength = newText.length; @@ -1324,7 +1326,7 @@ export class JupyterNotebookBase implements INotebook { text: [trimFunc(originalText)] }; data.outputs = [...data.outputs, output]; - trimmedTextLength = output.text.length; + trimmedTextLength = output.text[0].length; cell.data = data; } @@ -1333,13 +1335,11 @@ export class JupyterNotebookBase implements INotebook { // the output is trimmed and what setting changes that. // * If data.metadata.tags is undefined, define it so the following // code is can rely on it being defined. - if (data.metadata.tags === undefined) { - data.metadata.tags = []; - } - - data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend'); - if (trimmedTextLength < originalTextLength) { + if (data.metadata.tags === undefined) { + data.metadata.tags = []; + } + data.metadata.tags = data.metadata.tags.filter((t) => t !== 'outputPrepend'); data.metadata.tags.push('outputPrepend'); } } diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index 1114417ddad2..095c5b7c8d5b 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -915,19 +915,18 @@ df.head()`; "metadata": {}, "outputs": [ { - "data": { - "text/plain": [ - "2" - ] - }, - "execution_count": 2, - "metadata": {}, - "output_type": "execute_result" + "output_type": "stream", + "text": [ + "Hello World 9!\\n" + ], + "name": "stdout" } ], "source": [ - "b=2\\n", - "b" + "from IPython.display import clear_output\\n", + "for i in range(10):\\n", + " clear_output()\\n", + " print(\\"Hello World {0}!\\".format(i))\\n" ] }, { @@ -981,7 +980,13 @@ df.head()`; "nbformat_minor": 2 }`; addMockData(ioc, `a='<1>'\na`, `'<1>'`); - addMockData(ioc, 'b=2\nb', 2); + addContinuousMockData( + ioc, + 'from IPython.display import clear_output\nfor i in range(10):\n clear_output()\n print("Hello World {0}!".format(i))\n', + async () => { + return { result: 'Hello World 9!\n', haveMore: false }; + } + ); addMockData(ioc, 'c=3\nc', 3); const dsfs = ioc.get(IDataScienceFileSystem); const tf = await dsfs.createTemporaryLocalFile('.ipynb'); From d3f33b92273e7c0be3196e579397331b45cde8f8 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Wed, 30 Sep 2020 17:21:54 -0700 Subject: [PATCH 7/9] Fix traceback unit test --- src/test/datascience/jupyterUtils.unit.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/datascience/jupyterUtils.unit.test.ts b/src/test/datascience/jupyterUtils.unit.test.ts index 487c0205a729..6675afcbedee 100644 --- a/src/test/datascience/jupyterUtils.unit.test.ts +++ b/src/test/datascience/jupyterUtils.unit.test.ts @@ -87,7 +87,7 @@ suite('DataScience JupyterUtils', () => { }; // tslint:disable-next-line: no-any - return (hashProvider.preHandleIOPub(dummyMessage).content as any).traceback; + return (hashProvider.preHandleIOPub(dummyMessage).content as any).transient; } function addCell(code: string, file: string, line: number) { @@ -134,7 +134,7 @@ suite('DataScience JupyterUtils', () => { const after2 = [ '\u001b[1;31m---------------------------------------------------------------------------\u001b[0m', '\u001b[1;31mException\u001b[0m Traceback (most recent call last)', - `\u001b[1;32md:\\Training\\SnakePython\\manualTestFile.pytastic\u001b[0m in \u001b[0;36m\u001b[1;34m\u001b[0m\n\u001b[0;32m 4\u001b[0m \u001b[1;32mfor\u001b[0m \u001b[0mi\u001b[0m \u001b[1;32min\u001b[0m \u001b[0mtrange\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m100\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m:\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 5\u001b[0m \u001b[0mtime\u001b[0m\u001b[1;33m.\u001b[0m\u001b[0msleep\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m0.01\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 6\u001b[1;33m \u001b[1;32mraise\u001b[0m \u001b[0mException\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'spam'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m`, + `\u001b[1;32md:\\Training\\SnakePython\\manualTestFile.pytastic\u001b[0m in \u001b[0;36m\u001b[1;34m\u001b[0m\n\u001b[0;32m 4\u001b[0m \u001b[1;32mfor\u001b[0m \u001b[0mi\u001b[0m \u001b[1;32min\u001b[0m \u001b[0mtrange\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m100\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m:\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 5\u001b[0m \u001b[0mtime\u001b[0m\u001b[1;33m.\u001b[0m\u001b[0msleep\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;36m0.01\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 6\u001b[1;33m \u001b[1;32mraise\u001b[0m \u001b[0mException\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'spam'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m`, '\u001b[1;31mException\u001b[0m: spam' ]; assert.equal(after2.join('\n'), modifyTraceback(trace2).join('\n'), 'Exception failure'); @@ -158,8 +158,8 @@ import matplotlib.pyplot as plt`, const after3 = [ '\u001b[0;31m---------------------------------------------------------------------------\u001b[0m', '\u001b[0;31mModuleNotFoundError\u001b[0m Traceback (most recent call last)', - "\u001b[0;32m~/Test/manualTestFile.py\u001b[0m in \u001b[0;36m\u001b[0;34m\u001b[0m\n\u001b[0;32m----> 25\u001b[0;31m \u001b[0;32mimport\u001b[0m \u001b[0mnumpy\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mnp\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[0m\u001b[1;32m 26\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mpandas\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mpd\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[1;32m 27\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mmatplotlib\u001b[0m\u001b[0;34m.\u001b[0m\u001b[0mpyplot\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mplt\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n", - "\u001b[0;31mModuleNotFoundError\u001b[0m: No module named 'numpy'" + "\u001b[0;32m~/Test/manualTestFile.py\u001b[0m in \u001b[0;36m\u001b[0;34m\u001b[0m\n\u001b[0;32m----> 25\u001b[0;31m \u001b[0;32mimport\u001b[0m \u001b[0mnumpy\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mnp\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[0m\u001b[1;32m 26\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mpandas\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mpd\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n\u001b[1;32m 27\u001b[0m \u001b[0;32mimport\u001b[0m \u001b[0mmatplotlib\u001b[0m\u001b[0;34m.\u001b[0m\u001b[0mpyplot\u001b[0m \u001b[0;32mas\u001b[0m \u001b[0mplt\u001b[0m\u001b[0;34m\u001b[0m\u001b[0;34m\u001b[0m\u001b[0m\n", + '\u001b[0;31mModuleNotFoundError\u001b[0m: No module named 'numpy'' ]; assert.equal(after3.join('\n'), modifyTraceback(trace3).join('\n'), 'Exception unix failure'); when(fileSystem.getDisplayName(anything())).thenReturn('d:\\Training\\SnakePython\\foo.py'); @@ -194,8 +194,8 @@ cause_error()`, const after4 = [ '\u001b[1;31m---------------------------------------------------------------------------\u001b[0m', '\u001b[1;31mZeroDivisionError\u001b[0m Traceback (most recent call last)', - "\u001b[1;32md:\\Training\\SnakePython\\foo.py\u001b[0m in \u001b[0;36m\u001b[1;34m\u001b[0m\n\u001b[0;32m 144\u001b[0m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'some more'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 145\u001b[0m \u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 146\u001b[1;33m \u001b[0mcause_error\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m", - "\u001b[1;32md:\\Training\\SnakePython\\foo.py\u001b[0m in \u001b[0;36mcause_error\u001b[1;34m()\u001b[0m\n\u001b[0;32m 139\u001b[0m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'now'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 140\u001b[0m \u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 141\u001b[1;33m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m \u001b[1;36m1\u001b[0m \u001b[1;33m/\u001b[0m \u001b[1;36m0\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m" + "\u001b[1;32md:\\Training\\SnakePython\\foo.py\u001b[0m in \u001b[0;36m\u001b[1;34m\u001b[0m\n\u001b[0;32m 144\u001b[0m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'some more'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 145\u001b[0m \u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 146\u001b[1;33m \u001b[0mcause_error\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m", + "\u001b[1;32md:\\Training\\SnakePython\\foo.py\u001b[0m in \u001b[0;36mcause_error\u001b[1;34m()\u001b[0m\n\u001b[0;32m 139\u001b[0m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m\u001b[1;34m'now'\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0;32m 140\u001b[0m \u001b[1;33m\u001b[0m\u001b[0m\n\u001b[1;32m----> 141\u001b[1;33m \u001b[0mprint\u001b[0m\u001b[1;33m(\u001b[0m \u001b[1;36m1\u001b[0m \u001b[1;33m/\u001b[0m \u001b[1;36m0\u001b[0m\u001b[1;33m)\u001b[0m\u001b[1;33m\u001b[0m\u001b[1;33m\u001b[0m\u001b[0m\n\u001b[0m" ]; assert.equal(after4.join('\n'), modifyTraceback(trace4).join('\n'), 'Multiple levels'); }); From 75374a58c910f78c80bf36330a68c2ba3eb7eadf Mon Sep 17 00:00:00 2001 From: rchiodo Date: Thu, 1 Oct 2020 09:39:05 -0700 Subject: [PATCH 8/9] Fix problem caught by functional tests :) --- .../interactive-common/cellOutput.tsx | 2 +- src/test/datascience/notebook.functional.test.ts | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/datascience-ui/interactive-common/cellOutput.tsx b/src/datascience-ui/interactive-common/cellOutput.tsx index 109e828eac96..c05e787267d2 100644 --- a/src/datascience-ui/interactive-common/cellOutput.tsx +++ b/src/datascience-ui/interactive-common/cellOutput.tsx @@ -330,7 +330,7 @@ export class CellOutput extends React.Component { // tslint:disable-next-line: no-any const text = (input as any)['text/plain']; input = { - 'text/html': lodashEscape(text) + 'text/html': lodashEscape(concatMultilineString(text)) }; } else if (output.output_type === 'stream') { mimeType = 'text/html'; diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index 5a00e31c716c..965553349cc3 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -128,7 +128,7 @@ suite('DataScience notebook tests', () => { return path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'datascience'); } - function extractDataOutput(cell: ICell): any { + function extractDataOutput(cell: ICell): string | undefined { assert.equal(cell.data.cell_type, 'code', `Wrong type of cell returned`); const codeCell = cell.data as nbformat.ICodeCell; if (codeCell.outputs.length > 0) { @@ -143,7 +143,7 @@ suite('DataScience notebook tests', () => { // For linter assert.ok(data.hasOwnProperty('text/plain'), `Cell mime type not correct`); assert.ok((data as any)['text/plain'], `Cell mime type not correct`); - return (data as any)['text/plain']; + return concatMultilineString((data as any)['text/plain']); } } } @@ -157,9 +157,9 @@ suite('DataScience notebook tests', () => { const cells = await notebook!.execute(code, path.join(srcDirectory(), 'foo.py'), 2, uuid()); assert.equal(cells.length, 1, `Wrong number of cells returned`); const data = extractDataOutput(cells[0]); - if (pathVerify) { + if (pathVerify && data) { // For a path comparison normalize output - const normalizedOutput = path.normalize(data).toUpperCase().replace(/'/g, ''); + const normalizedOutput = path.normalize(data).toUpperCase().replace(/'/g, ''); const normalizedTarget = path.normalize(expectedValue).toUpperCase().replace(/'/g, ''); assert.equal(normalizedOutput, normalizedTarget, 'Cell path values does not match'); } else { @@ -1338,7 +1338,10 @@ plt.show()`, } public async postExecute(cell: ICell, silent: boolean): Promise { if (!silent) { - outputs.push(extractDataOutput(cell)); + const data = extractDataOutput(cell); + if (data) { + outputs.push(data); + } } } } From 2534df53b8c2a7e43af6077ba750644345461501 Mon Sep 17 00:00:00 2001 From: rchiodo Date: Thu, 1 Oct 2020 10:13:28 -0700 Subject: [PATCH 9/9] Another functional test catch --- src/datascience-ui/interactive-common/cellOutput.tsx | 2 +- src/datascience-ui/react-common/postOffice.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/datascience-ui/interactive-common/cellOutput.tsx b/src/datascience-ui/interactive-common/cellOutput.tsx index c05e787267d2..33cea77eecbb 100644 --- a/src/datascience-ui/interactive-common/cellOutput.tsx +++ b/src/datascience-ui/interactive-common/cellOutput.tsx @@ -402,7 +402,7 @@ export class CellOutput extends React.Component { } // Make sure text output is escaped (nteract texttransform won't) - if (mimeType === 'text/plain') { + if (mimeType === 'text/plain' && data) { data = lodashEscape(data.toString()); mimeType = 'text/html'; } diff --git a/src/datascience-ui/react-common/postOffice.ts b/src/datascience-ui/react-common/postOffice.ts index 8e671b2376e1..041432356cc1 100644 --- a/src/datascience-ui/react-common/postOffice.ts +++ b/src/datascience-ui/react-common/postOffice.ts @@ -88,7 +88,7 @@ export class PostOffice implements IDisposable { // See ./src/datascience-ui/native-editor/index.html // tslint:disable-next-line: no-any const api = (this.vscodeApi as any) as { handleMessage?: Function }; - if (api.handleMessage) { + if (api && api.handleMessage) { api.handleMessage(this.handleMessages.bind(this)); } } catch {