Integrate selenium atoms with marionette

RESOLVED FIXED in mozilla14

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mdas, Assigned: mdas)

Tracking

Trunk
mozilla14
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Selenium maintains a set of code called "atoms" which are optimized, compressed bits of JS code designed for specific Selenium calls (click, sendKeys, etc.). We should use these atoms where possible since the Selenium team maintains this code and there would be unnecessary overlap if we implement the exact same code on our end.
OS: Mac OS X → All
Created attachment 612705 [details] [diff] [review]
Integrate the selenium atoms and add related functionality

This implements the remaining methods from https://wiki.mozilla.org/Auto-tools/Projects/Marionette/Marionette_Client_API#HTMLElement_Methods
Attachment #612705 - Flags: review?(jgriffin)
Created attachment 612706 [details] [diff] [review]
Test cases

Here are the related test cases. I had to add a xul document that we can load in order to test some of the chrome methods.

Also getElementAttribute changed to getAttributeValue in the WebDriver docs, so I updated the client.
Attachment #612706 - Flags: review?(jgriffin)
Comment on attachment 612705 [details] [diff] [review]
Integrate the selenium atoms and add related functionality

Review of attachment 612705 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few minor notes:

::: testing/marionette/marionette-actors.js
@@ +301,5 @@
> +      lines.push(el.value);
> +    }
> +    else if (el["label"]) {
> +      lines.push(el.label);
> +    }

Should also probably include the value of 'label' elements, as well as the text content of any text nodes (you can have text nodes inside either <label> or <description> elements).

@@ +940,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        //Selenium atom doesn't quite work here
> +        let el = this.curBrowser.elementManager.getKnownElement(aRequest.element, this.getCurrentWindow());
> +        if (el.checked != undefined) {

We should probably be checking either the selected or checked attributes, as appropriate.

@@ +969,5 @@
> +    if (this.context == "chrome") {
> +      try {
> +        let el = this.curBrowser.elementManager.getKnownElement(aRequest.element, this.getCurrentWindow());
> +        el.focus();
> +        utils.synthesizeKey(aRequest.value, { type: "keypress" });

EventUtils.synthesizeKey is used for sending a single key, but this API, "sendKeysToElement", requires potentially sending multiple keys.  I think we probably want to call sendString here instead.
Attachment #612705 - Flags: review?(jgriffin) → review+
Comment on attachment 612706 [details] [diff] [review]
Test cases

Review of attachment 612706 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the new tests!  

There are a couple of minor issues here.  One is that a few tests use this line of code:

>        self.marionette.execute_script("window.open('file:///Users/mdas/code/malini_marionette_client/marionette/www/test.xul', '_blank', 'chrome,centerscreen');")

Can we use get_absolute_url to construct these url's?  If that doesn't work for these xul files, then we'll need to construct a path using Python.

Second issue is that a few chrome tests verify things about the current XUL structure of Firefox, like:

>	def test_child_elements(self):
>		el = self.marionette.find_element("id", "bundle_shell")
>		parent = self.marionette.find_element("id", "stringbundleset")
>		found_els = parent.find_elements("tag name", "stringbundle")
>		self.assertTrue(el.id in [found_el.id for found_el in found_els])

This seems potentially fragile; it would probably be better to use your test.xul file to do these tests.
Attachment #612706 - Flags: review?(jgriffin) → review-
Created attachment 613443 [details] [diff] [review]
Integrate the selenium atoms and add related functionality v0.2

Updated with your comments
Attachment #612705 - Attachment is obsolete: true
Attachment #613443 - Flags: review+
Created attachment 613445 [details] [diff] [review]
Test cases v0.2

Accessing the .xul file using http didn't quite work, so I had to get the file:// based url, so I ended up having to use os.path.

Also updated the dependencies on browser.xul, thanks for finding these issues!
Attachment #612706 - Attachment is obsolete: true
Attachment #613445 - Flags: review?(jgriffin)
Attachment #613445 - Flags: review?(jgriffin) → review+
I have one more request.  For the sake of future maintenance, can we get a list of atoms that we're bundling put into some text file, e.g., atoms/HOWTO?  I found it was actually difficult to figure this out, since the minified version of the atoms is so unreadable.
Created attachment 613687 [details] [diff] [review]
Integrate the selenium atoms and add related functionality v0.3

Sure, good point, I have them as comments in the atoms file, but adding them to the HOWTO would be a very helpful addition :)

I also noticed that last patch had leaked a change in browser/confvars.sh so I removed that.
Attachment #613443 - Attachment is obsolete: true
Attachment #613687 - Flags: review+
Backed out 65f02c832a0e due to b2g build bustage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/acb53b7241c5
Created attachment 613734 [details] [diff] [review]
Integrate the selenium atoms and add related functionality v0.4

Added the modified ChromeUtils.js and EventUtils.js files to testing/marionette.

Tested on a purged m-c repository with no problems.
Attachment #613687 - Attachment is obsolete: true
Attachment #613734 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d384f54707cc
https://hg.mozilla.org/mozilla-central/rev/731f7b016723
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.