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

Implement testdriver for Firefox using marionette backend #9391

Merged
merged 4 commits into from Mar 15, 2018

Conversation

Projects
None yet
6 participants
@jgraham
Copy link
Contributor

jgraham commented Feb 5, 2018

This change is Reviewable

@jgraham jgraham requested review from gsnedders and andreastt Feb 5, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Feb 5, 2018

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Feb 5, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 5, 2018

Build PASSED

Started: 2018-03-15 14:41:54
Finished: 2018-03-15 14:45:16

Failing Jobs

  • safari:11.0
  • MicrosoftEdge:16.16299

View more information about this build on:

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 5, 2018

Reviewed 7 of 7 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


tools/wptrunner/wptrunner/executors/base.py, line 489 at r1 (raw file):

    def __init__(self, logger, protocol, test_window):
        self.protocol = protocol

Can we state what the type of protocol is? I was expecting it to be a Marionette/Selenium object, but it appears to be an executor?


tools/wptrunner/wptrunner/executors/base.py, line 501 at r1 (raw file):

        }

    def __call__(self, result):

Can we add some comment saying what this assumes the structure of result is? Because it's not at all immediately obvious, especially with various subscripts of result being used all over the place.

It's one thing to have this assuming various things about the type of result when it's specific to the Selenium executor, but it seems much more confusing when it's supposedly generic.


tools/wptrunner/wptrunner/executors/base.py, line 513 at r1 (raw file):

    def process_action(self, result):
        parent = self.protocol.current_window_handle()

Can we somewhere document things that the protocol object are expected to implement, in an ABC or something?


Comments from Reviewable

def __call__(self, data):
selector = data["selector"]
elements = self.protocol.find_elements_by_css_selector(selector)
print elements

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Leftover from debug?

elements = self.protocol.find_elements_by_css_selector(selector)
print elements
if len(elements) == 0:
raise ValueError("Selector matches no elements")

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Do we think one error type is fine, or is there a requirement for more finely grained types?

if len(elements) == 0:
raise ValueError("Selector matches no elements")
elif len(elements) > 1:
raise ValueError("Selector matches multiple elements")

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Couldn’t this use find_element_by_css_selector above? Or is the concern that it is fallible and might return an error?

self.protocol.click_element(elements[0])
except Exception as e:
self.logger.warning(e)
return False

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Should the error not be propagated?

@@ -306,13 +333,33 @@ def clear_origin(self, url):
with self.marionette.using_context(self.marionette.CONTEXT_CHROME):
self.marionette.execute_script(script)

def find_elements_by_css_selector(self, selector):
return self.marionette.find_elements("css selector", selector)

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Instead of these, couldn’t you just called self.protocol.marionette.find_elements so you don’t have to reinvent the API?

}
if message:
obj["message"] = str(message)
self.marionette.execute_script("window.postMessage(%s, '*')" % json.dumps(obj))

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

This is clever, I like it.

clearTimeout(timer);
var tests = event.data.tests;
var status = event.data.status;
window.wrappedJSObject.message_queue = [];

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Do you still need wrappedJSObject?

@@ -384,6 +431,8 @@ def _run(self):


class MarionetteTestharnessExecutor(TestharnessExecutor):
supports_testdriver = True

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

Where does this get picked up?

raise ValueError("Unknown callback type %r" % result[1])

def process_complete(self, result):
rv = [result[0]] + result[2]

This comment has been minimized.

Copy link
@andreastt

andreastt Feb 5, 2018

Member

I think it’s worth documenting the message format somewhere. Reverse engineering the test data format when I implemented the pytest runner was not my greatest fun-packed day at work.

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Feb 5, 2018

Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.


tools/wptrunner/wptrunner/executors/base.py, line 489 at r1 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

Can we state what the type of protocol is? I was expecting it to be a Marionette/Selenium object, but it appears to be an executor?

It's a Protocol object. But yes. we can.


tools/wptrunner/wptrunner/executors/base.py, line 513 at r1 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

Can we somewhere document things that the protocol object are expected to implement, in an ABC or something?

I think if we want to do this the only sensible option is to pull it apart into multiple objects and compose them together somehow. At the moment a protocol is a grab-bag of things that use whatever the underlying wire protocol is.


tools/wptrunner/wptrunner/executors/base.py, line 550 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Couldn’t this use find_element_by_css_selector above? Or is the concern that it is fallible and might return an error?

YEah, it's that >1 shoudl return an error.


tools/wptrunner/wptrunner/executors/executormarionette.py, line 337 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Instead of these, couldn’t you just called self.protocol.marionette.find_elements so you don’t have to reinvent the API?

I think we decided this wasn't an issue


tools/wptrunner/wptrunner/executors/testharness_marionette.js, line 4 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Do you still need wrappedJSObject?

I don't know! If not then we can share scripts with the selenium implementation.


Comments from Reviewable

@andreastt

This comment has been minimized.

Copy link
Member

andreastt commented Feb 6, 2018

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.


tools/wptrunner/wptrunner/executors/testharness_marionette.js, line 4 at r1 (raw file):

Previously, jgraham wrote…

I don't know! If not then we can share scripts with the selenium implementation.

I suspect you could do that. But I’m fine with making that change at a later point.


Comments from Reviewable

