fix(LoginHelper): increase GitHub login timeout and handle GitHub popup re-authorization#105
Conversation
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
…method Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
… changes Signed-off-by: Dominik Augustín <daugusti@redhat.com>
| } | ||
|
|
||
| await this.page.waitForTimeout(3_000); | ||
| await this.page.waitForTimeout(30_000); |
There was a problem hiding this comment.
what exactly are we waiting for here? 30 seconds unconditional wait seems excessive
There was a problem hiding this comment.
if this is just to wait for the github login to finish, make it a conditional wait for some element on the github home page, like the heading
There was a problem hiding this comment.
I believe I have increased this timeout to wait for the GitHub login to finish as the login timed-out on GitHub homepage. Though, I cannot find the Playwright Test Report from my PR that timed-out here; maybe it was during local tests against RHDH in a cluster.
The time it took the tests to return to RHDH from GitHub was quite random, which I assumed, this would cover. Is my understanding correct?
Should the suggested conditional wait be targeting GitHub homepage content or rather RHDH homepage/login screen?
There was a problem hiding this comment.
Should the suggested conditional wait be targeting GitHub homepage content or rather RHDH homepage/login screen?
RHDH login is hanled outside this function, go for the github page
There was a problem hiding this comment.
I have changed it to wait for Home heading
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
| } | ||
|
|
||
| await this.page.waitForTimeout(3_000); | ||
| await this.page.waitForSelector('h2:has-text("Home")', { |
There was a problem hiding this comment.
using waitForSelector is discouraged by playwright
I'd suggest using a user-facing locator like page.getByRole("heading", { name: "text" })
and waitFor
There was a problem hiding this comment.
Oh, I missed that one. I have fixed that.
In fact, the waitForSelector warning does not appear for me in this rhdh-e2e-test-utils repo, but in rhdh-plugin-export-overlays it does. What can cause the different behavior?
Signed-off-by: Dominik Augustín <daugusti@redhat.com>
This pull request improves the reliability and robustness of the GitHub login automation in the Playwright test helper by increasing timeouts and refactoring the GitHub reauthorization flow. The main enhancements include handling both automatic logins and popup-based reauthorization, increasing timeouts to accommodate slower responses, and extracting popup handling logic for clarity and reuse.
Login flow robustness and reauthorization handling:
LoginHelper.loginAsGithubUserto wait for either the sidebar (auto-login) or a popup (reauthorization required) after clicking "Sign In," and to handle the popup if it appears. This improves resilience against session staleness and unexpected authentication states.handleGithubPopupReauthmethod, improving code clarity and reusability.Timeout adjustments for reliability:
Assisted-By: Claude Code