Skip to content

Commit 72806b4

Browse files
amungeramunger
committed
add/fix tests
1 parent 6501a3b commit 72806b4

File tree

4 files changed

+122
-66
lines changed

4 files changed

+122
-66
lines changed

‎extensions/notebook-renderers/src/index.ts

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import type { ActivationFunction, OutputItem, RendererContext } from 'vscode-notebook-renderer';
7-
import { appendScrollableOutput, createOutputContent, scrollableClass } from './textHelper';
7+
import { createOutputContent, appendOutput, scrollableClass } from './textHelper';
88
import { HtmlRenderingHook, IDisposable, IRichRenderContext, JavaScriptRenderingHook, OutputWithAppend, RenderOptions } from './rendererTypes';
99
import { ttPolicy } from './htmlHelper';
1010

@@ -152,7 +152,7 @@ function renderError(
152152
outputInfo: OutputItem,
153153
outputElement: HTMLElement,
154154
ctx: IRichRenderContext,
155-
trustHTML: boolean
155+
trustHtml: boolean
156156
): IDisposable {
157157
const disposableStore = createDisposableStore();
158158

@@ -172,7 +172,7 @@ function renderError(
172172
outputElement.classList.add('traceback');
173173

174174
const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);
175-
const content = createOutputContent(outputInfo.id, err.stack ?? '', ctx.settings.lineLimit, outputScrolling, trustHTML);
175+
const content = createOutputContent(outputInfo.id, err.stack ?? '', { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml });
176176
const contentParent = document.createElement('div');
177177
contentParent.classList.toggle('word-wrap', ctx.settings.outputWordWrap);
178178
disposableStore.push(ctx.onDidChangeSettings(e => {
@@ -271,9 +271,9 @@ function scrollingEnabled(output: OutputItem, options: RenderOptions) {
271271
// div.scrollable? tabindex="0" <-- contentParent
272272
// div output-item-id="{guid}" <-- content from outputItem parameter
273273
function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement, error: boolean, ctx: IRichRenderContext): IDisposable {
274-
const appendedText = outputInfo.appendedText?.();
275274
const disposableStore = createDisposableStore();
276275
const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);
276+
const outputOptions = { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml: false, error };
277277

278278
outputElement.classList.add('output-stream');
279279

@@ -284,15 +284,9 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement,
284284
if (previousOutputParent) {
285285
const existingContent = previousOutputParent.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null;
286286
if (existingContent) {
287-
if (appendedText && outputScrolling) {
288-
appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false);
289-
}
290-
else {
291-
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
292-
existingContent.replaceWith(newContent);
293-
}
287+
appendOutput(outputInfo, existingContent, outputOptions);
294288
} else {
295-
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
289+
const newContent = createOutputContent(outputInfo.id, outputInfo.text(), outputOptions);
296290
previousOutputParent.appendChild(newContent);
297291
}
298292
previousOutputParent.classList.toggle('scrollbar-visible', previousOutputParent.scrollHeight > previousOutputParent.clientHeight);
@@ -301,20 +295,9 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement,
301295
const existingContent = outputElement.querySelector(`[output-item-id="${outputInfo.id}"]`) as HTMLElement | null;
302296
let contentParent = existingContent?.parentElement;
303297
if (existingContent && contentParent) {
304-
// appending output only in scrollable ouputs currently
305-
if (appendedText && outputScrolling) {
306-
appendScrollableOutput(existingContent, outputInfo.id, appendedText, outputInfo.text(), false);
307-
}
308-
else {
309-
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
310-
existingContent.replaceWith(newContent);
311-
while (newContent.nextSibling) {
312-
// clear out any stale content if we had previously combined streaming outputs into this one
313-
newContent.nextSibling.remove();
314-
}
315-
}
298+
appendOutput(outputInfo, existingContent, outputOptions);
316299
} else {
317-
const newContent = createContent(outputInfo, ctx, outputScrolling, error);
300+
const newContent = createOutputContent(outputInfo.id, outputInfo.text(), outputOptions);
318301
contentParent = document.createElement('div');
319302
contentParent.appendChild(newContent);
320303
while (outputElement.firstChild) {
@@ -335,23 +318,13 @@ function renderStream(outputInfo: OutputWithAppend, outputElement: HTMLElement,
335318
return disposableStore;
336319
}
337320

338-
function createContent(outputInfo: OutputWithAppend, ctx: IRichRenderContext, outputScrolling: boolean, error: boolean) {
339-
const text = outputInfo.text();
340-
const newContent = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false);
341-
newContent.setAttribute('output-item-id', outputInfo.id);
342-
if (error) {
343-
newContent.classList.add('error');
344-
}
345-
return newContent;
346-
}
347-
348321
function renderText(outputInfo: OutputItem, outputElement: HTMLElement, ctx: IRichRenderContext): IDisposable {
349322
const disposableStore = createDisposableStore();
350323
clearContainer(outputElement);
351324

352325
const text = outputInfo.text();
353326
const outputScrolling = scrollingEnabled(outputInfo, ctx.settings);
354-
const content = createOutputContent(outputInfo.id, text, ctx.settings.lineLimit, outputScrolling, false);
327+
const content = createOutputContent(outputInfo.id, text, { linesLimit: ctx.settings.lineLimit, scrollable: outputScrolling, trustHtml: false });
355328
content.classList.add('output-plaintext');
356329
if (ctx.settings.outputWordWrap) {
357330
content.classList.add('word-wrap');

‎extensions/notebook-renderers/src/rendererTypes.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@ export interface RenderOptions {
3636

3737
export type IRichRenderContext = RendererContext<void> & { readonly settings: RenderOptions; readonly onDidChangeSettings: Event<RenderOptions> };
3838

39+
export type OutputElementOptions = {
40+
linesLimit: number;
41+
scrollable?: boolean;
42+
error?: boolean;
43+
trustHtml?: boolean;
44+
};
45+
3946
export interface OutputWithAppend extends OutputItem {
4047
appendedText?(): string | undefined;
4148
}

‎extensions/notebook-renderers/src/test/notebookRenderer.test.ts

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -193,14 +193,14 @@ suite('Notebook builtin output renderer', () => {
193193
});
194194

195195
test('Append streaming output', async () => {
196-
const context = createContext({ outputWordWrap: false, outputScrolling: false });
196+
const context = createContext({ outputWordWrap: false, outputScrolling: true });
197197
const renderer = await activate(context);
198198
assert.ok(renderer, 'Renderer not created');
199199

200200
const outputElement = new OutputHtml().getFirstOuputElement();
201201
const outputItem = createOutputItem('content', stdoutMimeType, '123', 'ignoredAppend');
202202
await renderer!.renderOutputItem(outputItem, outputElement);
203-
const outputItem2 = createOutputItem('content\nappended', stdoutMimeType, '\nappended');
203+
const outputItem2 = createOutputItem('content\nappended', stdoutMimeType, '123', '\nappended');
204204
await renderer!.renderOutputItem(outputItem2, outputElement);
205205

206206
const inserted = outputElement.firstChild as HTMLElement;
@@ -210,39 +210,67 @@ suite('Notebook builtin output renderer', () => {
210210
assert.ok(inserted.innerHTML.indexOf('>content</') === inserted.innerHTML.lastIndexOf('>content</'), `Original content should not be duplicated: ${outputElement.innerHTML}`);
211211
});
212212

213+
test(`Appending multiple streaming outputs`, async () => {
214+
const context = createContext({ outputScrolling: true });
215+
const renderer = await activate(context);
216+
assert.ok(renderer, 'Renderer not created');
217+
218+
const outputHtml = new OutputHtml();
219+
const firstOutputElement = outputHtml.getFirstOuputElement();
220+
const outputItem1 = createOutputItem('first stream content', stdoutMimeType, '1');
221+
const outputItem2 = createOutputItem(JSON.stringify(error), errorMimeType, '2');
222+
const outputItem3 = createOutputItem('second stream content', stdoutMimeType, '3');
223+
await renderer!.renderOutputItem(outputItem1, firstOutputElement);
224+
const secondOutputElement = outputHtml.appendOutputElement();
225+
await renderer!.renderOutputItem(outputItem2, secondOutputElement);
226+
const thirdOutputElement = outputHtml.appendOutputElement();
227+
await renderer!.renderOutputItem(outputItem3, thirdOutputElement);
228+
229+
const appendedItem1 = createOutputItem('', stdoutMimeType, '1', ' appended1');
230+
await renderer!.renderOutputItem(appendedItem1, firstOutputElement);
231+
const appendedItem3 = createOutputItem('', stdoutMimeType, '3', ' appended3');
232+
await renderer!.renderOutputItem(appendedItem3, thirdOutputElement);
233+
234+
assert.ok(firstOutputElement.innerHTML.indexOf('>first stream content') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`);
235+
assert.ok(firstOutputElement.innerHTML.indexOf('appended1') > -1, `Content was not appended to output element: ${outputHtml.cellElement.innerHTML}`);
236+
assert.ok(secondOutputElement.innerHTML.indexOf('>NameError</') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`);
237+
assert.ok(thirdOutputElement.innerHTML.indexOf('>second stream content') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`);
238+
assert.ok(thirdOutputElement.innerHTML.indexOf('appended3') > -1, `Content was not appended to output element: ${outputHtml.cellElement.innerHTML}`);
239+
});
240+
213241
test('Append large streaming outputs', async () => {
214-
const context = createContext({ outputWordWrap: false, outputScrolling: false });
242+
const context = createContext({ outputWordWrap: false, outputScrolling: true });
215243
const renderer = await activate(context);
216244
assert.ok(renderer, 'Renderer not created');
217245

218246
const outputElement = new OutputHtml().getFirstOuputElement();
219-
const lotsOfLines = new Array(4998).fill('line').join('\n') + 'endOfInitialContent';
247+
const lotsOfLines = new Array(4998).fill('line').join('\n');
220248
const firstOuput = lotsOfLines + 'expected1';
221249
const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123');
222250
await renderer!.renderOutputItem(outputItem, outputElement);
223251
const appended = '\n' + lotsOfLines + 'expectedAppend';
224-
const outputItem2 = createOutputItem(firstOuput + appended, stdoutMimeType, appended);
252+
const outputItem2 = createOutputItem(firstOuput + appended, stdoutMimeType, '123', appended);
225253
await renderer!.renderOutputItem(outputItem2, outputElement);
226254

227255
const inserted = outputElement.firstChild as HTMLElement;
228-
assert.ok(inserted.innerHTML.indexOf('>expected1</') !== -1, `Last bit of previous content should still exist: ${outputElement.innerHTML}`);
229-
assert.ok(inserted.innerHTML.indexOf('>expectedAppend</') !== -1, `Content was not appended to output element: ${outputElement.innerHTML}`);
256+
assert.ok(inserted.innerHTML.indexOf('expected1') !== -1, `Last bit of previous content should still exist`);
257+
assert.ok(inserted.innerHTML.indexOf('expectedAppend') !== -1, `Content was not appended to output element`);
230258
});
231259

232260
test('Streaming outputs larger than the line limit are truncated', async () => {
233-
const context = createContext({ outputWordWrap: false, outputScrolling: false });
261+
const context = createContext({ outputWordWrap: false, outputScrolling: true });
234262
const renderer = await activate(context);
235263
assert.ok(renderer, 'Renderer not created');
236264

237265
const outputElement = new OutputHtml().getFirstOuputElement();
238-
const lotsOfLines = new Array(11000).fill('line').join('\n') + 'endOfInitialContent';
266+
const lotsOfLines = new Array(11000).fill('line').join('\n');
239267
const firstOuput = 'shouldBeTruncated' + lotsOfLines + 'expected1';
240268
const outputItem = createOutputItem(firstOuput, stdoutMimeType, '123');
241269
await renderer!.renderOutputItem(outputItem, outputElement);
242270

243271
const inserted = outputElement.firstChild as HTMLElement;
244-
assert.ok(inserted.innerHTML.indexOf('>endOfInitialContent</') !== -1, `Last bit of content should exist: ${outputElement.innerHTML}`);
245-
assert.ok(inserted.innerHTML.indexOf('>shouldBeTruncated</') === -1, `Beginning content should be truncated: ${outputElement.innerHTML}`);
272+
assert.ok(inserted.innerHTML.indexOf('expected1') !== -1, `Last bit of content should exist`);
273+
assert.ok(inserted.innerHTML.indexOf('shouldBeTruncated') === -1, `Beginning content should be truncated`);
246274
});
247275

248276
test(`Render with wordwrap and scrolling for error output`, async () => {
@@ -324,6 +352,29 @@ suite('Notebook builtin output renderer', () => {
324352
assert.ok(inserted.innerHTML.indexOf('>second stream content</') === -1, `Content was not replaced in output element: ${outputHtml.cellElement.innerHTML}`);
325353
});
326354

355+
test(`Consolidated streaming outputs should append matching outputs correctly`, async () => {
356+
const context = createContext({ outputScrolling: true });
357+
const renderer = await activate(context);
358+
assert.ok(renderer, 'Renderer not created');
359+
360+
const outputHtml = new OutputHtml();
361+
const outputElement = outputHtml.getFirstOuputElement();
362+
const outputItem1 = createOutputItem('first stream content', stdoutMimeType, '1');
363+
const outputItem2 = createOutputItem('second stream content', stdoutMimeType, '2');
364+
await renderer!.renderOutputItem(outputItem1, outputElement);
365+
const secondOutput = outputHtml.appendOutputElement();
366+
await renderer!.renderOutputItem(outputItem2, secondOutput);
367+
const appendingOutput = createOutputItem('', stdoutMimeType, '2', ' appended');
368+
await renderer!.renderOutputItem(appendingOutput, secondOutput);
369+
370+
371+
const inserted = outputElement.firstChild as HTMLElement;
372+
assert.ok(inserted, `nothing appended to output element: ${outputElement.innerHTML}`);
373+
assert.ok(inserted.innerHTML.indexOf('>first stream content</') > -1, `Content was not added to output element: ${outputHtml.cellElement.innerHTML}`);
374+
assert.ok(inserted.innerHTML.indexOf('>second stream content') > -1, `Second content was not added to ouptut element: ${outputHtml.cellElement.innerHTML}`);
375+
assert.ok(inserted.innerHTML.indexOf('appended') > -1, `Content was not appended to ouptut element: ${outputHtml.cellElement.innerHTML}`);
376+
});
377+
327378
test(`Streaming outputs interleaved with other mime types will produce separate outputs`, async () => {
328379
const context = createContext({ outputScrolling: false });
329380
const renderer = await activate(context);

‎extensions/notebook-renderers/src/textHelper.ts

Lines changed: 44 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { handleANSIOutput } from './ansi';
7+
import { OutputElementOptions, OutputWithAppend } from './rendererTypes';
78
export const scrollableClass = 'scrollable';
89

910
const softScrollableLineLimit = 5000;
@@ -102,37 +103,61 @@ function scrollableArrayOfString(id: string, buffer: string[], trustHtml: boolea
102103
return element;
103104
}
104105

105-
export function createOutputContent(id: string, outputText: string, linesLimit: number, scrollable: boolean, trustHtml: boolean): HTMLElement {
106-
107-
const buffer = outputText.split(/\r\n|\r|\n/g);
108-
109-
if (scrollable) {
110-
return scrollableArrayOfString(id, buffer, trustHtml);
111-
} else {
112-
return truncatedArrayOfString(id, buffer, linesLimit, trustHtml);
113-
}
114-
}
115-
116106
const outputLengths: Record<string, number> = {};
117107

118-
export function appendScrollableOutput(element: HTMLElement, id: string, appended: string, fullText: string, trustHtml: boolean) {
108+
function appendScrollableOutput(element: HTMLElement, id: string, appended: string, trustHtml: boolean) {
119109
if (!outputLengths[id]) {
120110
outputLengths[id] = 0;
121111
}
122112

123113
const buffer = appended.split(/\r\n|\r|\n/g);
124114
const appendedLength = buffer.length + outputLengths[id];
125-
// Allow the output to grow to the hard limit then replace it with the last softLimit number of lines if it grows too large
126-
if (buffer.length + outputLengths[id] > hardScrollableLineLimit) {
127-
const fullBuffer = fullText.split(/\r\n|\r|\n/g);
128-
outputLengths[id] = Math.min(fullBuffer.length, softScrollableLineLimit);
129-
const newElement = scrollableArrayOfString(id, fullBuffer.slice(-1 * softScrollableLineLimit), trustHtml);
130-
newElement.setAttribute('output-item-id', id);
131-
element.replaceWith(newElement);
115+
// Only append outputs up to the hard limit of lines, then replace it with the last softLimit number of lines
116+
if (appendedLength > hardScrollableLineLimit) {
117+
return false;
132118
}
133119
else {
134120
element.appendChild(handleANSIOutput(buffer.join('\n'), trustHtml));
135121
outputLengths[id] = appendedLength;
136122
}
123+
return true;
124+
}
125+
126+
export function createOutputContent(id: string, outputText: string, options: OutputElementOptions): HTMLElement {
127+
const { linesLimit, error, scrollable, trustHtml } = options;
128+
const buffer = outputText.split(/\r\n|\r|\n/g);
129+
outputLengths[id] = outputLengths[id] = Math.min(buffer.length, softScrollableLineLimit);
130+
131+
let outputElement: HTMLElement;
132+
if (scrollable) {
133+
outputElement = scrollableArrayOfString(id, buffer, !!trustHtml);
134+
} else {
135+
outputElement = truncatedArrayOfString(id, buffer, linesLimit, !!trustHtml);
136+
}
137+
138+
outputElement.setAttribute('output-item-id', id);
139+
if (error) {
140+
outputElement.classList.add('error');
141+
}
142+
143+
return outputElement;
144+
}
145+
146+
export function appendOutput(outputInfo: OutputWithAppend, existingContent: HTMLElement, options: OutputElementOptions) {
147+
const appendedText = outputInfo.appendedText?.();
148+
// appending output only supported for scrollable ouputs currently
149+
if (appendedText && options.scrollable) {
150+
if (appendScrollableOutput(existingContent, outputInfo.id, appendedText, false)) {
151+
return;
152+
}
153+
}
154+
155+
const newContent = createOutputContent(outputInfo.id, outputInfo.text(), options);
156+
existingContent.replaceWith(newContent);
157+
while (newContent.nextSibling) {
158+
// clear out any stale content if we had previously combined streaming outputs into this one
159+
newContent.nextSibling.remove();
160+
}
161+
137162
}
138163

0 commit comments

Comments
 (0)