@jgraham jgraham force-pushed the testdriver_marionette branch from af20fe0 to 9c23224 Feb 22, 2018

jgraham added some commits Feb 1, 2018

Refactor testdriver implemenation.
This puts the main logic for handling testdriver callbacks into the
base class where it can be shared by multiple implemenations. It also
moves most of the interaction onto the Protocol object which can be
implemented on a per-executor basis.
Implement testdriver for marionette.
This brings the implemenatation in line with the Chrome one i.e. a
single click method is supported.

@jgraham jgraham force-pushed the testdriver_marionette branch from 9c23224 to 18ceb94 Feb 27, 2018

@kereliuk

This comment has been minimized.

Copy link
Contributor

kereliuk commented Feb 28, 2018

Review status: 4 of 11 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.


tools/wptrunner/wptrunner/executors/base.py, line 489 at r1 (raw file):

Previously, jgraham wrote…

It's a Protocol object. But yes. we can.

+1 to this. Although I don't think we should be draconian about it, I think we should air on the side of documenting the types of stuff everywhere we can since this is python :)


tools/wptrunner/wptrunner/executors/base.py, line 509 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

I think it’s worth documenting the message format somewhere. Reverse engineering the test data format when I implemented the pytest runner was not my greatest fun-packed day at work.

I have also had this same problem :)


tools/wptrunner/wptrunner/executors/base.py, line 513 at r1 (raw file):

Previously, jgraham wrote…

I think if we want to do this the only sensible option is to pull it apart into multiple objects and compose them together somehow. At the moment a protocol is a grab-bag of things that use whatever the underlying wire protocol is.

AFAIK the protocol is expected to implement:
current_window_handle
execute_sync
execute_async
switch_to_window
new_session
get_elements_by_css (for testdriver.js)

but in the future the plan is to have many more things implemented for testdriver, so I am +1 to at least documenting what things are currently expected somewhere


Comments from Reviewable

@kereliuk

This comment has been minimized.

Copy link
Contributor

kereliuk commented Feb 28, 2018

Also are you able to write tests for at least part of this (maybe the protocol file?). We currently don't have much coverage test wise with wptrunner, but I want to change that :)

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Feb 28, 2018

Review status: 4 of 11 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.


tools/wptrunner/wptrunner/executors/base.py, line 501 at r1 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

Can we add some comment saying what this assumes the structure of result is? Because it's not at all immediately obvious, especially with various subscripts of result being used all over the place.

It's one thing to have this assuming various things about the type of result when it's specific to the Selenium executor, but it seems much more confusing when it's supposedly generic.

I didn't add a comment, but I made the structure explicit in the usage.


tools/wptrunner/wptrunner/executors/base.py, line 513 at r1 (raw file):

Previously, kereliuk (Jonathon Kereliuk) wrote…

AFAIK the protocol is expected to implement:
current_window_handle
execute_sync
execute_async
switch_to_window
new_session
get_elements_by_css (for testdriver.js)

but in the future the plan is to have many more things implemented for testdriver, so I am +1 to at least documenting what things are currently expected somewhere

I'm not really sure what the documentation would look like here. What you need to implement depends on what features you support, which in turn depends on your executor.

A long term consequence of the design here is that we could remove some of the duplication between executors and put all the protocol-specific logic in the Protocol object. I haven't done that refactor yet because I would like to land this change at some point ;)


tools/wptrunner/wptrunner/executors/base.py, line 548 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Do we think one error type is fine, or is there a requirement for more finely grained types?

I think that it doesn't matter much because these aren't going to be handled, they're just going to be reported to the user.


Comments from Reviewable

@jgraham

This comment has been minimized.

Copy link
Contributor Author

jgraham commented Feb 28, 2018

The protocol file seems hard to test in isolation since it requires an actual browser connection to test.

My preferred way of testing this is to write wptrunner integration tests, check them in to infrastructure and then land #9676 so that they are run in Travis (note this PR has such a test for click).

Refactor the Protocol object into multiple ProtocolParts.
The goal here is to make it possible for different implemenations to
implement defined subsets of the Protocol object, but require each
subset to match a specific base class so it's clear when you have
implemented all the pieces of a specific subset.

The code here uses a sort of composition rather than
inheritance. Multiple inheritance tends to be rather confusing, so
instead we do something similar but slihgtly more explicit; assigning
each ProtocolPart to an attibute of the Protocol so that the methods
on a part with name foo are available as protocol.foo.method. This
involves some changes to the lifecycle methods. In particular setup()
is now split into connect() and after_connect() that are expected to
be implemented in base classes. Between these being called we call
setup() on each ProtocolPart so that we may give them direct access to
a connection object or similar.

@jgraham jgraham force-pushed the testdriver_marionette branch from 18ceb94 to 5fd825d Feb 28, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Mar 15, 2018

:lgtm:


Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


tools/wptrunner/wptrunner/executors/executormarionette.py, line 434 at r1 (raw file):

Previously, andreastt (Andreas Tolfsen) wrote…

Where does this get picked up?

tools/wptrunner/wptrunner/wptrunner.py


Comments from Reviewable

to actually land this

@gsnedders gsnedders merged commit 409239b into master Mar 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gsnedders gsnedders deleted the testdriver_marionette branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.