-
Notifications
You must be signed in to change notification settings - Fork 31
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: Safari noSuchWindowException and Page/Frame not ready #385
Conversation
@page.evaluate_script <<-JS | ||
document.readyState === 'complete' | ||
JS | ||
wait = Selenium::WebDriver::Wait.new(:timeout => 10) |
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 now checks readyState
repeatedly for a 'complete'
state. Previously, the state would be checked once; given an 'interactive'
state, assert_frame_ready
would fail and never succeed, hence #353.
d3408fd
to
ca52618
Compare
ca52618
to
a1dba66
Compare
a1dba66
to
2469099
Compare
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.
The noSuchWindowException
change is looking good! I had a question on the Page/Frame not ready change inline -
wait = Selenium::WebDriver::Wait.new(:timeout => 10) | ||
wait.until { | ||
readyState = @page.evaluate_script <<-JS | ||
document.readyState |
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 know this has been here for a while, but have you tested if we even need this anymore? I know for Selenium they have implemented the WebDriver as per the spec which by default is set to so we should be waiting for the page finish loading before trying to inject:
An HTTP session has an associated page loading strategy, which is one of the keywords from the table of page load strategies. This is initially set to normal.
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.
If it's for backcompat with older selenium versions that we expect are missing that behavior, it'd be helpful to leave a comment explaining that
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 tested removing this against both latest 3 and 4, and I didn't find any issue against either the "fast" or "slow" URLs in my repro script. I'm removing it for this reason. If I'm misunderstanding, please let me know.
It seems odd that it would work on latest 3, but there may have been a fix in place since this was first written.
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.
👇
QA Notes:
The following code occasionally caused Safari to throw a
noSuchWindowException
around 1 in 10 times, and I expect it never to do so now.Note the commented
# driver.navigate to "http://google.com"
. Issue #353 notes that attempting to navigate tohttp://google.com
would also occasionally cause an exception "Page/Frame not ready" to be thrown; it should no longer do so.Please uncomment this line, comment
driver.navigate.to "https://dequeuniversity.com/demo/mars/"
and test again. This fix is included here since they are closely related.Closes: #352
Closes: #353