[ZEPPELIN-6358] Add E2E tests about /#/notebook/:noteid for New UI #5101

dididy · 2025-10-11T14:47:24Z

What is this PR for?

Addition and improvement of notebook-related E2E tests for New UI

/#/notebook/:noteId – View or edit a specific notebook
/#/notebook/:noteId/revision/:revisionId – View a specific revision of a notebook
/#/notebook/:noteId/paragraph/:paragraphId – Notebook paragraph presentation mode

PAGES.WORKSPACE.NOTEBOOK
→ src/app/pages/workspace/notebook/notebook.component

PAGES.WORKSPACE.NOTEBOOK_ACTION_BAR
→ src/app/pages/workspace/notebook/action-bar/action-bar.component

PAGES.WORKSPACE.NOTEBOOK_PARAGRAPH
→ src/app/pages/workspace/notebook/paragraph/paragraph.component

PAGES.WORKSPACE.NOTEBOOK_SIDEBAR
→ src/app/pages/workspace/notebook/sidebar/sidebar.component

PAGES.WORKSPACE.PUBLISHED_PARAGRAPH
→ src/app/pages/workspace/published/paragraph/paragraph.component

What type of PR is it?

Improvement

Todos

What is the Jira issue?

ZEPPELIN-6358

How should this be tested?

Screenshots (if appropriate)

Questions:

Does the license files need to update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

tbonelee · 2025-10-26T08:17:30Z

Could you rebase this onto master branch?

tbonelee

Could you take a quick pass on two areas?

There are places where try/catch accepts both error and non-error paths. Could we make these more explicit, either intentionally silencing with a clear rationale or failing, so we reduce false positives?
For browser-specific branches (e.g., notebook-keyboard-page.ts → executePlatformShortcut), could we extract that logic so the main flow reads sequentially and the differences are isolated?

tbonelee · 2025-10-24T12:54:52Z

zeppelin-web-angular/e2e/models/notebook-action-bar-page.util.ts

+  async verifyCodeVisibilityToggle(): Promise<void> {
+    await expect(this.actionBarPage.showHideCodeButton).toBeVisible();
+    await expect(this.actionBarPage.showHideCodeButton).toBeEnabled();
+
+    await this.actionBarPage.toggleCodeVisibility();
+
+    // Verify the button is still functional after click
+    await expect(this.actionBarPage.showHideCodeButton).toBeEnabled();
+  }
+
+  async verifyOutputVisibilityToggle(): Promise<void> {
+    await expect(this.actionBarPage.showHideOutputButton).toBeVisible();
+    await expect(this.actionBarPage.showHideOutputButton).toBeEnabled();
+
+    await this.actionBarPage.toggleOutputVisibility();
+
+    // Verify the button is still functional after click
+    await expect(this.actionBarPage.showHideOutputButton).toBeEnabled();
+  }


How about verifying the icon change as well? I'm not sure this is reliable, nz-button seems to set the data-icon attribute on the svg to the nzType value. We could assert that attribute and confirm it updates when the button is clicked.

a1d9c7a
As you suggested, I modified it to capture the data-icon attribute of the SVG inside the <i> tag and compare its value before and after the click.

tbonelee · 2025-10-24T13:00:00Z

zeppelin-web-angular/e2e/models/notebook-keyboard-page.ts

+
+  async navigateToNotebook(noteId: string): Promise<void> {
+    if (!noteId) {
+      console.error('noteId is undefined or null. Cannot navigate to notebook.');


It seems better to throw an error right away when noteId is falsy, since that implies an earlier-step issue, so we don't end up silencing it. WDYT?

36977fb
Updated to handle it by throwing an Error.

tbonelee · 2025-10-24T13:50:34Z

zeppelin-web-angular/e2e/global-teardown.ts

+  await cleanupTestNotebooks();
+}
+
+async function cleanupTestNotebooks() {


We need authorizations to use Rest APIs. How about using test notebook directory?

zeppelin/zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java

Line 982 in 0a768bc

ZEPPELIN_NOTEBOOK_DIR("zeppelin.notebook.dir", "notebook"),

Maybe we could set ZEPPELIN_NOTEBOOK_DIR environment variable, and let e2e script to use this to clean up test notebooks.

It seems to me that this approach would require restarting the server. Could you confirm if I understood that correctly? If so, it might feel a bit awkward. In the meantime, I’ll check whether a method that doesn’t rely on a REST API—such as using sockets—would be feasible.

tbonelee · 2025-10-26T08:26:46Z

zeppelin-web-angular/e2e/models/notebook-action-bar-page.util.ts

+  async verifyTitleEditingFunctionality(expectedTitle?: string): Promise<void> {
+    await expect(this.actionBarPage.titleEditor).toBeVisible();
+    const titleText = await this.actionBarPage.getTitleText();
+    expect(titleText).toBeDefined();
+    expect(titleText.length).toBeGreaterThan(0);
+
+    if (expectedTitle) {
+      expect(titleText).toContain(expectedTitle);
+    }
+  }


If we're keeping this method name, the test should assert the editing behavior, not just element presence. Otherwise, consider renaming it to reflect what it currently does.

606ae73
Refactored verifyTitleEditingFunctionality so that it now asserts the actual title editing behavior instead of only checking for element visibility. This makes the method behavior consistent with its name.

tbonelee · 2025-10-26T08:30:04Z

zeppelin-web-angular/e2e/models/notebook-action-bar-page.util.ts

+    try {
+      // Try multiple possible confirmation dialog selectors
+      const confirmSelector = this.page
+        .locator('nz-popconfirm button:has-text("OK"), .ant-popconfirm button:has-text("OK"), button:has-text("OK")')
+        .first();
+      await expect(confirmSelector).toBeVisible({ timeout: 2000 });
+      await confirmSelector.click();
+      await expect(confirmSelector).not.toBeVisible();
+    } catch (error) {
+      // If no confirmation dialog appears, that's also valid behavior
+      console.log('Run all executed without confirmation dialog');
+    }


Rather than catching and continuing, could we refactor (e.g., split methods) so that cases which should fail actually fail? Same note for the other similar try/catch blocks.

e92d5ae
I’ve removed the try/catch blocks from tests that should fail instead of continuing. Please let me know if I missed any cases.

tbonelee · 2025-10-26T08:30:45Z

zeppelin-web-angular/e2e/models/notebook-action-bar-page.util.ts

+    try {
+      // Try multiple possible confirmation dialog selectors
+      const confirmSelector = this.page
+        .locator('nz-popconfirm button:has-text("OK"), .ant-popconfirm button:has-text("OK"), button:has-text("OK")')
+        .first();
+      await expect(confirmSelector).toBeVisible({ timeout: 2000 });
+      await confirmSelector.click();
+      await expect(confirmSelector).not.toBeVisible();
+    } catch (error) {
+      // If no confirmation dialog appears, that's also valid behavior
+      console.log('Clear output executed without confirmation dialog');
+    }


The same comment as the above one.

dididy · 2025-10-27T16:53:40Z

You can see that request item 1 has been addressed in e92d5ae and item 2 in eea335a. Thank you for the thorough review.

dididy force-pushed the e2e/notebook branch 8 times, most recently from d6228ac to b07b933 Compare October 19, 2025 05:34

dididy force-pushed the e2e/notebook branch from b81dda2 to ceb2140 Compare October 24, 2025 11:50

dididy mentioned this pull request Oct 25, 2025

[ZEPPELIN-6349] Bump Angular from 9 to 13 #5109

Merged

5 tasks

tbonelee requested changes Oct 26, 2025

View reviewed changes

dididy added 10 commits October 26, 2025 18:50

Add notebook related tests

833f1dd

add retry for local test

7821362

combine tests

ad3df86

add shortcut tests

c3c06cf

fix broken tests

65b8bd3

remove unsued function and apply review

5364c36

add global teardown's cleanup test notebooks

6e5f900

cleanupTestNotebooks only for local, add waitForSelector

4875639

enhance tests

8f9cb78

apply lint:fix

ab09089

dididy force-pushed the e2e/notebook branch 3 times, most recently from d244721 to 3aaaed0 Compare October 26, 2025 17:14

dididy added 2 commits October 27, 2025 19:11

update playwright.config

f8e076e

fix broken tests

9bd2f40

dididy force-pushed the e2e/notebook branch from d058c16 to 9bd2f40 Compare October 27, 2025 10:13

dididy added 2 commits October 27, 2025 20:27

refactor error handling to fail fast instead of catching and continuing

e92d5ae

fix broken tests

647848b

dididy added 4 commits October 28, 2025 00:55

verify icon updates by asserting svg data-icon attribute

a1d9c7a

make verifyTitleEditingFunctionality assert actual editing behavior

606ae73

lint fix + throw error

36977fb

extract browser-specific logic in executePlatformShortcut for clarity

eea335a

dididy force-pushed the e2e/notebook branch 9 times, most recently from 45186ed to 365f64c Compare October 29, 2025 18:50

fix broken tests

df1d0f5

dididy force-pushed the e2e/notebook branch from 365f64c to df1d0f5 Compare October 30, 2025 00:13

Uh oh!

[ZEPPELIN-6358] Add E2E tests about /#/notebook/:noteid for New UI #5101

Are you sure you want to change the base?

[ZEPPELIN-6358] Add E2E tests about /#/notebook/:noteid for New UI #5101

Conversation

dididy commented Oct 11, 2025

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

tbonelee commented Oct 26, 2025

Uh oh!

tbonelee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dididy commented Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants