Closed Bug 1130979 Opened 5 years ago Closed 4 years ago

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

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: whimboo, Assigned: kb15, Mentored)

References

()

Details

(Keywords: pi-marionette-client, Whiteboard: [good first bug][lang=py])

Attachments

(2 files, 3 obsolete files)

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
We would like to take this bug :)
Attached patch patch.dif (obsolete) — Splinter Review
Attachment #8561686 - Flags: review?(hskupin)
Mentor: hskupin → dburns
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]
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-
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)
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
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)
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
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
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".
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
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
Attachment #8729855 - Attachment is obsolete: true
Attachment #8729856 - Attachment is obsolete: true
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 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/
https://hg.mozilla.org/mozilla-central/rev/31c47d3ac2bb
https://hg.mozilla.org/mozilla-central/rev/1d1a5a63f89b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.