Closed Bug 1400233 Opened 7 years ago Closed 6 years ago

Drop ContentWebElement.LegacyIdentifier from Marionette

Categories

(Remote Protocol :: Marionette, defect, P3)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: wambui, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=py][blocked by bug 1451916])

Attachments

(1 file, 6 obsolete files)

Drop ContentWebElement.LegacyIdentifier key from Marionette.
11.93 KB, patch
ato
: review+
Details | Diff | Splinter Review 
When Marionette creates web element references, it uses the web
element identifier specified by the WebDriver specification
(element.Key) but also a element.LegacyKey.

This will require changes to element.makeWebElement in
testing/marionette/element.js as well as the Marionette
Python client in testing/marionette/client and geckodriver in
testing/geckodriver.
Priority: -- → P3
I believe it is now safe to make this change.
Mentor: ato
Keywords: good-first-bug
Whiteboard: [lang=py]
Version: Version 3 → Trunk
Can I try fixing this bug?
Sure.  The key is now referred to as
ContentWebElement.LegacyIdentifier in Marionette.  Please upload
your patches and flag me for review.
Summary: Drop element.LegacyKey from Marionette → Drop ContentWebElement.LegacyIdentifier from Marionette
Is the key also present in the Python client?  
I have only been able to find it in the element.js file.
Flags: needinfo?(ato)
(In reply to Wambui (:wambui) from comment #4)
> Is the key also present in the Python client?  
> I have only been able to find it in the element.js file.

Yes: https://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#28
Flags: needinfo?(ato)
Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review233210

Thanks for the patch!  This looks good, but you need to make some
minor changes.

You also need to update
https://searchfox.org/mozilla-central/source/testing/marionette/test_element.js.
You can run this test with "./mach xpcshell-test --sequential testing/marionette/test_element.js".

::: commit-message-6ff60:1
(Diff revision 1)
> +Bug 1400233 - Remove ContentWebElement.LegacyIdentifier Key from Marionette; r?ato
> +Marionette currently uses the ContentWebElement.Identifier Key but also has the Legacy Key.
> +The key is removed by making changes to testing/marionette/element.js and
> +testing/marionette/client/marionette_driver/marionette.py.

There should be a blank line between the short commit message and
the long-form message.

::: commit-message-6ff60:2
(Diff revision 1)
> +Bug 1400233 - Remove ContentWebElement.LegacyIdentifier Key from Marionette; r?ato
> +Marionette currently uses the ContentWebElement.Identifier Key but also has the Legacy Key.

s/Key/key/
s/Legacy Key/a legacy key, LegacyIdentifier/

::: moz.configure:549
(Diff revision 1)
> +# Automatically download and use compiled C++ components:
> +ac_add_options --enable-artifact-builds

This shouldn’t be checked in.  You will want to add this to your
local $topsrcdir/mozconfig file, not to the build system defaults.

::: testing/marionette/client/marionette_driver/marionette.py:29
(Diff revision 1)
>  from .geckoinstance import GeckoInstance
>  from .keys import Keys
>  from .timeout import Timeouts
>  
> -WEBELEMENT_KEY = "ELEMENT"
> +
>  W3C_WEBELEMENT_KEY = "element-6066-11e4-a52e-4f735466cecf"

Let’s rename this to WEB_ELEMENT_KEY now.
Attachment #8958374 - Flags: review?(ato) → review-
There are also some linting problems that you can see locally if
you run "./mach lint testing/marionette".
Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review235362


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/element.js:1553
(Diff revision 1)
>   * transported over the wire protocol.
>   */
>  class ContentWebElement extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebElement.Identifier]: this.uuid,
> +      [ContentWebElement.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1580
(Diff revision 1)
>   * over the wire protocol.
>   */
>  class ContentWebWindow extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebWindow.Identifier]: this.uuid,
> +      [ContentWebWindow.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1604
(Diff revision 1)
>   * are represented as web frames over the wire protocol.
>   */
>  class ContentWebFrame extends WebElement {
>    toJSON() {
>      return {
> -      [ContentWebFrame.Identifier]: this.uuid,
> +      [ContentWebFrame.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]

::: testing/marionette/element.js:1627
(Diff revision 1)
>   * over the wire protocol.
>   */
>  class ChromeWebElement extends WebElement {
>    toJSON() {
>      return {
> -      [ChromeWebElement.Identifier]: this.uuid,
> +      [ChromeWebElement.Identifier]: this.uuid

Error: Missing trailing comma. [eslint: comma-dangle]
Comment on attachment 8960862 [details]
Bug 1400233 - Remove ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/229626/#review235414

Your update to this changeset introduced a second commit.  The correct
way to address issues is to _modify_ the original commit instead.
Can you squash these two commits together?

::: commit-message-1b080:3
(Diff revision 1)
> +Marionette currently uses the ContentWebElement.Identifier key but also has the legacy key.
> +The key is removed by making changes to testing/marionette/element.js, testing/marionette/test_element.js
> + and testing/marionette/client/marionette_driver/marionette.py.

Format this at ~72 characters.
Attachment #8960862 - Flags: review?(ato) → review-
Assignee: nobody → wambui.dev
Status: NEW → ASSIGNED
Can you help me on how to squash the two commits together?
I've tried to do it, but I still end up with two commits showing.
You are using mercurial? Then run `hg histedit` for it. Select `r` for the second commit and finish.
Attachment #8960862 - Attachment is obsolete: true
I have triggered a new try run, but I’m not actually sure how to
close the outstanding lint issues.  Maybe the linter will close
them?  It seems like a pretty big flaw to the review system that the
reviewer can’t dismiss them.
wambui, you appear to have some test failures in
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
Will you please have a look and fix up the patch?
Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review236810
Attachment #8958374 - Flags: review?(ato) → review-
Flags: needinfo?(wambui.dev)
Sure, let me have a look at them.
Flags: needinfo?(wambui.dev)
After looking at some of the failed tests, there are seem to be three keys being used "ELEMENT", "element-6066-11e4-a52e-4f735466cecf" and "chromeelement-9fc5-4b51-a3c8-01716eedeb04". The key objects created seem to be made up of the "ELEMENT" key and either the "element-6066-11e4-a52e-4f735466cecf" key or the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key. Without the "ELEMENT" key, the other two are the only ones used. If the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key is used, the tests fail as the `if` statements that use the "element-6066-11e4-a52e-4f735466cecf" key fail.

Here's an example from `testing/marionette/harness/marionette_harness/tests/unit/test_click.py TestClickNavigation.test_click_link_install_addon` that fails with the changes.

https://pastebin.mozilla.org/9081315


Could this be why the tests are failing?


(In reply to Andreas Tolfsen ‹:ato› from comment #17)
> wambui, you appear to have some test failures in
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
> Will you please have a look and fix up the patch?
Flags: needinfo?(ato)
The objects with the "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key seem to not get some attributes because of this, which causes the tests failures.

(In reply to Wambui (:wambui) from comment #20)
> After looking at some of the failed tests, there are seem to be three keys
> being used "ELEMENT", "element-6066-11e4-a52e-4f735466cecf" and
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04". The key objects created seem to
> be made up of the "ELEMENT" key and either the
> "element-6066-11e4-a52e-4f735466cecf" key or the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key. Without the "ELEMENT" key,
> the other two are the only ones used. If the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key is used, the tests fail as
> the `if` statements that use the "element-6066-11e4-a52e-4f735466cecf" key
> fail.
> 
> Here's an example from
> `testing/marionette/harness/marionette_harness/tests/unit/test_click.py
> TestClickNavigation.test_click_link_install_addon` that fails with the
> changes.
> 
> https://pastebin.mozilla.org/9081315
> 
> 
> Could this be why the tests are failing?
> 
> 
> (In reply to Andreas Tolfsen ‹:ato› from comment #17)
> > wambui, you appear to have some test failures in
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d01f198301a7e53d0065c351ee22933abbf9a44c.
> > Will you please have a look and fix up the patch?
(In reply to Wambui (:wambui) from comment #20)

> The key objects created seem to be made up of the "ELEMENT" key
> and either the "element-6066-11e4-a52e-4f735466cecf" key or the
> "chromeelement-9fc5-4b51-a3c8-01716eedeb04" key.

That’s correct.  Marionette used to send a dict such as
{"ELEMENT": <uuid>}, then moved to {"ELEMENT": <uuid>,
"element-6066-11e4-a52e-4f735466cecf": <uuid>}, and most recently
your patch here attempts to drop the ELEMENT part of that.  This is
part of a migration effort.

Marionette has a further specialisation in that it supports
automation of chrome context, where web elements are identified by
chromeelement-9fc5-4b51-a3c8-01716eedeb04.  You will need to change
your patch to also map this to the HTMLElement class in the Python
client.
Flags: needinfo?(ato)
Apparently I’m not allowed to dismiss the issues raised by the code
review bot.  Can you mark them as fixed?
Flags: needinfo?(wambui.dev)
Comment on attachment 8958374 [details]
Bug 1400233 - Drop ContentWebElement.LegacyIdentifier key from Marionette;

https://reviewboard.mozilla.org/r/227322/#review239228
Attachment #8958374 - Flags: review?(ato) → review+
I've marked them as fixed on mozreview.
https://reviewboard.mozilla.org/r/227322/#review235362
Flags: needinfo?(wambui.dev)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato
https://hg.mozilla.org/mozilla-central/rev/6fd96d40bdb3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This change is causing all the Firefox Screenshots Selenium tests to fail.  I believe it's because geckodriver hasn't been updated to use the new key: https://hg.mozilla.org/mozilla-central/file/tip/testing/geckodriver/src/marionette.rs#l655

Here's a trace log snippet:

1522961405067   webdriver::server       DEBUG   -> POST /session/9f2df171-edef-4c6f-8dce-4165742571b2/elements {"using":"css selector","value":"*[id=\"pageActionButton\"]"}
1522961405067   geckodriver::marionette TRACE   -> 82:[0,5,"findElements",{"using":"css selector","value":"*[id=\"pageActionButton\"]"}]
1522961405097   Marionette      TRACE   0 -> [0,5,"findElements",{"using":"css selector","value":"*[id=\"pageActionButton\"]"}]
1522961405100   Marionette      TRACE   0 <- [1,5,null,[{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8aedd9a2-a7f8-4ac7-9560-b4d5ac8e7217"}]]
1522961405106   geckodriver::marionette TRACE   <- [1,5,null,[{"chromeelement-9fc5-4b51-a3c8-01716eedeb04":"8aedd9a2-a7f8-4ac7-9560-b4d5ac8e7217"}]]
1522961405106   webdriver::server       DEBUG   <- 500 Internal Server Error {"value":{"error":"unknown error","message":"Failed to extract Web Element from response","stacktrace":""
}}
(In reply to Barry Chen from comment #30)
> This change is causing all the Firefox Screenshots Selenium tests to fail. 
> I believe it's because geckodriver hasn't been updated to use the new key:
> https://hg.mozilla.org/mozilla-central/file/tip/testing/geckodriver/src/
> marionette.rs#l655

Yes, that is something we figured out yesterday too. See bug 1451916. Maybe we have to wait here until the other bug has been fixed.
Well, I just saw that this patch already landed, and caused a major regression. I would suggest that we get it backed-out, so we can fix bug 1451916 first.

(In reply to Pulsebot from comment #28)
> Pushed by atolfsen@mozilla.com:
> https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
> Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato

Sheriffs, please backout this change. Thanks.
Depends on: 1451916
(In reply to Henrik Skupin (:whimboo) from comment #32)
> Well, I just saw that this patch already landed, and caused a major
> regression. I would suggest that we get it backed-out, so we can fix bug
> 1451916 first.
> 
> (In reply to Pulsebot from comment #28)
> > Pushed by atolfsen@mozilla.com:
> > https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
> > Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato
> 
> Sheriffs, please backout this change. Thanks.

Backout done, merged around trees: https://hg.mozilla.org/mozilla-central/rev/62c2508f27a865dd0ac5ca3f0485cb20afafbbd0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We have to wait for bug 1451916 being fixed and a new geckodriver release, before we can re-land this patch.
Flags: needinfo?(wambui.dev)
Whiteboard: [lang=py] → [lang=py][blocked by bug 1451916]
geckodriver 0.21.0 has been released yesterday. So we can finally land this patch again.
Andreas, I don't have permissions to reopen the mozreview patch for landing. Can you please have a look? Maybe you can as reviewer?

Otherwise if Wambui is faster, I would appreciate if you could do that. Just click the `review` link next to the patch at the top of this bug, and then `Reopen`. Thanks
Flags: needinfo?(wambui.dev)
Flags: needinfo?(ato)
Apparently I don’t have permission to do this either.  My tolerance
for mozreview is diminishing rapidly.

But fear not!  I’ll re-submit the patches for landing.
Flags: needinfo?(wambui.dev)
Flags: needinfo?(ato)
Attachment #8958374 - Attachment is obsolete: true
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Attachment #8985903 - Attachment is obsolete: true
Attachment #8985904 - Flags: review?(hskupin)
Attachment #8985904 - Flags: review?(ato)
Comment on attachment 8985904 [details] [diff] [review]
Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

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

whimboo: I’d like you to have a second look at this before we re-land
it.  Perhaps we need to communicate to the Selenium project that
they need to upgrade to 0.21.0.
Attachment #8985904 - Flags: review?(ato) → review+
Comment on attachment 8985904 [details] [diff] [review]
Drop ContentWebElement.LegacyIdentifier key from Marionette. r?ato,whimboo

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

I miss tests for the chrome element identifier in test_element.js. Mind adding at least a basic one?
Attachment #8985904 - Flags: review?(hskupin) → review+
Alexei, we are trying to land this patch that requires geckodriver 0.21.0 which has been released on Friday last week. Please note that failing Nightly tests would mean you haven't updated yet. 

Otherwise I don't think that we have to worry given that we land on Nightly only. People might notice that they have to upgrade.
Flags: needinfo?(barancev)
Selenium CI uses the latest geckodriver version published to GitHub and Nightly broswer version.
Flags: needinfo?(barancev)
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Attachment #8985904 - Attachment is obsolete: true
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Attachment #8990306 - Attachment is obsolete: true
Sorry I dropped the ball on this one.  The last try run looks good,
but I suspect another rebase is needed by now.
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Attachment #8995530 - Flags: review+
Attachment #8990307 - Attachment is obsolete: true
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/036eaf559f72
Drop ContentWebElement.LegacyIdentifier key from Marionette. r=ato,whimboo
https://hg.mozilla.org/mozilla-central/rev/036eaf559f72
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: