-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6358] simplify utils, promote POM usage, and consolidate base logic from #5101 #5131
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
base: master
Are you sure you want to change the base?
Conversation
| 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; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove this if it isn't used anywhere.
| try { | ||
| await this.welcomeTitle.waitFor({ state: 'visible', timeout: 5000 }); | ||
| return true; | ||
| } catch (e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not being used.
| if (browserName === 'webkit') { | ||
| const currentUrl = page.url(); | ||
| await page.goto(currentUrl, { waitUntil: 'load' }); | ||
| } else { | ||
| page.reload(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some comments about this if-else branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic was originally added as a workaround for an issue that occurred only on WebKit after upgrading Playwright to a newer version. Since it is expected to work correctly on WebKit with the current Playwright version, I removed the conditional handling. I’ll verify the CI results and address any issues if needed. I also cleaned up other unused and unnecessary methods.
What is this PR for?
PR Description
This PR improves the readability and maintainability of the E2E notebook tests.
As a result, the tests are clearer, flatter, and easier to maintain.
What type of PR is it?
Refactoring
Todos
What is the Jira issue?
ZEPPELIN-6358
How should this be tested?
Screenshots (if appropriate)
Questions: