Skip to content
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

Add error indicator in Table of Contents #14784

Merged
merged 21 commits into from
Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
15 changes: 15 additions & 0 deletions galata/test/jupyterlab/toc-running.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Jupyter Development Team.

Check failure on line 1 in galata/test/jupyterlab/toc-running.test.ts

View workflow job for this annotation

GitHub Actions / Visual Regression Tests

[jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators

2) [jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators Test finished within timeout of 60000ms, but tearing down "context" ran out of time. Please allow more time for the test, since teardown is attributed towards the test timeout budget.

Check failure on line 1 in galata/test/jupyterlab/toc-running.test.ts

View workflow job for this annotation

GitHub Actions / Visual Regression Tests

[jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators

2) [jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Test finished within timeout of 60000ms, but tearing down "context" ran out of time. Please allow more time for the test, since teardown is attributed towards the test timeout budget.
// Distributed under the terms of the Modified BSD License.

import { expect, test } from '@jupyterlab/galata';
Expand All @@ -14,6 +14,8 @@
await page.notebook.addCell('code', 'sleep(2)');
await page.notebook.addCell('markdown', '## Title 1.2');
await page.notebook.addCell('code', 'sleep(1)');
await page.notebook.addCell('markdown', '## Title 1.3');
await page.notebook.addCell('code', 'raise ValueError("Test error")');

await page.sidebar.openTab('table-of-contents');
await page.waitForSelector(
Expand All @@ -27,13 +29,26 @@
);
const executed = page.notebook.run();
await tocPanel.waitForSelector('[data-running="1"]');
expect(await tocPanel.screenshot()).toMatchSnapshot(

Check failure on line 32 in galata/test/jupyterlab/toc-running.test.ts

View workflow job for this annotation

GitHub Actions / Visual Regression Tests

[jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators

2) [jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators Error: Screenshot comparison failed: 178 pixels (ratio 0.01 of all image pixels) are different. Expected: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab/toc-running-indicators-expected.png Received: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab/toc-running-indicators-actual.png Diff: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab/toc-running-indicators-diff.png 30 | const executed = page.notebook.run(); 31 | await tocPanel.waitForSelector('[data-running="1"]'); > 32 | expect(await tocPanel.screenshot()).toMatchSnapshot( | ^ 33 | 'toc-running-indicators.png' 34 | ); 35 | at /home/runner/work/jupyterlab/jupyterlab/galata/test/jupyterlab/toc-running.test.ts:32:41

Check failure on line 32 in galata/test/jupyterlab/toc-running.test.ts

View workflow job for this annotation

GitHub Actions / Visual Regression Tests

[jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators

2) [jupyterlab] › test/jupyterlab/toc-running.test.ts:26:7 › ToC Running indicator › should display running indicators Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Screenshot comparison failed: 178 pixels (ratio 0.01 of all image pixels) are different. Expected: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab-retry1/toc-running-indicators-expected.png Received: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab-retry1/toc-running-indicators-actual.png Diff: /home/runner/work/jupyterlab/jupyterlab/galata/test-results/test-jupyterlab-toc-running-ToC-Running-indicator-should-display-running-indicators-jupyterlab-retry1/toc-running-indicators-diff.png 30 | const executed = page.notebook.run(); 31 | await tocPanel.waitForSelector('[data-running="1"]'); > 32 | expect(await tocPanel.screenshot()).toMatchSnapshot( | ^ 33 | 'toc-running-indicators.png' 34 | ); 35 | at /home/runner/work/jupyterlab/jupyterlab/galata/test/jupyterlab/toc-running.test.ts:32:41
'toc-running-indicators.png'
);

await executed;
});

test('should display error indicators', async ({ page }) => {
const tocPanel = await page.sidebar.getContentPanel(
await page.sidebar.getTabPosition('table-of-contents')
);
const executed = page.notebook.run();
await tocPanel.waitForSelector('[data-running="-1"]');
expect(await tocPanel.screenshot()).toMatchSnapshot(
'toc-running-indicator-error.png'
);

await executed;
});

test('should display running indicator on first visible top level', async ({
page
}) => {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions packages/notebook/src/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,13 @@ export class NotebookActions {
return Private.selectionExecuted;
}

/**
* A signal that emits when a cell's output is cleared.
*/
static get outputCleared(): ISignal<any, { notebook: Notebook; cell: Cell }> {
return Private.outputCleared;
}

/**
* A private constructor for the `NotebookActions` class.
*
Expand Down Expand Up @@ -1420,6 +1427,7 @@ export namespace NotebookActions {
(cell as ICodeCellModel).clearExecution();
(child as CodeCell).outputHidden = false;
}, false);
Private.outputCleared.emit({ notebook, cell: child });
}
}
Private.handleState(notebook, state, true);
Expand Down Expand Up @@ -1448,6 +1456,7 @@ export namespace NotebookActions {
(cell as ICodeCellModel).clearExecution();
(child as CodeCell).outputHidden = false;
}, false);
Private.outputCleared.emit({ notebook, cell: child });
}
}
Private.handleState(notebook, state, true);
Expand Down Expand Up @@ -2111,6 +2120,14 @@ namespace Private {
{ notebook: Notebook; lastCell: Cell }
>({});

/**
* A signal that emits when one notebook's cells are all executed.
*/
export const outputCleared = new Signal<
any,
{ notebook: Notebook; cell: Cell }
>({});

/**
* The interface for a widget state.
*/
Expand Down
72 changes: 64 additions & 8 deletions packages/notebook/src/toc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
TableOfContentsModel,
TableOfContentsUtils
} from '@jupyterlab/toc';
import { NotebookActions } from './actions';
import { KernelError, NotebookActions } from './actions';
import { NotebookPanel } from './panel';
import { INotebookTracker } from './tokens';
import { Notebook } from './widget';
Expand All @@ -21,7 +21,11 @@ export enum RunningStatus {
/**
* Cell is idle
*/
Idle = -1,
Idle = -2,
/**
* Cell execution is unsuccessful
*/
Error = -1,
skyetim marked this conversation as resolved.
Show resolved Hide resolved
/**
* Cell execution is scheduled
*/
Expand Down Expand Up @@ -80,6 +84,7 @@ export class NotebookToCModel extends TableOfContentsModel<
) {
super(widget, configuration);
this._runningCells = new Array<Cell>();
this._errorCells = new Array<Cell>();
this._cellToHeadingIndex = new WeakMap<Cell, number>();

void widget.context.ready.then(() => {
Expand All @@ -97,6 +102,7 @@ export class NotebookToCModel extends TableOfContentsModel<
);
NotebookActions.executionScheduled.connect(this.onExecutionScheduled, this);
NotebookActions.executed.connect(this.onExecuted, this);
NotebookActions.outputCleared.connect(this.onOutputCleared, this);
this.headingsChanged.connect(this.onHeadingsChanged, this);
}

Expand Down Expand Up @@ -180,8 +186,10 @@ export class NotebookToCModel extends TableOfContentsModel<
this
);
NotebookActions.executed.disconnect(this.onExecuted, this);
NotebookActions.outputCleared.disconnect(this.onOutputCleared, this);

this._runningCells.length = 0;
this._errorCells.length = 0;

super.dispose();
}
Expand Down Expand Up @@ -344,7 +352,12 @@ export class NotebookToCModel extends TableOfContentsModel<

protected onExecuted(
_: unknown,
args: { notebook: Notebook; cell: Cell }
args: {
notebook: Notebook;
cell: Cell;
success: boolean;
error: KernelError | null;
}
): void {
this._runningCells.forEach((cell, index) => {
if (cell === args.cell) {
Expand All @@ -353,7 +366,16 @@ export class NotebookToCModel extends TableOfContentsModel<
const headingIndex = this._cellToHeadingIndex.get(cell);
if (headingIndex !== undefined) {
const heading = this.headings[headingIndex];
heading.isRunning = RunningStatus.Idle;
// when the execution is not successful but errorName is undefined,
// the execution is interrupted by previous cells
if (args.success || args.error?.errorName === undefined) {
heading.isRunning = RunningStatus.Idle;
return;
}
heading.isRunning = RunningStatus.Error;
if (!this._errorCells.includes(cell)) {
this._errorCells.push(cell);
}
}
}
});
Expand All @@ -374,6 +396,25 @@ export class NotebookToCModel extends TableOfContentsModel<
this.stateChanged.emit();
}

protected onOutputCleared(
_: unknown,
args: { notebook: Notebook; cell: Cell }
): void {
this._errorCells.forEach((cell, index) => {
if (cell === args.cell) {
this._errorCells.splice(index, 1);

const headingIndex = this._cellToHeadingIndex.get(cell);
if (headingIndex !== undefined) {
const heading = this.headings[headingIndex];
heading.isRunning = RunningStatus.Idle;
}
}
});
this.updateRunningStatus(this.headings);
this.stateChanged.emit();
}

protected onMetadataChanged(): void {
this.setConfiguration({});
}
Expand All @@ -384,10 +425,24 @@ export class NotebookToCModel extends TableOfContentsModel<
const headingIndex = this._cellToHeadingIndex.get(cell);
if (headingIndex !== undefined) {
const heading = this.headings[headingIndex];
heading.isRunning = Math.max(
index > 0 ? RunningStatus.Scheduled : RunningStatus.Running,
heading.isRunning
);
// Running is prioritized over Scheduled, so if a heading is
// running don't change status
if (heading.isRunning !== RunningStatus.Running) {
heading.isRunning =
index > 0 ? RunningStatus.Scheduled : RunningStatus.Running;
}
}
});

this._errorCells.forEach((cell, index) => {
const headingIndex = this._cellToHeadingIndex.get(cell);
if (headingIndex !== undefined) {
const heading = this.headings[headingIndex];
// Running and Scheduled are prioritized over Error, so only if
// a heading is idle will it be set to Error
if (heading.isRunning === RunningStatus.Idle) {
heading.isRunning = RunningStatus.Error;
}
}
});

Expand Down Expand Up @@ -463,6 +518,7 @@ export class NotebookToCModel extends TableOfContentsModel<
};

private _runningCells: Cell[];
private _errorCells: Cell[];
private _cellToHeadingIndex: WeakMap<Cell, number>;
}

Expand Down
9 changes: 8 additions & 1 deletion packages/notebook/style/toc.css
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,14 @@
background-color: var(--jp-inverse-layout-color3);
}

.jp-tocItem-content[data-running='-1']::after {
border-radius: 50%;
border: var(--jp-border-width) solid var(--jp-error-color2);
background-color: var(--jp-error-color2);
skyetim marked this conversation as resolved.
Show resolved Hide resolved
}

.jp-tocItem-content[data-running='0'],
.jp-tocItem-content[data-running='1'] {
.jp-tocItem-content[data-running='1'],
.jp-tocItem-content[data-running='-1'] {
margin-right: 12px;
}
Loading