Skip to content

[ZEPPELIN-6544] Harden keyboard-shortcut e2e helpers against silent passes #5315

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.

Sign up for GitHub

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

Open
voidmatcha wants to merge 1 commit into
base: master
Choose a base branch
from
+121 −78
Open

[ZEPPELIN-6544] Harden keyboard-shortcut e2e helpers against silent passes #5315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
138 changes: 81 additions & 57 deletions zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
* limitations under the License.
*/

import test, { expect, Locator, Page } from '@playwright/test';
import { expect, Locator, Page } from '@playwright/test';
import { navigateToNotebookWithFallback } from '../utils';
import { ShortcutsMap } from '../../src/app/key-binding/shortcuts-map';
import { ParagraphActions } from '../../src/app/key-binding/paragraph-actions';
Expand Down Expand Up @@ -123,6 +123,8 @@ export class NotebookKeyboardPage extends BasePage {
if (!(await isSettled())) {
await this.focusParagraphHost(paragraphIndex);
await press();
// Wait for this press's effect before retrying — an immediate recheck would double-press a slow toggle. The wait must comfortably exceed legitimate settle latency so only a genuinely dropped press is retried.
await expect.poll(isSettled, { timeout: 10000 }).toBe(true);
}
expect(await isSettled()).toBe(true);
}).toPass({ timeout: 15000 });
Expand Down Expand Up @@ -369,27 +371,30 @@ export class NotebookKeyboardPage extends BasePage {
return this.readEditorText(this.paragraphContainer.first());
}

// Gate for emptiness assertions — an empty Monaco model still renders one (empty) .view-line.
async waitForEditorRendered(paragraphIndex: number): Promise<void> {
const paragraph = this.getParagraphByIndex(paragraphIndex);
await expect(paragraph.locator('.monaco-editor .view-line').first()).toBeAttached({ timeout: 10000 });
}

