-
Notifications
You must be signed in to change notification settings - Fork 3.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
[wdspec] change test_..._closes_browsing_context #49765
base: master
Are you sure you want to change the base?
[wdspec] change test_..._closes_browsing_context #49765
Conversation
@whimboo it looks like this functionality does not work properly on ChromeDriver Classic, so I cannot verify if the test actually works. Could you please confirm it works for Firefox? |
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.
Sorry for the delay. I commented yesterday but forgot to actually submit the comments.
6503c38
to
2ff3fb2
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.
LGTM. Just a small nit. Could you check why the Chrome stability job is failing?
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.
Actually the tests for Firefox are failing because the input
element cannot be located. We should actually wait until the page in the new window is loaded, or the element is present.
@sadym-chromium actually something is wrong with your implementation of https://w3c.github.io/webdriver/#new-window. When a new tab gets opened by chromedriver I can see |
I assume this is not related to this tests, as we don't use "POST /session/{session id}/window/new" in the test, but open a new window via web API |
I filed a related ChromeDriver issue: https://crbug.com/390100762 |
Hm, this is not what I'm referring to. The test actually fails the assertion in Chrome because the window is not closed. Please run |
@sadym-chromium indeed, I think if navigation happens from about:blank, the URL navigated to should replace https://html.spec.whatwg.org/#the-navigation-must-be-a-replace the about:blank, thus making it script-closable due to the history entry size staying 1. |
Classic tests. Open context with `window.open` to allow it to be closed by script.
6860a93
to
d4a1035
Compare
@@ -24,10 +24,17 @@ def test_no_browsing_context(session, closed_frame, key_chain): | |||
def test_key_down_closes_browsing_context( | |||
session, configuration, http_new_tab, inline, key_chain |
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.
Now that a window gets opened by the script we do no longer need the http_new_tab
fixture here and in all the other tests.
session, configuration, http_new_tab, inline, key_chain | |
session, configuration, inline, key_chain |
new_window = session.execute_script(f"return window.open('{url}')") | ||
|
||
# Switch to the new window. | ||
session.window_handle = new_window.id |
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 is still failing Firefox tests because none of the tests are waiting for the navigation to be completed. We need to wait for the input
element to have the focus before we can start performing the action.
Classic tests. Open context with
window.open
to allow it to be closed by script.