tbonelee commented on code in PR #5131:
URL: https://github.com/apache/zeppelin/pull/5131#discussion_r2659528990


##########
zeppelin-web-angular/e2e/models/base-page.ts:
##########
@@ -63,4 +83,85 @@ export class BasePage {
   async getElementText(locator: Locator): Promise<string> {
     return (await locator.textContent()) || '';
   }
+
+  async clickFirstVisibleElement(
+    selectors: string[],
+    options?: {
+      parent?: Locator;
+      visibilityTimeout?: number;
+      clickTimeout?: number;
+    }
+  ): Promise<boolean> {
+    const { parent, visibilityTimeout = 2000, clickTimeout = 5000 } = options 
|| {};
+    const baseLocator = parent || this.page;
+
+    for (const selector of selectors) {
+      try {
+        const element = baseLocator.locator(selector);
+        if (await element.isVisible({ timeout: visibilityTimeout })) {
+          await element.click({ timeout: clickTimeout });
+          return true;
+        }
+      } catch (e) {
+        continue;
+      }
+    }
+    return false;
+  }

Review Comment:
   We could remove this if it isn't used anywhere.



##########
zeppelin-web-angular/e2e/tests/theme/dark-mode.spec.ts:
##########
@@ -11,124 +11,118 @@
  */
 
 import { expect, test } from '@playwright/test';
-import { ThemePage } from '../../models/theme.page';
+import { DarkModePage } from '../../models/dark-mode-page';
 import { addPageAnnotationBeforeEach, performLoginIfRequired, 
waitForZeppelinReady, PAGES } from '../../utils';
 
 test.describe('Dark Mode Theme Switching', () => {
   addPageAnnotationBeforeEach(PAGES.SHARE.THEME_TOGGLE);
-  let themePage: ThemePage;
+  let darkModePage: DarkModePage;
 
   test.beforeEach(async ({ page }) => {
-    themePage = new ThemePage(page);
-    await page.goto('/');
+    darkModePage = new DarkModePage(page);
+    await page.goto('/#/');
     await waitForZeppelinReady(page);
 
     // Handle authentication if shiro.ini exists
     await performLoginIfRequired(page);
 
     // Ensure a clean localStorage for each test
-    await themePage.clearLocalStorage();
+    await darkModePage.clearLocalStorage();
   });
 
-  test('Scenario: User can switch to dark mode and persistence is maintained', 
async ({ page, context }) => {
-    let currentPage = page;
-
+  test('Scenario: User can switch to dark mode and persistence is maintained', 
async ({ page, browserName }) => {
     // GIVEN: User is on the main page, which starts in 'system' mode by 
default (localStorage cleared).
     await test.step('GIVEN the page starts in system mode', async () => {
-      await themePage.assertSystemTheme(); // Robot icon for system theme
+      await darkModePage.assertSystemTheme(); // Robot icon for system theme
     });
 
     // WHEN: Explicitly set theme to light mode for the rest of the test.
     await test.step('WHEN the user explicitly sets theme to light mode', async 
() => {
-      await themePage.setThemeInLocalStorage('light');
-      await page.reload();
+      await darkModePage.setThemeInLocalStorage('light');
+      await page.waitForTimeout(500);
+      if (browserName === 'webkit') {
+        const currentUrl = page.url();
+        await page.goto(currentUrl, { waitUntil: 'load' });
+      } else {
+        page.reload();
+      }
       await waitForZeppelinReady(page);
-      await themePage.assertLightTheme(); // Now it should be light mode with 
sun icon
+      await darkModePage.assertLightTheme(); // Now it should be light mode 
with sun icon
     });
 
     // WHEN: User switches to dark mode by setting localStorage and reloading.
-    await test.step('WHEN the user switches to dark mode', async () => {
-      await themePage.setThemeInLocalStorage('dark');
-      const newPage = await context.newPage();
-      await newPage.goto(currentPage.url());
-      await waitForZeppelinReady(newPage);
-
-      // Update themePage to use newPage and verify dark mode
-      themePage = new ThemePage(newPage);
-      currentPage = newPage;
-      await themePage.assertDarkTheme();
-    });
-
-    // AND: User refreshes the page.
-    await test.step('AND the user refreshes the page', async () => {
-      await currentPage.reload();
-      await waitForZeppelinReady(currentPage);
-    });
-
-    // THEN: Dark mode is maintained after refresh.
-    await test.step('THEN dark mode is maintained after refresh', async () => {
-      await themePage.assertDarkTheme();
+    await test.step('WHEN the user explicitly sets theme to dark mode', async 
() => {
+      await darkModePage.setThemeInLocalStorage('dark');
+      await page.waitForTimeout(500);
+      if (browserName === 'webkit') {
+        const currentUrl = page.url();
+        await page.goto(currentUrl, { waitUntil: 'load' });
+      } else {
+        page.reload();
+      }

Review Comment:
   Could you add some comments about this if-else branch?



##########
zeppelin-web-angular/e2e/models/published-paragraph-page.ts:
##########
@@ -11,58 +11,45 @@
  */
 
 import { Locator, Page } from '@playwright/test';
+import { navigateToNotebookWithFallback } from '../utils';
 import { BasePage } from './base-page';
 
 export class PublishedParagraphPage extends BasePage {
-  readonly publishedParagraphContainer: Locator;
-  readonly dynamicForms: Locator;
   readonly paragraphResult: Locator;
-  readonly errorModal: Locator;
-  readonly errorModalTitle: Locator;
   readonly errorModalContent: Locator;
   readonly errorModalOkButton: Locator;
   readonly confirmationModal: Locator;
-  readonly modalTitle: Locator;
-  readonly runButton: Locator;
 
   constructor(page: Page) {
     super(page);
-    this.publishedParagraphContainer = 
page.locator('zeppelin-publish-paragraph');
-    this.dynamicForms = 
page.locator('zeppelin-notebook-paragraph-dynamic-forms');
     this.paragraphResult = page.locator('zeppelin-notebook-paragraph-result');
-    this.errorModal = page.locator('.ant-modal').last();
-    this.errorModalTitle = page.locator('.ant-modal-title');
     this.errorModalContent = this.page.locator('.ant-modal-body', { hasText: 
'Paragraph Not Found' }).last();
     this.errorModalOkButton = page.getByRole('button', { name: 'OK' }).last();
     this.confirmationModal = page.locator('div.ant-modal-confirm').last();
-    this.modalTitle = 
this.confirmationModal.locator('.ant-modal-confirm-title');
-    this.runButton = this.confirmationModal.locator('button', { hasText: 'Run' 
});
   }
 
   async navigateToNotebook(noteId: string): Promise<void> {
-    await this.page.goto(`/#/notebook/${noteId}`);
-    await this.waitForPageLoad();
+    await navigateToNotebookWithFallback(this.page, noteId);
   }
 
   async navigateToPublishedParagraph(noteId: string, paragraphId: string): 
Promise<void> {
-    await this.page.goto(`/#/notebook/${noteId}/paragraph/${paragraphId}`);
-    await this.waitForPageLoad();
+    await this.navigateToRoute(`/notebook/${noteId}/paragraph/${paragraphId}`);
   }
 
   async getErrorModalContent(): Promise<string> {
-    return (await this.errorModalContent.textContent()) || '';
+    return await this.getElementText(this.errorModalContent);
   }
 
   async clickErrorModalOk(): Promise<void> {
-    await this.errorModalOkButton.click();
-  }
-
-  async getCurrentUrl(): Promise<string> {
-    return this.page.url();
+    await this.errorModalOkButton.click({ timeout: 15000 });
   }
 
   async isOnHomePage(): Promise<boolean> {
-    const url = await this.getCurrentUrl();
-    return url.includes('/#/') && !url.includes('/notebook/');
+    try {
+      await this.welcomeTitle.waitFor({ state: 'visible', timeout: 5000 });
+      return true;
+    } catch (e) {
+      return false;
+    }

Review Comment:
   This method is not being used.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to