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

RESOLVED FIXED in Firefox 48

Status

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: whimboo, Assigned: kb15, Mentored)

Tracking

({pi-marionette-client})

unspecified
mozilla48
pi-marionette-client
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [good first bug][lang=py], URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
We would like to take this bug :)

Comment 2

4 years ago
Created attachment 8561686 [details] [diff] [review]
patch.dif
Attachment #8561686 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Mentor: hskupin → dburns
(Reporter)

Comment 3

4 years ago
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.
Assignee: nobody → sara_z_tang
Status: NEW → ASSIGNED
Component: Firefox UI Tests → Marionette
Product: Mozilla QA → Testing
Whiteboard: [lang=py][good first bug] → [lang=py]
(Reporter)

Updated

4 years ago
Attachment #8561686 - Flags: review?(hskupin) → review?(dburns)
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
Attachment #8561686 - Flags: review?(dburns) → review-
Keywords: ateam-marionette-client
Whiteboard: [lang=py] → [good first bug][lang=py]
Hey, is there anything I can do to help you finish this bug?
Flags: needinfo?(sara_z_tang)

Comment 6

3 years ago
Hey,
I wish to work on this bug.Can you please tell me from where should I start !!!
Assignee: sara_z_tang → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 7

3 years ago
I'd like to work on this bug.  Please assign it to me.  Thanks!
(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.
Flags: needinfo?(sara_z_tang)
Attachment #8561686 - Attachment is obsolete: true
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
Assignee: nobody → kbmoz1515
Mentor: mjzffr
(Assignee)

Comment 10

3 years ago
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?
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

3 years ago
Created attachment 8729853 [details]
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/
(Assignee)

Comment 13

3 years ago
Created 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 commit: https://reviewboard.mozilla.org/r/39601/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39601/
(Assignee)

Comment 14

3 years ago
Created attachment 8729855 [details]
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/
(Assignee)

Comment 15

3 years ago
Created attachment 8729856 [details]
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/
Attachment #8729853 - Flags: review?(mjzffr)
Attachment #8729854 - Flags: review?(mjzffr)
Attachment #8729855 - Flags: review?(mjzffr)
Attachment #8729856 - Flags: review?(mjzffr)
Attachment #8729855 - Flags: review?(mjzffr) → review+
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
Attachment #8729856 - Flags: review?(mjzffr) → review+
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 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
Attachment #8729853 - Flags: review?(mjzffr) → review+
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
Attachment #8729854 - Flags: review?(mjzffr) → review+
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".
(Assignee)

Comment 22

2 years ago
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/
Attachment #8729853 - Attachment description: MozReview Request: Bug 1130979 - Changed hard-coded values in find_element() and find_elements() calls to constants from marionette_driver.by. → MozReview Request: Bug 1130979 - Changed hard-coded values to constants from By; r?maja_zf
(Assignee)

Comment 23

2 years ago
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/
Attachment #8729854 - Attachment description: MozReview Request: Bug 1130979 - Changed hard-coded values in find_element() calls to constants from by. → MozReview Request: Bug 1130979 - Added ANON constant to By class and changed hard-coded values in test file; r?maja_zf
(Assignee)

Updated

2 years ago
Attachment #8729855 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8729856 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/31c47d3ac2bb
https://hg.mozilla.org/mozilla-central/rev/1d1a5a63f89b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.