This is an automated email from the ASF dual-hosted git repository.
tbonelee pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 386d110c08 [ZEPPELIN-6544] Harden keyboard-shortcut e2e helpers
against silent passes
386d110c08 is described below
commit 386d110c089fb26bbbe6d61b30dd395c6c181037
Author: YONGJAE LEE (이용재) <[email protected]>
AuthorDate: Sun Jul 19 02:24:53 2026 +0900
[ZEPPELIN-6544] Harden keyboard-shortcut e2e helpers against silent passes
### What is this PR for?
Follow-up to ZEPPELIN-6536 (#5304). A cross-review of the merged
keyboard-shortcut suite found four silent-pass risks in the helpers; this PR
closes them:
- `pressShortcutFromHostUntil` now polls for the press's effect before
retrying, so a slow toggle is not double-pressed and the loop cannot return on
the transient state between two in-flight effects.
- The clear-output test asserts the result is visible before pressing;
previously a no-output run let the helper skip the press entirely.
- InsertAbove/InsertBelow emptiness assertions now gate on the editor being
rendered (`waitForEditorRendered`); an unrendered editor reads as `''` and
matched vacuously.
- `setCodeEditorContent` compares normalized equality instead of
containment, so stale fixture text fails at seeding time.
Test-only change; no product code touched.
### What type of PR is it?
Improvement
### What is the Jira issue?
ZEPPELIN-6544
### How should this be tested?
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5315 from voidmatcha/fix/harden-keyboard-e2e-helpers.
Signed-off-by: ChanHo Lee <[email protected]>
---
.../e2e/models/notebook-keyboard-page.ts | 138 ++++++++++++---------
.../keyboard/notebook-keyboard-shortcuts.spec.ts | 61 +++++----
2 files changed, 121 insertions(+), 78 deletions(-)
diff --git a/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
b/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
index 44e5c3118c..fb4a24a3a6 100644
--- a/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
+++ b/zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts
@@ -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';
@@ -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 });
@@ -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> {
@@ -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
diff --git
a/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
b/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
index 2f43bb6f33..6f3029fa51 100644
---
a/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
+++
b/zeppelin-web-angular/e2e/tests/notebook/keyboard/notebook-keyboard-shortcuts.spec.ts
@@ -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).
@@ -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+)?$/);
});
});
@@ -354,7 +358,10 @@ 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();
@@ -362,8 +369,8 @@ test.describe.serial('Comprehensive Keyboard Shortcuts
(ShortcutsMap)', () => {
// 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);
});
});
@@ -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);
@@ -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');
});
});
@@ -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);
@@ -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');
});
});
@@ -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(),
@@ -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();
@@ -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 () => {
