Closed Bug 742947 Opened 8 years ago Closed 8 years ago

Integrate selenium atoms with marionette

Categories

(Testing :: Marionette, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: mdas, Assigned: mdas)

Details

Attachments

(2 files, 4 obsolete files)

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
Attached patch Test cases (obsolete) — Splinter Review
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-
Updated with your comments
Attachment #612705 - Attachment is obsolete: true
Attachment #613443 - Flags: review+
Attached patch Test cases v0.2Splinter Review
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.
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+
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+
You need to log in before you can comment on or make changes to this bug.