Skip to content
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

Add workaround to retry waitFor until it is success or retry count is 0 #1469

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Oct 8, 2024

What?

We're allowing retries on waitFor when the following errors occur when waitFor is waiting:

  • "Inspected target navigated or closed"
  • "Cannot find context with specified id"
  • "Execution context was destroyed"

To prevent a stack overflow, it will retry for a maximum of 20 times before failing. In my tests i've seen the retry count drop from 20 to 12 at most. I feel 20 is a safe number that shouldn't be reached in normal SSO login flows. I want to avoid using a larger retry count to force us to fix this permanently asap.

Why?

This PR fixes an issue where waitFor fails to wait while the underlying page it is waiting on is navigating. A common scenario where we might see this is during a login/logout flow using SSO. The initial page will be redirected multiple times until we get to the desired page/state.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Updates: #1472

@ankur22 ankur22 changed the title Add a POC to retry waitFor until it is success Add workaround to retry waitFor until it is success or retry count is 0 Oct 10, 2024
@ankur22 ankur22 requested a review from inancgumus October 10, 2024 09:14
@ankur22 ankur22 force-pushed the investigate/waitFor-early-exit branch 3 times, most recently from 62d21fc to 3048af0 Compare October 10, 2024 15:26
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@ankur22 ankur22 force-pushed the investigate/waitFor-early-exit branch from 3048af0 to f51abb3 Compare October 10, 2024 15:33
@ankur22 ankur22 merged commit 75647e9 into main Oct 11, 2024
23 checks passed
@ankur22 ankur22 deleted the investigate/waitFor-early-exit branch October 11, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants