Drop ContentWebElement.LegacyIdentifier from Marionette

RESOLVED FIXED in Firefox 63

Status

P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: ato, Assigned: wambui, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla63
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [lang=py][blocked by bug 1451916])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Priority: -- → P3
(Reporter)

Comment 1

a year ago
I believe it is now safe to make this change.
Mentor: ato
Keywords: good-first-bug
Whiteboard: [lang=py]
(Reporter)

Updated

a year ago
Version: Version 3 → Trunk
(Assignee)

Comment 2

a year ago
Can I try fixing this bug?
(Reporter)

Comment 3

a year ago
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
(Assignee)

Comment 4

a year ago
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)
(Reporter)

Comment 5

a year ago
(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 hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
mozreview-review
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-
(Reporter)

Comment 9

a year ago
There are also some linting problems that you can see locally if
you run "./mach lint testing/marionette".
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
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]
(Reporter)

Comment 12

a year ago
mozreview-review
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-
(Reporter)

Updated

a year ago
Assignee: nobody → wambui.dev
Status: NEW → ASSIGNED
(Assignee)

Comment 13

a year ago
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.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8960862 - Attachment is obsolete: true
(Reporter)

Comment 16

a year ago
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.
(Reporter)

Comment 17

a year ago
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?
(Reporter)

Comment 18

a year ago
mozreview-review
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-
(Reporter)

Updated

a year ago
Flags: needinfo?(wambui.dev)
(Assignee)

Comment 19

a year ago
Sure, let me have a look at them.
Flags: needinfo?(wambui.dev)
(Assignee)

Comment 20

a year ago
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)
(Assignee)

Comment 21

a year ago
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?
(Reporter)

Comment 22

a year ago
(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)
Comment hidden (mozreview-request)
(Reporter)

Comment 25

a year ago
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)
(Reporter)

Comment 26

a year ago
mozreview-review
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+
(Assignee)

Comment 27

a year ago
I've marked them as fixed on mozreview.
https://reviewboard.mozilla.org/r/227322/#review235362
Flags: needinfo?(wambui.dev)

Comment 28

a year ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6fd96d40bdb3
Drop ContentWebElement.LegacyIdentifier key from Marionette; r=ato

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fd96d40bdb3
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Comment 30

a year ago
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 → ---
status-firefox61: fixed → ---
Target Milestone: mozilla61 → ---
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]
Comment hidden (obsolete)
Comment hidden (obsolete)
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)
(Reporter)

Comment 39

9 months ago
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)
(Reporter)

Updated

9 months ago
Attachment #8958374 - Attachment is obsolete: true
(Reporter)

Comment 40

9 months ago
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
(Reporter)

Comment 41

9 months ago
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
(Reporter)

Updated

9 months ago
Attachment #8985903 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Attachment #8985904 - Flags: review?(hskupin)
Attachment #8985904 - Flags: review?(ato)
(Reporter)

Comment 42

9 months ago
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)

Comment 45

9 months ago
Selenium CI uses the latest geckodriver version published to GitHub and Nightly broswer version.
Flags: needinfo?(barancev)
(Reporter)

Comment 46

9 months ago
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
(Reporter)

Updated

9 months ago
Attachment #8985904 - Attachment is obsolete: true
(Reporter)

Comment 47

9 months ago
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
(Reporter)

Updated

9 months ago
Attachment #8990306 - Attachment is obsolete: true
(Reporter)

Comment 49

8 months ago
Sorry I dropped the ball on this one.  The last try run looks good,
but I suspect another rebase is needed by now.
(Reporter)

Comment 50

8 months ago
Remove the legacy key that Marionette uses to identify web elements.

MozReview-Commit-ID: 1gLvw18Ecv8
Attachment #8995530 - Flags: review+
(Reporter)

Updated

8 months ago
Attachment #8990307 - Attachment is obsolete: true

Comment 51

8 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/036eaf559f72
Drop ContentWebElement.LegacyIdentifier key from Marionette. r=ato,whimboo

Comment 52

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/036eaf559f72
Status: REOPENED → RESOLVED
Last Resolved: a year ago8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.