-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Reload the page to clear the state #77181
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
@karenroldan Ah shoot, I was experimenting with this as well. I think we should try merging yours in first, and then reverting the changes @dpasque made in #75436 to re-expose instances of this failure. What do you think? |
@@ -31,6 +31,8 @@ export class LoginPage { | |||
status: 200, | |||
} ); | |||
} ); | |||
// Reload the page to clear the state. | |||
await this.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.
question:non-blocking - my interpretation of microsoft/playwright#18137 (comment) is that the page reload should come after the login process, but before the navigation to the target URL.
Sorry for the late response! I just saw this comment today. After syncing with @worldomonation, we've decided to close this ticket and carry on our efforts to investigate further and find the root cause of the initial issue. We'll park this change for now! 👍🏻 |
Related to #70674
Proposed Changes
With the addition of reload in the login page, the execution time wasn't significantly impacted. The average execution time remains similar to the average time I observed without the
reload
.Testing Instructions
Pre-merge Checklist