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

Make testdriver click/send_keys/actions work in more browsing contexts #25550

Merged
merged 2 commits into from Sep 25, 2020

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Sep 15, 2020

This is a slightly hacky change to allow testdriver actions to work in
more browsing contexts. The basic idea is that when we want to run an
action in a different borsing context, we pass wptrunner a context
list like [window_id, frame_id...], and use that to select the correct
browsing context just for the duration of the action, before switching
back.

There are a number of limitations with the current patch, some more
serious than others:

  • So far this is only implmented for testdriver commands that take an
    explicit element. Other commands could be modified to also allow
    passing in an explicit context.

  • It must be possible to get a window reference and set the name
    property (see below). This means this approach only works for
    windows that are same origin with the test window.

  • WebDriver implementations don't generally support returning a window
    object from script, so we can't return a window handle
    directly. Instead we either use the existing window.name property or
    add a window.name when we try to start the action. Then in wptrunner
    we do a linear search of all windows to find the one with the
    appropriate name. We have to use window.name rather than writing a
    custom property into the window global because the way marionette
    sandboxes js we aren't able to read the global property.

However despite these limitations this makes the feature considerably
more versatile than it was previously.

@wpt-pr-bot wpt-pr-bot added docs infra testdriver.js wptrunner The automated test runner, commonly called through ./wpt run labels Sep 15, 2020
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25550 September 15, 2020 19:48 Inactive
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, I think it's a great improvement to testdriver!

Looked at the non-test files so far.

resources/testdriver.js Outdated Show resolved Hide resolved
tools/wptrunner/wptrunner/executors/base.py Outdated Show resolved Hide resolved
tools/wptrunner/wptrunner/testdriver-extra.js Show resolved Hide resolved
tools/wptrunner/wptrunner/testdriver-extra.js Outdated Show resolved Hide resolved
tools/wptrunner/wptrunner/testdriver-extra.js Outdated Show resolved Hide resolved
tools/wptrunner/wptrunner/testdriver-extra.js Outdated Show resolved Hide resolved
tools/wptrunner/wptrunner/executors/base.py Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25550 September 18, 2020 14:40 Inactive
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

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

Generally LGTM modulo comments, but seems like there are some failures in the unittests (unable to find the target window?) and the lint (mostly trailing-whitespace, one usage of console).

tools/wptrunner/wptrunner/executors/base.py Show resolved Hide resolved
tools/wptrunner/wptrunner/testdriver-extra.js Show resolved Hide resolved
infrastructure/testdriver/click_iframe.html Outdated Show resolved Hide resolved
infrastructure/testdriver/click_nested.html Show resolved Hide resolved
infrastructure/testdriver/click_iframe.html Outdated Show resolved Hide resolved
infrastructure/testdriver/click_iframe.html Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25550 September 23, 2020 20:23 Inactive
Copy link
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

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

LGTM after the infra tests are fixed.

@wpt-pr-bot wpt-pr-bot temporarily deployed to wpt-preview-25550 September 24, 2020 13:20 Inactive
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

infrastructure/testdriver/click_iframe.html Outdated Show resolved Hide resolved
infrastructure/testdriver/click_nested.html Outdated Show resolved Hide resolved
infrastructure/testdriver/click_window.html Outdated Show resolved Hide resolved
resources/testdriver.js Outdated Show resolved Hide resolved
jgraham and others added 2 commits September 24, 2020 20:20
This is a slightly hacky change to allow testdriver actions to work in
more browsing contexts. The basic idea is that when we want to run an
action in a different borsing context, we pass wptrunner a context
list like [window_id, frame_id...], and use that to select the correct
browsing context just for the duration of the action, before switching
back.

There are a number of limitations with the current patch, some more
serious than others:

* So far this is only implmented for testdriver commands that take an
explicit element. Other commands could be modified to also allow
passing in an explicit context.

* It must be possible to get a window reference and set the name
property (see below). This means this approach only works for
windows that are same origin with the test window.

* WebDriver implementations don't generally support returning a window
object from script, so we can't return a window handle
directly. Instead we either use the existing window.name property or
add a window.name when we try to start the action. Then in wptrunner
we do a linear search of all windows to find the one with the
appropriate name. We have to use window.name rather than writing a
custom property into the window global because the way marionette
sandboxes js we aren't able to read the global property.

However despite these limitations this makes the feature considerably
more versatile than it was previously.
Co-authored-by: Robert Ma <robertma@chromium.org>
@jgraham jgraham merged commit 141c517 into master Sep 25, 2020
@jgraham jgraham deleted the testdriver_context branch September 25, 2020 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs infra testdriver.js webdriver wg-testing_browser wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants