-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
FIX #20226 : Navigation timeout of 30000 ms in click-all-buttons-on-about-foundation-page #20279
Conversation
Hi @Akhilesh-max, can you complete the following:
|
Hi @Akhilesh-max please assign the required reviewer(s) for this PR. Thanks! |
@@ -401,6 +402,9 @@ export class LoggedInUser extends BaseUser { | |||
await this.page.$eval(weCannotContentId, element => | |||
element.getElementsByTagName('a')[0].click() | |||
); |
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 test is intermittently failing due to a race condition between two navigations. After clicking the '420 million' link, the test is not waiting for the resulting navigation to complete before the beforeEach block started a new navigation. This lefts the first navigation unresolved, which sometimes leads to failures.
Despite the test logging the final showMessage, it is not properly handling the navigation it initiated.
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.
Great find, @Akhilesh-max!
@@ -401,6 +402,9 @@ export class LoggedInUser extends BaseUser { | |||
await this.page.$eval(weCannotContentId, element => | |||
element.getElementsByTagName('a')[0].click() | |||
); | |||
await this.page.waitForNavigation({waitUntil: ['load', 'networkidle2']}); | |||
await this.page.waitForSelector(_420millionPageHeaderSelector); | |||
|
|||
if (this.page.url() !== _420MillionUrl) { | |||
throw new Error('The 420 Million link does not open the right page!'); |
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.
I suggest printing the incorrect URL here in this error message (and in other similar error messages) -- it would help with debugging.
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.
Done.
@@ -401,6 +402,9 @@ export class LoggedInUser extends BaseUser { | |||
await this.page.$eval(weCannotContentId, element => | |||
element.getElementsByTagName('a')[0].click() | |||
); | |||
await this.page.waitForNavigation({waitUntil: ['load', 'networkidle2']}); | |||
await this.page.waitForSelector(_420millionPageHeaderSelector); |
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.
Do you actually need this line, or is the previous one alone sufficient?
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.
Previous one alone is good enough to handle this.
@@ -401,6 +402,9 @@ export class LoggedInUser extends BaseUser { | |||
await this.page.$eval(weCannotContentId, element => | |||
element.getElementsByTagName('a')[0].click() | |||
); |
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.
Great find, @Akhilesh-max!
Hi @Akhilesh-max, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue. |
Hi @Akhilesh-max. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
@@ -407,8 +410,13 @@ export class LoggedOutUser extends BaseUser { | |||
await this.page.$eval(weCannotContentId, element => | |||
element.getElementsByTagName('a')[0].click() | |||
); | |||
await this.page.waitForNavigation({waitUntil: ['load', 'networkidle2']}); |
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.
To use this waitForNavigation correctly, make sure to call it while the button is being clicked.
Example:
await Promise.all([this.page.waitForNavigation(), this.clickOn(button)]);
It avoids the navigation timeout error when this.page.waitForNavigation() started after the navigation is fully completed.
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.
Done.
@@ -171,7 +171,8 @@ export class LoggedOutUser extends BaseUser { | |||
|
|||
expect(this.page.url()) | |||
.withContext( | |||
`${buttonName} should open the ${expectedDestinationPageName} page` | |||
`${buttonName} should open the ${expectedDestinationPageName} page, | |||
but it opens ${this.page.url()} instead.` |
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.
Can you also add this printing this.page.url() part to the rest? We want to make our code consistent.
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.
Reverting to the erstwhile one. As expect().toBe() prints a clear message itself.
However, have added consoling of this.page.url() elsewhere we are not using expect().toBe().
Unassigning @imchristie since the review is done. |
@imchristie PTAL |
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.
@Akhilesh-max Once @imchristie approves, I will too. Please reassign me once that happens.
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.
LGTM! @seanlip
Unassigning @imchristie since they have already approved the PR. |
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.
LGTM based on Christie's review (thanks!)
Hi @Akhilesh-max, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
PR Pointers