// Reconstruct editor text from Monaco's absolutely-positioned `.view-line` divs sorted by top (DOM order need not match line order), via textContent; innerText is "" for off-layout lines in headless Chromium.
// Constraints: Monaco virtualizes lines (keep fixtures short); returns '' for both an empty and a not-yet-rendered editor.
private async readEditorText(paragraph: Locator): Promise<string> {
const monaco = paragraph.locator('.monaco-editor').first();
if ((await monaco.count()) > 0) {
const text = await monaco.evaluate((el: Element) => {
const lines = Array.from(el.querySelectorAll('.view-line')) as HTMLElement[];
lines.sort((a, b) => parseInt(a.style.top || '0', 10) - parseInt(b.style.top || '0', 10));
return lines.map(l => (l.textContent || '').replace(/\u00a0/g, ' ')).join('\n');
});
if (text.trim().length > 0) {
return text;
}
if ((await monaco.count()) === 0) {
return '';
}
const textarea = paragraph.locator('.monaco-editor textarea').first();
if ((await textarea.count()) > 0) {
const value = await textarea.inputValue().catch(() => '');
if (value) {
return value;
}
}
return '';
// Reconstruct from view-lines only. The hidden textarea holds Monaco's IME buffer
// (a fragment, not the full model), so falling back to it returns partial text and
// makes reads non-deterministic. Callers poll, so '' while lines render is fine.
return monaco.evaluate((el: Element) => {
const lines = Array.from(el.querySelectorAll('.view-line')) as HTMLElement[];
lines.sort((a, b) => parseInt(a.style.top || '0', 10) - parseInt(b.style.top || '0', 10));
// Normalize all Unicode space separators (NBSP etc. that Monaco renders for
// whitespace) to a plain space, but keep line structure so exact-match assertions
// still verify it.
return lines.map(l => (l.textContent || '').replace(/\p{Zs}/gu, ' ')).join('\n');
});
}

async setCodeEditorContent(content: string, paragraphIndex: number = 0): Promise<void> {
Expand All @@ -398,55 +403,74 @@ export class NotebookKeyboardPage extends BasePage {
return;
}

await this.tryFocusCodeEditor(paragraphIndex);
if (this.page.isClosed()) {
console.warn('Cannot set code editor content: page closed after focusing');
return;
}
// Seed via REST, not the editor: per-keystroke seeding fights the collaborative
// patch loop, merging a fresh note's "%python" prefix into the content on Firefox.
await this.seedParagraphViaRest(paragraphIndex, content);

const paragraph = this.getParagraphByIndex(paragraphIndex);
const editorInput = paragraph.locator('.monaco-editor .inputarea, .monaco-editor textarea').first();
await this.tryFocusCodeEditor(paragraphIndex);

const browserName = test.info().project.name;
if (browserName !== 'firefox') {
await editorInput.waitFor({ state: 'visible', timeout: 30000 });
await editorInput.click();
await editorInput.clear();
// The REST flush resets the cursor to (1,1); restore end-of-content so tests that
// seed then press End/Enter start from the right line. Keymap-independent.
if (content) {
await this.pressSelectAll();
await this.page.keyboard.press('ArrowRight');
}
}

// Clear existing content with keyboard shortcuts for better reliability
await editorInput.focus();
private noteIdFromUrl(): string {
const match = /\/notebook\/([^/?#]+)/.exec(this.page.url());
if (!match) {
throw new Error(`No noteId in URL ${this.page.url()}`);
}
return match[1];
}

if (browserName === 'firefox') {
// Clear by backspacing existing content length
const currentContent = await editorInput.inputValue();
const contentLength = currentContent.length;
// Read a paragraph's persisted text from the server. Use this instead of the editor
// DOM when asserting content equality: Monaco reconstructs the same text with
// different whitespace codepoints across reads, which breaks byte-exact matches.
async getParagraphTextByIndex(paragraphIndex: number): Promise<string> {
const noteId = this.noteIdFromUrl();
const response = await this.page.request.get(`/api/notebook/${noteId}`, { failOnStatusCode: false });
if (!response.ok()) {
throw new Error(`Fetch notebook REST request failed: ${response.status()} ${await response.text()}`);
}
const json = (await response.json()) as { body?: { paragraphs?: Array<{ text?: string }> } };
return json.body?.paragraphs?.[paragraphIndex]?.text ?? '';
}

// Position cursor at end and backspace all content
await this.page.keyboard.press('End');
for (let i = 0; i < contentLength; i++) {
await this.page.keyboard.press('Backspace');
}
private async seedParagraphViaRest(paragraphIndex: number, content: string): Promise<void> {
const noteId = this.noteIdFromUrl();
const paragraph = this.getParagraphByIndex(paragraphIndex);

// JUSTIFIED: Monaco textarea can be covered by editor overlays during fixture setup.
await editorInput.fill(content, { force: true });
} else {
// Standard clearing for other browsers
await this.pressSelectAll();
await this.page.keyboard.press('Delete');
// JUSTIFIED: Monaco textarea can be overlaid after select+delete during fixture setup.
await editorInput.fill(content, { force: true });
const paragraphId = await this.resolveParagraphId(noteId, paragraphIndex);
const response = await this.page.request.put(`/api/notebook/${noteId}/paragraph/${paragraphId}`, {
data: { text: content },
failOnStatusCode: false
});
if (!response.ok()) {
throw new Error(`Seed paragraph REST request failed: ${response.status()} ${await response.text()}`);
}

// Wait for the full normalized editor content to avoid stale Monaco renders.
// The PUT broadcasts the paragraph; wait for the flush to render the exact content.
const expected = content.replace(/\s+/g, '');
if (expected.length === 0) {
await expect.poll(async () => (await this.readEditorText(paragraph)).trim(), { timeout: 10000 }).toBe('');
} else {
await expect
.poll(async () => (await this.readEditorText(paragraph)).replace(/\s+/g, ''), { timeout: 10000 })
.toContain(expected);
}
await expect
.poll(async () => (await this.readEditorText(paragraph)).replace(/\s+/g, ''), { timeout: 15000 })
.toBe(expected);
}

private async resolveParagraphId(noteId: string, paragraphIndex: number): Promise<string> {
// Retry: a paragraph just inserted through the UI may not be persisted server-side yet.
let id: string | undefined;
await expect(async () => {
const response = await this.page.request.get(`/api/notebook/${noteId}`, { failOnStatusCode: false });
if (!response.ok()) {
throw new Error(`Fetch notebook REST request failed: ${response.status()} ${await response.text()}`);
}
const json = (await response.json()) as { body?: { paragraphs?: Array<{ id?: string }> } };
id = json.body?.paragraphs?.[paragraphIndex]?.id;
expect(id, `No paragraph at index ${paragraphIndex} in note ${noteId}`).toBeTruthy();
}).toPass({ timeout: 10000, intervals: [200, 400, 800] });
return id!;
}

// Helper methods for verifying shortcut effects
Expand Down
61 changes: 40 additions & 21 deletions zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const finalCount = await keyboardPage.getParagraphCount();
expect(finalCount).toBe(initialCount + 1);

// And: the new paragraph at index 0 holds no user content; empty or just an interpreter directive (poll so the async insert/render settles).
// And: the new paragraph at index 0 holds no user content; empty or just an interpreter directive.
// Render gate: an unrendered editor reads as '' and would vacuously match.
await keyboardPage.waitForEditorRendered(0);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(0).then(c => c.trim())).toMatch(/^(%\w+)?$/);

// And the original content moved to index 1 (normalize whitespace; Monaco reflows).
Expand Down Expand Up @@ -343,6 +345,8 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
expect(originalParagraphContent).toMatch(/Content\s+for\s+insert\s+below\s+test/);

// And: a new paragraph exists at index 1 holding no user content.
// Render gate: an unrendered editor reads as '' and would vacuously match.
await keyboardPage.waitForEditorRendered(1);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(1).then(c => c.trim())).toMatch(/^(%\w+)?$/);
});
});
Expand All @@ -354,16 +358,19 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
await keyboardPage.setCodeEditorContent('%md\n# Copy Test\nContent to be copied below');

const initialCount = await keyboardPage.getParagraphCount();
const originalContent = await keyboardPage.getCodeEditorContentByIndex(0);
// Compare persisted server text, not the editor DOM: the clone's correctness is
// that the copied paragraph is stored identically, and reading from the server
// avoids Monaco's non-deterministic whitespace rendering.
const originalContent = await keyboardPage.getParagraphTextByIndex(0);

// When: User presses Control+Shift+C
await keyboardPage.pressInsertCopy();

// Then: a copy is inserted below carrying the same text, and the original is unchanged
await keyboardPage.waitForParagraphCountChange(initialCount + 1);
expect(await keyboardPage.getParagraphCount()).toBe(initialCount + 1);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(0)).toBe(originalContent);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(1)).toBe(originalContent);
await expect.poll(() => keyboardPage.getParagraphTextByIndex(0)).toBe(originalContent);
await expect.poll(() => keyboardPage.getParagraphTextByIndex(1)).toBe(originalContent);
});
});

Expand All @@ -389,9 +396,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const paragraphCount = await keyboardPage.getParagraphCount();
expect(paragraphCount).toBe(2);

// Verify initial content before move
const initialFirst = await keyboardPage.getCodeEditorContentByIndex(0);
const initialSecond = await keyboardPage.getCodeEditorContentByIndex(1);
// Capture server-persisted text (avoids Monaco DOM whitespace non-determinism)
const initialFirst = await keyboardPage.getParagraphTextByIndex(0);
const initialSecond = await keyboardPage.getParagraphTextByIndex(1);

// Focus on second paragraph for move operation
await keyboardPage.tryFocusCodeEditor(1);
Expand All @@ -403,9 +410,13 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const finalParagraphCount = await keyboardPage.getParagraphCount();
expect(finalParagraphCount).toBe(2);

// And: Paragraph positions should be swapped (poll until the move lands in the DOM)
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(0)).toBe(initialSecond);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(1)).toBe(initialFirst);
// And: positions are swapped (poll the server until the move persists)
await expect.poll(() => keyboardPage.getParagraphTextByIndex(0)).toBe(initialSecond);
await expect.poll(() => keyboardPage.getParagraphTextByIndex(1)).toBe(initialFirst);
// And the visible order reflects the swap. Containment on a distinctive marker
// proves the user-visible reorder without an exact Monaco whitespace match.
await expect(keyboardPage.getParagraphByIndex(0).locator('.view-lines')).toContainText('Second Paragraph');
await expect(keyboardPage.getParagraphByIndex(1).locator('.view-lines')).toContainText('First Paragraph');
});
});

