From 72806b44034caf4fb0202beef64d5aece7017681 Mon Sep 17 00:00:00 2001 From: aamunger Date: Mon, 17 Jul 2023 15:15:51 -0700 Subject: [PATCH] add/fix tests --- extensions/notebook-renderers/src/index.ts | 45 +++--------- .../notebook-renderers/src/rendererTypes.ts | 7 ++ .../src/test/notebookRenderer.test.ts | 73 ++++++++++++++++--- .../notebook-renderers/src/textHelper.ts | 63 +++++++++++----- 4 files changed, 122 insertions(+), 66 deletions(-) diff --git a/extensions/notebook-renderers/src/index.ts b/extensions/notebook-renderers/src/index.ts index 62c9fe70295..090e9719420 100644 --- a/extensions/notebook-renderers/src/index.ts +++ b/extensions/notebook-renderers/src/index.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import type { ActivationFunction, OutputItem, RendererContext } from 'vscode-notebook-renderer'; -import { appendScrollableOutput, createOutputContent, scrollableClass } from './textHelper'; +import { createOutputContent, appendOutput, scrollableClass } from './textHelper'; import { HtmlRenderingHook, IDisposable, IRichRenderContext, JavaScriptRenderingHook, OutputWithAppend, RenderOptions } from './rendererTypes'; import { ttPolicy } from './htmlHelper'; @@ -152,7 +152,7 @@ function renderError( outputInfo: OutputItem, outputElement: HTMLElement, ctx: IRichRenderContext, - trustHTML: boolean + trustHtml: boolean ): IDisposable { const disposableStore = createDisposableStore(); @@ -172,7 +172,7 @@ function renderError( outputElement.classList.add('traceback'); const outputScrolling = scrollingEnabled(outputInfo, ctx.settings); - const content = createOutputContent(outputInfo.id, err.stack ?? '', ctx.settings.lineLimit, outputScrolling, trustHTML); + const content = createOutputContent(outputInfo.id, err.stack ?? '', { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml }); const contentParent = document.createElement('div'); contentParent.classList.toggle('word-wrap', ctx.settings.outputWordWrap); disposableStore.push(ctx.onDidChangeSettings(e => { @@ -271,9 +271,9 @@ function scrollingEnabled(output: OutputItem, options: RenderOptions) { // div.scrollable? tabindex="0" <-- contentParent // div output-item-id="{guid}" <-- content from outputItem parameter function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, error: boolean, ctx: IRichRenderContext): IDisposable { - const appendedText = outputInfo.appendedText?.(); const disposableStore = createDisposableStore(); const outputScrolling = scrollingEnabled(outputInfo, ctx.settings); + const outputOptions = { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml: false, error }; outputElement.classList.add('output-stream'); @@ -284,15 +284,9 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, if (previousOutputParent) { const existingContent = previousOutputParent.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null; if (existingContent) { - if (appendedText && outputScrolling) { - appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false); - } - else { - const newContent = createContent(outputInfo, ctx, outputScrolling, error); - existingContent.replaceWith(newContent); - } + appendOutput(outputInfo, existingContent, outputOptions); } else { - const newContent = createContent(outputInfo, ctx, outputScrolling, error); + const newContent = createOutputContent(outputInfo.id, outputInfo.text(), outputOptions); previousOutputParent.appendChild(newContent); } previousOutputParent.classList.toggle('scrollbar-visible', previousOutputParent.scrollHeight > previousOutputParent.clientHeight); @@ -301,20 +295,9 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, const existingContent = outputElement.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null; let contentParent = existingContent?.parentElement; if (existingContent && contentParent) { - // appending output only in scrollable ouputs currently - if (appendedText && outputScrolling) { - appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false); - } - else { - const newContent = createContent(outputInfo, ctx, outputScrolling, error); - existingContent.replaceWith(newContent); - while (newContent.nextSibling) { - // clear out any stale content if we had previously combined streaming outputs into this one - newContent.nextSibling.remove(); - } - } + appendOutput(outputInfo, existingContent, outputOptions); } else { - const newContent = createContent(outputInfo, ctx, outputScrolling, error); + const newContent = createOutputContent(outputInfo.id, outputInfo.text(), outputOptions); contentParent = document.createElement('div'); contentParent.appendChild(newContent); while (outputElement.firstChild) { @@ -335,23 +318,13 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, return disposableStore; } -function createContent(outputInfo: OutputWithAppend, ctx: IRichRenderContext, outputScrolling: boolean, error: boolean) { - const text = outputInfo.text(); - const newContent = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false); - newContent.setAttribute('output-item-id', outputInfo.id); - if (error) { - newContent.classList.add('error'); - } - return newContent; -} - function renderText(outputInfo: OutputItem, outputElement: HTMLElement, ctx: IRichRenderContext): IDisposable { const disposableStore = createDisposableStore(); clearContainer(outputElement); const text = outputInfo.text(); const outputScrolling = scrollingEnabled(outputInfo, ctx.settings); - const content = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false); + const content = createOutputContent(outputInfo.id, text, { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml: false }); content.classList.add('output-plaintext'); if (ctx.settings.outputWordWrap) { content.classList.add('word-wrap'); diff --git a/extensions/notebook-renderers/src/rendererTypes.ts b/extensions/notebook-renderers/src/rendererTypes.ts index 77f9ebac472..ded12bdcacc 100644 --- a/extensions/notebook-renderers/src/rendererTypes.ts +++ b/extensions/notebook-renderers/src/rendererTypes.ts @@ -36,6 +36,13 @@ export interface RenderOptions { export type IRichRenderContext = RendererContext & { readonly settings: RenderOptions; readonly onDidChangeSettings: Event }; +export type OutputElementOptions = { + linesLimit: number; + scrollable?: boolean; + error?: boolean; + trustHtml?: boolean; +}; + export interface OutputWithAppend extends OutputItem { appendedText?(): string | undefined; } diff --git a/extensions/notebook-renderers/src/test/notebookRenderer.test.ts b/extensions/notebook-renderers/src/test/notebookRenderer.test.ts index 3c0c609140d..0f747900377 100644 --- a/extensions/notebook-renderers/src/test/notebookRenderer.test.ts +++ b/extensions/notebook-renderers/src/test/notebookRenderer.test.ts @@ -193,14 +193,14 @@ suite('Notebook builtin output renderer', () => { }); test('Append streaming output', async () => { - const context = createContext({ outputWordWrap: false, outputScrolling: false }); + const context = createContext({ outputWordWrap: false, outputScrolling: true }); const renderer = await activate(context); assert.ok(renderer, 'Renderer not created'); const outputElement = new OutputHtml().getFirstOuputElement(); const outputItem = createOutputItem('content', stdoutMimeType, '123', 'ignoredAppend'); await renderer!.renderOutputItem(outputItem, outputElement); - const outputItem2 = createOutputItem('content\nappended', stdoutMimeType, '\nappended'); + const outputItem2 = createOutputItem('content\nappended', stdoutMimeType, '123', '\nappended'); await renderer!.renderOutputItem(outputItem2, outputElement); const inserted = outputElement.firstChild as HTMLElement; @@ -210,39 +210,67 @@ suite('Notebook builtin output renderer', () => { assert.ok(inserted.innerHTML.indexOf('>contentcontent { + const context = createContext({ outputScrolling: true }); + const renderer = await activate(context); + assert.ok(renderer, 'Renderer not created'); + + const outputHtml = new OutputHtml(); + const firstOutputElement = outputHtml.getFirstOuputElement(); + const outputItem1 = createOutputItem('first stream content', stdoutMimeType, '1'); + const outputItem2 = createOutputItem(JSON.stringify(error), errorMimeType, '2'); + const outputItem3 = createOutputItem('second stream content', stdoutMimeType, '3'); + await renderer!.renderOutputItem(outputItem1, firstOutputElement); + const secondOutputElement = outputHtml.appendOutputElement(); + await renderer!.renderOutputItem(outputItem2, secondOutputElement); + const thirdOutputElement = outputHtml.appendOutputElement(); + await renderer!.renderOutputItem(outputItem3, thirdOutputElement); + + const appendedItem1 = createOutputItem('', stdoutMimeType, '1', ' appended1'); + await renderer!.renderOutputItem(appendedItem1, firstOutputElement); + const appendedItem3 = createOutputItem('', stdoutMimeType, '3', ' appended3'); + await renderer!.renderOutputItem(appendedItem3, thirdOutputElement); + + assert.ok(firstOutputElement.innerHTML.indexOf('>first stream content') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(firstOutputElement.innerHTML.indexOf('appended1') > -1, `Content was not appended to output element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(secondOutputElement.innerHTML.indexOf('>NameError -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(thirdOutputElement.innerHTML.indexOf('>second stream content') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(thirdOutputElement.innerHTML.indexOf('appended3') > -1, `Content was not appended to output element: ${outputHtml.cellElement.innerHTML}`); + }); + test('Append large streaming outputs', async () => { - const context = createContext({ outputWordWrap: false, outputScrolling: false }); + const context = createContext({ outputWordWrap: false, outputScrolling: true }); const renderer = await activate(context); assert.ok(renderer, 'Renderer not created'); const outputElement = new OutputHtml().getFirstOuputElement(); - const lotsOfLines = new Array(4998).fill('line').join('\n') + 'endOfInitialContent'; + const lotsOfLines = new Array(4998).fill('line').join('\n'); const firstOuput = lotsOfLines + 'expected1'; const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123'); await renderer!.renderOutputItem(outputItem, outputElement); const appended = '\n' + lotsOfLines + 'expectedAppend'; - const outputItem2 = createOutputItem(firstOuput + appended, stdoutMimeType, appended); + const outputItem2 = createOutputItem(firstOuput + appended, stdoutMimeType, '123', appended); await renderer!.renderOutputItem(outputItem2, outputElement); const inserted = outputElement.firstChild as HTMLElement; - assert.ok(inserted.innerHTML.indexOf('>expected1expectedAppend { - const context = createContext({ outputWordWrap: false, outputScrolling: false }); + const context = createContext({ outputWordWrap: false, outputScrolling: true }); const renderer = await activate(context); assert.ok(renderer, 'Renderer not created'); const outputElement = new OutputHtml().getFirstOuputElement(); - const lotsOfLines = new Array(11000).fill('line').join('\n') + 'endOfInitialContent'; + const lotsOfLines = new Array(11000).fill('line').join('\n'); const firstOuput = 'shouldBeTruncated' + lotsOfLines + 'expected1'; const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123'); await renderer!.renderOutputItem(outputItem, outputElement); const inserted = outputElement.firstChild as HTMLElement; - assert.ok(inserted.innerHTML.indexOf('>endOfInitialContentshouldBeTruncated { @@ -324,6 +352,29 @@ suite('Notebook builtin output renderer', () => { assert.ok(inserted.innerHTML.indexOf('>second stream content { + const context = createContext({ outputScrolling: true }); + const renderer = await activate(context); + assert.ok(renderer, 'Renderer not created'); + + const outputHtml = new OutputHtml(); + const outputElement = outputHtml.getFirstOuputElement(); + const outputItem1 = createOutputItem('first stream content', stdoutMimeType, '1'); + const outputItem2 = createOutputItem('second stream content', stdoutMimeType, '2'); + await renderer!.renderOutputItem(outputItem1, outputElement); + const secondOutput = outputHtml.appendOutputElement(); + await renderer!.renderOutputItem(outputItem2, secondOutput); + const appendingOutput = createOutputItem('', stdoutMimeType, '2', ' appended'); + await renderer!.renderOutputItem(appendingOutput, secondOutput); + + + const inserted = outputElement.firstChild as HTMLElement; + assert.ok(inserted, `nothing appended to output element: ${outputElement.innerHTML}`); + assert.ok(inserted.innerHTML.indexOf('>first stream content -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(inserted.innerHTML.indexOf('>second stream content') > -1, `Second content was not added to ouptut element: ${outputHtml.cellElement.innerHTML}`); + assert.ok(inserted.innerHTML.indexOf('appended') > -1, `Content was not appended to ouptut element: ${outputHtml.cellElement.innerHTML}`); + }); + test(`Streaming outputs interleaved with other mime types will produce separate outputs`, async () => { const context = createContext({ outputScrolling: false }); const renderer = await activate(context); diff --git a/extensions/notebook-renderers/src/textHelper.ts b/extensions/notebook-renderers/src/textHelper.ts index 0c65e0f6d93..5cf0e24eb96 100644 --- a/extensions/notebook-renderers/src/textHelper.ts +++ b/extensions/notebook-renderers/src/textHelper.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { handleANSIOutput } from './ansi'; +import { OutputElementOptions, OutputWithAppend } from './rendererTypes'; export const scrollableClass = 'scrollable'; const softScrollableLineLimit = 5000; @@ -102,37 +103,61 @@ function scrollableArrayOfString(id: string, buffer: string[], trustHtml: boolea return element; } -export function createOutputContent(id: string, outputText: string, linesLimit: number, scrollable: boolean, trustHtml: boolean): HTMLElement { - - const buffer = outputText.split(/\r\n|\r|\n/g); - - if (scrollable) { - return scrollableArrayOfString(id, buffer, trustHtml); - } else { - return truncatedArrayOfString(id, buffer, linesLimit, trustHtml); - } -} - const outputLengths: Record = {}; -export function appendScrollableOutput(element: HTMLElement, id: string, appended: string, fullText: string, trustHtml: boolean) { +function appendScrollableOutput(element: HTMLElement, id: string, appended: string, trustHtml: boolean) { if (!outputLengths[id]) { outputLengths[id] = 0; } const buffer = appended.split(/\r\n|\r|\n/g); const appendedLength = buffer.length + outputLengths[id]; - // Allow the output to grow to the hard limit then replace it with the last softLimit number of lines if it grows too large - if (buffer.length + outputLengths[id] > hardScrollableLineLimit) { - const fullBuffer = fullText.split(/\r\n|\r|\n/g); - outputLengths[id] = Math.min(fullBuffer.length, softScrollableLineLimit); - const newElement = scrollableArrayOfString(id, fullBuffer.slice(-1 * softScrollableLineLimit), trustHtml); - newElement.setAttribute('output-item-id', id); - element.replaceWith(newElement); + // Only append outputs up to the hard limit of lines, then replace it with the last softLimit number of lines + if (appendedLength > hardScrollableLineLimit) { + return false; } else { element.appendChild(handleANSIOutput(buffer.join('\n'), trustHtml)); outputLengths[id] = appendedLength; } + return true; +} + +export function createOutputContent(id: string, outputText: string, options: OutputElementOptions): HTMLElement { + const { linesLimit, error, scrollable, trustHtml } = options; + const buffer = outputText.split(/\r\n|\r|\n/g); + outputLengths[id] = outputLengths[id] = Math.min(buffer.length, softScrollableLineLimit); + + let outputElement: HTMLElement; + if (scrollable) { + outputElement = scrollableArrayOfString(id, buffer, !!trustHtml); + } else { + outputElement = truncatedArrayOfString(id, buffer, linesLimit, !!trustHtml); + } + + outputElement.setAttribute('output-item-id', id); + if (error) { + outputElement.classList.add('error'); + } + + return outputElement; +} + +export function appendOutput(outputInfo: OutputWithAppend, existingContent: HTMLElement, options: OutputElementOptions) { + const appendedText = outputInfo.appendedText?.(); + // appending output only supported for scrollable ouputs currently + if (appendedText && options.scrollable) { + if (appendScrollableOutput(existingContent, outputInfo.id, appendedText, false)) { + return; + } + } + + const newContent = createOutputContent(outputInfo.id, outputInfo.text(), options); + existingContent.replaceWith(newContent); + while (newContent.nextSibling) { + // clear out any stale content if we had previously combined streaming outputs into this one + newContent.nextSibling.remove(); + } + }