Closed
Bug 1375660
Opened 7 years ago
Closed 7 years ago
Update various used atoms from selenium
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: automatedtester, Assigned: whimboo)
References
Details
Attachments
(10 files)
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
ato
:
review+
|
Details |
It looks like there has been a change to the selenium atom that is causing people to get different values from Firefox compared to the other drivers/browsers
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(dave.hunt)
Reporter | ||
Comment 2•7 years ago
|
||
Dave, can you download one of the builds from treeherder, linked from mozreview and see if that works for you as expected
Comment 3•7 years ago
|
||
I downloaded the target.dmg from the OS X 10.10 opt build from https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5dc3c73291c1d431abaebd61bb34b9f83f06ae, extracted Nightly.app, set this as my target binary and the test still failed for the same reason.
Here are my steps to reproduce (using Python selenium package 3.4.3):
from selenium.webdriver import Firefox
from selenium.webdriver.firefox.options import Options
options = Options()
options.binary = '/Users/dhunt/Downloads/Nightly.app/Contents/MacOS/firefox-bin'
firefox = Firefox(firefox_options=options)
firefox.get('https://addons.mozilla.org/en-US/firefox/addon/firebug/')
firefox.find_element_by_css_selector('#preview .thumbnail').click()
assert not firefox.find_element_by_css_selector('#lightbox .next').is_displayed()
Flags: needinfo?(dave.hunt)
Reporter | ||
Comment 4•7 years ago
|
||
Then this is not because the atom is out of date, we need to figure out what Selenium is doing differently.
Reporter | ||
Comment 5•7 years ago
|
||
The atom does appear to be slightly out of date so that needs fixing but the underlying issue is we appear to be doing the wrong thing for isDisplayed...
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 6•7 years ago
|
||
Given that we have to update a couple of different atoms lets get this done all at once in this bug. For now we know that there is at least bug 1381519 which might benefit from an update too.
Sadly there are no instructions available in how to build the atoms. So I had to do a bit of investigation...
I queried a bit and found: https://github.com/SeleniumHQ/selenium/wiki/Automation-Atoms
Sadly it doesn't tell that much about compiled fragments, and the link at the bottom is also outdated. But I found build targets like: "./go //javascript/atoms/fragments:get_visible_text:firefox", which generates a minimized version of this specific method. Having a look at our Marionette code I can find the following atom methods in use:
clearElement
getElementAttribute
getElementText
isElementDisplayed
isElementEnabled
isElementSelected
In bug 1245153 Andreas moved the atoms from the /atoms subfolder directly into /testing/marionette. So the build instruction also got removed. But thanks to the history here those are: https://hg.mozilla.org/mozilla-central/diff/3edb67388ad6/testing/marionette/atoms/HOWTO.
So as it looks like we would have to compile the methods for each of the above listed items, and feed the individual functions in atoms.js with the generated code.
Here the build targets which I found for the methods above:
./go //javascript/atoms/fragments:is_enabled:firefox
./go //javascript/atoms/fragments:clear:firefox
./go //javascript/atoms/fragments:get_attribute:firefox
./go //javascript/atoms/fragments:get_visible_text:firefox
./go //javascript/atoms/fragments:is_shown:firefox
./go //javascript/atoms/fragments:is_selected:firefox
I will check how updating our used atoms work, and file dependencies if necessary.
Assignee | ||
Updated•7 years ago
|
Assignee: dburns → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Summary: Update isDisplayed atom from selenium → Update various used atoms from selenium
Assignee | ||
Comment 7•7 years ago
|
||
I got all the changes integrated, but noticed some problems:
1) The clearElement update is not working given that the code in atom.js is requiring a global window object which does not exist in our case. When I run the code in a scratch pad it works all fine. But in Marionette I get a window reference not found
2) With the update of isElementDisplayed I see a test failure in test_accessibility.py which is not that clear to me and would need further investigation.
3) Exporting those atoms separately seem to add a huge overhead to the code. David, do you know how to get them all exported with as minimal as possible in size?
Flags: needinfo?(dburns)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #7)
> 1) The clearElement update is not working given that the code in atom.js is
> requiring a global window object which does not exist in our case. When I
> run the code in a scratch pad it works all fine. But in Marionette I get a
> window reference not found
Just to add... where is this code coming from:
> goog.require('bot.action'); goog.exportSymbol('_', bot.action.clear);
I assume it's a Google API. It would be good to have a look if the calling conventions of clear have been changed.
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #7)
> 1) The clearElement update is not working given that the code in atom.js is
> requiring a global window object which does not exist in our case. When I
> run the code in a scratch pad it works all fine. But in Marionette I get a
> window reference not found
After talking to Simon he proposed just to add the parameters for element and window back into the imported function. After I did that all is working fine.
> 2) With the update of isElementDisplayed I see a test failure in
> test_accessibility.py which is not that clear to me and would need further
> investigation.
This needs investigation.
> 3) Exporting those atoms separately seem to add a huge overhead to the code.
> David, do you know how to get them all exported with as minimal as possible
> in size?
I exported the wrong ones. We don't need /javascript/atoms but /javascript/webdriver/atoms. Given that there were no rules anymore in BUCKET Simon did us the favor and added them back via:
https://github.com/SeleniumHQ/selenium/commit/facd199c31c9d81316a1db3bbb0c10603799bb8b
I will continue on Monday, so we hopefully have updated data mid next week.
Flags: needinfo?(dburns)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
The command to build all the exports should be the following now:
python buck-out/crazy-fun/be2bf932342e5d67f58c9b26f5065c745d285d0d/buck.pex build //javascript/webdriver/atoms:clear-element-firefox //javascript/webdriver/atoms:get-attribute-firefox //javascript/webdriver/atoms:get-text-firefox //javascript/webdriver/atoms:is-displayed-firefox //javascript/webdriver/atoms:is-enabled-firefox //javascript/webdriver/atoms:is-selected-firefox --show-output
I have updated all of our used atoms, and as it looks like only the accessibility failures will remain. I will do another try push, so that we can get more clarification.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 31•7 years ago
|
||
Ok, so the accessibility test is broken because of the following reason:
> # Elements that are either accessible to accessibility API or not accessible
> # at all
> falsy_elements = [
> # Element is only visible to the accessibility API and may be
> # manipulated by it
> "button9",
> # Element is not currently visible
> "button10"
> ]
>
> displayed_elementIDs = [
> "button1", "button2", "button3", "button4", "button5", "button6",
> "button9", "no_accessible_but_displayed"
> ]
As you can see button 9 has been added to both the 'falsy_elements' and 'displayed_elementIDs'. Checking the style definitions of 'button9' we have:
> style="position:absolute;left:-100px;top:-455px;"
So it means this button has not the displayed=true state because it has been moved off-screen and is not visible to the user.
Having it as part of the displayed list is a bug, and as it looks like an update of the Selenium atom for `is_displayed` fixed that.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8913678 -
Flags: review?(ato)
Attachment #8914524 -
Flags: review?(ato)
Attachment #8913679 -
Flags: review?(ato)
Attachment #8913680 -
Flags: review?(ato)
Attachment #8913681 -
Flags: review?(ato)
Attachment #8913682 -
Flags: review?(ato)
Attachment #8913683 -
Flags: review?(ato)
Attachment #8913684 -
Flags: review?(ato)
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8913678 [details]
Bug 1375660 - Fix test_l10n.py for title text.
https://reviewboard.mozilla.org/r/185068/#review190950
Attachment #8913678 -
Flags: review?(ato) → review+
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8914524 [details]
Bug 1375660 - Remove duplicate button 9 reference in test_accessibility.
https://reviewboard.mozilla.org/r/185840/#review190952
Attachment #8914524 -
Flags: review?(ato) → review+
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.
https://reviewboard.mozilla.org/r/185070/#review190954
This atom is, as far as I can tell, used in one single place in the
Marionette code:
> interaction.isElementEnabled = function(el, strict = false) {
> let enabled = true;
> let win = getWindow(el);
>
> if (element.isXULElement(el)) {
> // check if XUL element supports disabled attribute
> if (DISABLED_ATTRIBUTE_SUPPORTED_XUL.has(el.tagName.toUpperCase())) {
> let disabled = atom.getElementAttribute(el, "disabled", win);
> if (disabled && disabled === "true") {
> enabled = false;
> }
> }
> } else {
> enabled = atom.isElementEnabled(el, {frame: win});
> }
[…]
In the above code we use it to get a XULElement’s "disabled"
attribute, which we could also get using el.getAttribute. It
would be preferable to rely on as few Selenium atoms as possible.
As far as I know the only atom we eventually want to rely on is
the getElementText atom; the rest will have to be reimplemented
according to the specification to make Marionette spec conforming.
Instead of upgrading the getElementText atom, I think
it would be prudent to remove it altogether and fix
interaction.isElementEnabled.
Attachment #8913679 -
Flags: review?(ato) → review-
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8913680 [details]
Bug 1375660 - Update Selenium atom: getElementText.
https://reviewboard.mozilla.org/r/185072/#review190956
Attachment #8913680 -
Flags: review?(ato) → review+
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8913681 [details]
Bug 1375660 - Update Selenium atom: isElementDisplayed.
https://reviewboard.mozilla.org/r/185074/#review190958
Attachment #8913681 -
Flags: review?(ato) → review+
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8913682 [details]
Bug 1375660 - Update Selenium atom: isElementEnabled.
https://reviewboard.mozilla.org/r/185076/#review190960
At some point we are going to have to reimplement this too.
Attachment #8913682 -
Flags: review?(ato) → review+
Comment 45•7 years ago
|
||
mozreview-review |
Comment on attachment 8913683 [details]
Bug 1375660 - Update Selenium atom: isElementSelected.
https://reviewboard.mozilla.org/r/185078/#review190962
I see we have a lot of work ahead of us. Do you mind checking that
we have bug files against the “remove Selenium atoms” bug for
implementing spec-conforming versions of the commands using these
atoms?
Attachment #8913683 -
Flags: review?(ato) → review+
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8913684 [details]
Bug 1375660 - Update Selenium atom: clearElement.
https://reviewboard.mozilla.org/r/185080/#review190964
Attachment #8913684 -
Flags: review?(ato) → review+
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.
https://reviewboard.mozilla.org/r/185070/#review190954
That makes sense and is easy to fix. An updated patch will contain the removal of this atom.
Assignee | ||
Comment 48•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8913683 [details]
Bug 1375660 - Update Selenium atom: isElementSelected.
https://reviewboard.mozilla.org/r/185078/#review190962
Yes, there is bug 1354203 and it already tracks the removal of the individual Selenium atoms.
Assignee | ||
Comment 49•7 years ago
|
||
Andreas, I think that it would make sense to put the commands necessary to export the atoms in some readme. But not sure which to use. The readme.html under testing/marionette doesn't seem to be appropriate. Do you have any suggestions?
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 56•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #49)
> Andreas, I think that it would make sense to put the commands
> necessary to export the atoms in some readme. But not sure which
> to use. The readme.html under testing/marionette doesn't seem to
> be appropriate. Do you have any suggestions?
I will put the API documentation under testing/marionette/doc/api
in https://bugzil.la/1405757 so that you can put this in
testing/marionette/doc/ExportingAtoms.md or similar, like we have
done for testing/geckodriver/doc/Releasing.md:
https://searchfox.org/mozilla-central/source/testing/geckodriver/doc/Releasing.md
Flags: needinfo?(ato)
Comment 57•7 years ago
|
||
mozreview-review |
Comment on attachment 8913679 [details]
Bug 1375660 - Remove Selenium atom: getElementAttribute.
https://reviewboard.mozilla.org/r/185070/#review191520
Attachment #8913679 -
Flags: review?(ato) → review+
Assignee | ||
Comment 58•7 years ago
|
||
So I will wait for bug 1405757 being fixed on central.
Depends on: 1405757
Comment 59•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #58)
> So I will wait for bug 1405757 being fixed on central.
You can actually put the file in there right away. The directory
isn’t going away.
Comment hidden (mozreview-request) |
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8915539 [details]
Bug 1375660 - Add instructions for updating the Selenium atoms.
https://reviewboard.mozilla.org/r/186748/#review191810
Attachment #8915539 -
Flags: review?(ato) → review+
Comment 62•7 years ago
|
||
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6db2db8b29f4
Fix test_l10n.py for title text. r=ato
https://hg.mozilla.org/integration/autoland/rev/3c3867c8593f
Remove duplicate button 9 reference in test_accessibility. r=ato
https://hg.mozilla.org/integration/autoland/rev/8d29f94988a9
Remove Selenium atom: getElementAttribute. r=ato
https://hg.mozilla.org/integration/autoland/rev/988dca8eca1a
Update Selenium atom: getElementText. r=ato
https://hg.mozilla.org/integration/autoland/rev/fbcd3b841574
Update Selenium atom: isElementDisplayed. r=ato
https://hg.mozilla.org/integration/autoland/rev/64093a2fa94a
Update Selenium atom: isElementEnabled. r=ato
https://hg.mozilla.org/integration/autoland/rev/6a7091b50bbf
Update Selenium atom: isElementSelected. r=ato
https://hg.mozilla.org/integration/autoland/rev/8d47e6059cdf
Update Selenium atom: clearElement. r=ato
https://hg.mozilla.org/integration/autoland/rev/b63029f0a1c2
Add instructions for updating the Selenium atoms. r=ato
Comment 63•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6db2db8b29f4
https://hg.mozilla.org/mozilla-central/rev/3c3867c8593f
https://hg.mozilla.org/mozilla-central/rev/8d29f94988a9
https://hg.mozilla.org/mozilla-central/rev/988dca8eca1a
https://hg.mozilla.org/mozilla-central/rev/fbcd3b841574
https://hg.mozilla.org/mozilla-central/rev/64093a2fa94a
https://hg.mozilla.org/mozilla-central/rev/6a7091b50bbf
https://hg.mozilla.org/mozilla-central/rev/8d47e6059cdf
https://hg.mozilla.org/mozilla-central/rev/b63029f0a1c2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•