Expand All @@ -431,9 +442,9 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const paragraphCount = await keyboardPage.getParagraphCount();
expect(paragraphCount).toBe(2);

// Verify initial content before move
const initialFirst = await keyboardPage.getCodeEditorContentByIndex(0);
const initialSecond = await keyboardPage.getCodeEditorContentByIndex(1);
// Capture server-persisted text (avoids Monaco DOM whitespace non-determinism)
const initialFirst = await keyboardPage.getParagraphTextByIndex(0);
const initialSecond = await keyboardPage.getParagraphTextByIndex(1);

// Focus first paragraph for move operation
await keyboardPage.tryFocusCodeEditor(0);
Expand All @@ -445,9 +456,13 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const finalParagraphCount = await keyboardPage.getParagraphCount();
expect(finalParagraphCount).toBe(2);

// And: Paragraph positions should be swapped (poll until the move lands in the DOM)
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(0)).toBe(initialSecond);
await expect.poll(() => keyboardPage.getCodeEditorContentByIndex(1)).toBe(initialFirst);
// And: positions are swapped (poll the server until the move persists)
await expect.poll(() => keyboardPage.getParagraphTextByIndex(0)).toBe(initialSecond);
await expect.poll(() => keyboardPage.getParagraphTextByIndex(1)).toBe(initialFirst);
// And the visible order reflects the swap. Containment on a distinctive marker
// proves the user-visible reorder without an exact Monaco whitespace match.
await expect(keyboardPage.getParagraphByIndex(0).locator('.view-lines')).toContainText('Second Paragraph');
await expect(keyboardPage.getParagraphByIndex(1).locator('.view-lines')).toContainText('First Paragraph');
});
});

Expand Down Expand Up @@ -559,8 +574,11 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
const statusElBefore = keyboardPage.paragraphContainer.first().locator('.status');
await expect(statusElBefore).toHaveText(/FINISHED|ERROR|PENDING|RUNNING/);

// When: User presses Control+Alt+L (editor hidden after %md run; dispatch from the host)
// Gate: without visible output, isSettled starts true and the helper would skip the press entirely.
const resultLocator = keyboardPage.getParagraphByIndex(0).locator('[data-testid="paragraph-result"]');
await expect(resultLocator).toBeVisible();

// When: User presses Control+Alt+L (editor hidden after %md run; dispatch from the host)
await keyboardPage.pressShortcutFromHostUntil(
0,
() => keyboardPage.pressClearOutput(),
Expand Down Expand Up @@ -932,7 +950,7 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
// Verify error result exists (invalid syntax produces a final ERROR or FINISHED with error output)
// JUSTIFIED: single-paragraph test notebook; first() is deterministic
const statusElError = keyboardPage.paragraphContainer.first().locator('.status');
await expect(statusElError).toHaveText(/FINISHED|ERROR/, { timeout: 30000 });
await expect(statusElError).toHaveText(/FINISHED|ERROR/, { timeout: 60000 });

// When: User continues with shortcuts (insert new paragraph)
const initialCount = await keyboardPage.getParagraphCount();
Expand All @@ -945,11 +963,12 @@ test.describe.serial('Comprehensive Keyboard Shortcuts (ShortcutsMap)', () => {
await keyboardPage.setCodeEditorContent('%md\n# Recovery Test\nShortcuts work after error', newParagraphIndex);
await keyboardPage.pressRunParagraph();

// Then: Shortcut execution still reaches a terminal state
await keyboardPage.waitForParagraphExecution(newParagraphIndex);
// Then: Shortcut execution still reaches a terminal state (real interpreter run;
// allow extra time as a cold interpreter under CI load can stay RUNNING past 30s)
await keyboardPage.waitForParagraphExecution(newParagraphIndex, 60000);
// JUSTIFIED: newParagraphIndex is dynamically computed from getParagraphCount(); nth() is the only way to address this specific paragraph
const statusElNew = keyboardPage.paragraphContainer.nth(newParagraphIndex).locator('.status');
await expect(statusElNew).toHaveText(/FINISHED|ERROR/, { timeout: 30000 });
await expect(statusElNew).toHaveText(/FINISHED|ERROR/, { timeout: 60000 });
});

test('should gracefully handle shortcuts when no paragraph is focused', async () => {
Expand Down
Loading