1130979 - Calling find_element() should use constants from marionette.By instead of hard-coded values

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now a lot of our tests and library modules are using a hard-coded method identifier to retrieve elements. We should change that so that the By module as provided by Marionette is always used.

Before:
find_element('ID', ...)

After:
from marionette import By
[..]
find_element(By.ID

http://marionette-client.readthedocs.org/en/latest/reference.html#marionette

Comment 1

[sara_z_tang]

We would like to take this bug :)

Comment 2

[sara_z_tang]

Attachment: patch.dif

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This bug was meant to fix the usage of find_element in the firefox-ui-tests repository but not Marionette. But as I was able to see on IRC there was a discussion with David. He mentioned that parts could be taken for Marionette proper. So I'm going to move this bug over to the Marionette component and hope that you ok with mentoring this bug.

Comment 4

[David Burns :automatedtester]

Comment on attachment 8561686 [details] [diff] [review]
patch.dif

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

This is a great first pass. The following files have other search criteria that can be updated to use By. I have put some below and would be worth checking for more instances.

client/marionette/tests/unit/test_window_switching.py                       
18:        link = self.marionette.find_element("link text", "Open new window")

client/marionette/tests/unit/test_window_handles.py                         
122:        link = self.marionette.find_element("link text", "Open new window")

client/marionette/tests/unit/test_text.py 
19:        l = self.marionette.find_element("name", "myInput") 
27:        l = self.marionette.find_element("name", "myInput")

::: testing/marionette/client/marionette/marionette.py
@@ +31,4 @@
>  
>      CLASS = "class name"
>      SELECTOR = "css selector"
> +    ID = By.ID

We don't need to do this here. If we are going to do ID we might as well do the rest but I have raised https://bugzilla.mozilla.org/show_bug.cgi?id=1131763 to get rid of all of them. The documentation updates can stay

Comment 5

[David Burns :automatedtester]

Hey, is there anything I can do to help you finish this bug?

Comment 6

[Akshat Jain]

Hey,
I wish to work on this bug.Can you please tell me from where should I start !!!

Comment 7

[Kim Brown [:kb15]]

I'd like to work on this bug.  Please assign it to me.  Thanks!

Comment 8

[Andreas Tolfsen ❲:ato❳]

(In reply to :kb15 from comment #7)
> I'd like to work on this bug.  Please assign it to me.  Thanks!

Please submit your patch and I will assign it.

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

FYI, Kim is working on this patch as part of an application to my Outreachy project.
Kim, please feel free to ask questions in the bug or on IRC. Submit your patch using Mozreview [1].

[1] http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html#configuring-your-machine-to-use-mozreview

Comment 10

[Kim Brown [:kb15]]

I came across a something that we might want to add as a constant in the By class.  In the file tests/unit/test_anonymous_content, in the last method (test_find_anonymous_children), find_element is called with the parameter "anon".  Does it make sense to add ANON = "anon" to the By class?

Comment 11

[Andreas Tolfsen ❲:ato❳]

(In reply to Kim Brown [:kb15] from comment #10)
> I came across a something that we might want to add as a constant in the By
> class.  In the file tests/unit/test_anonymous_content, in the last method
> (test_find_anonymous_children), find_element is called with the parameter
> "anon".  Does it make sense to add ANON = "anon" to the By class?

I noticed this the other day when rewriting some tests as well.  Yes, please add this.

Comment 12

[Kim Brown [:kb15]]

Attachment: MozReview Request: Bug 1130979 - Changed hard-coded values to constants from By; r?maja_zf

Review commit: https://reviewboard.mozilla.org/r/39599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39599/

Comment 13

[Kim Brown [:kb15]]

Attachment: MozReview Request: Bug 1130979 - Added ANON constant to By class and changed hard-coded values in test file; r?maja_zf

Review commit: https://reviewboard.mozilla.org/r/39601/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39601/

Comment 14

[Kim Brown [:kb15]]

Attachment: MozReview Request: Bug 1130979 - Added ANON constant to By class

Review commit: https://reviewboard.mozilla.org/r/39603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39603/

Comment 15

[Kim Brown [:kb15]]

Attachment: MozReview Request: Bug 1130979 - Changed hard-coded values "anon" to constant in By class By.ANON

Review commit: https://reviewboard.mozilla.org/r/39605/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39605/

Comment 16

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8729855 [details]
MozReview Request: Bug 1130979 - Added ANON constant to By class

https://reviewboard.mozilla.org/r/39603/#review36625

Comment 17

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8729856 [details]
MozReview Request: Bug 1130979 - Changed hard-coded values "anon" to constant in By class By.ANON

https://reviewboard.mozilla.org/r/39605/#review36629

Comment 18

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39605/#review36629

Looks good, thanks! Please use |hg histedit| to roll the two commits related to 'anon' into one commit and push up for review again. The combined commit should have a commit message like "Bug 123 - Added ANON constants to By class; r?maja_zf".

Comment 19

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8729853 [details]
MozReview Request: Bug 1130979 - Changed hard-coded values to constants from By; r?maja_zf

https://reviewboard.mozilla.org/r/39599/#review36645

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8729854 [details]
MozReview Request: Bug 1130979 - Added ANON constant to By class and changed hard-coded values in test file; r?maja_zf

https://reviewboard.mozilla.org/r/39601/#review36647

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/39601/#review36647

Same for the first two commits that deal with By.ID, etc. - Please use |hg histedit| to roll the two commits into one and push up for review again. The combined commit should have a commit message like "Bug 123 - Change hard-coded values to constants from By; r?maja_zf".

Comment 22

[Kim Brown [:kb15]]

Comment on attachment 8729853 [details]
MozReview Request: Bug 1130979 - Changed hard-coded values to constants from By; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39599/diff/1-2/

Comment 23

[Kim Brown [:kb15]]

Comment on attachment 8729854 [details]
MozReview Request: Bug 1130979 - Added ANON constant to By class and changed hard-coded values in test file; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39601/diff/1-2/

Comment 24

[Kim Brown [:kb15]]

Comment on attachment 8729853 [details]
MozReview Request: Bug 1130979 - Changed hard-coded values to constants from By; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39599/diff/2-3/

Comment 25

[Kim Brown [:kb15]]

Comment on attachment 8729854 [details]
MozReview Request: Bug 1130979 - Added ANON constant to By class and changed hard-coded values in test file; r?maja_zf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39601/diff/2-3/

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31c47d3ac2bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1a5a63f89b

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31c47d3ac2bb
https://hg.mozilla.org/mozilla-central/rev/1d1a5a63f89b

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.