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

RESOLVED FIXED in mozilla38

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: whimboo, Assigned: ankit.goyal90, Mentored)

Tracking

unspecified
mozilla38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Right now a lot of our tests and library modules in the firefox-ui-tests repository 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

This does not only apply to ID but also all the other values which can be used here.
(Assignee)

Comment 1

4 years ago
Henrik, I am working on it.
Flags: needinfo?(hskupin)
(Reporter)

Comment 2

4 years ago
Awesome! Thanks for letting me know. I will mark you as assignee of the bug then!
Assignee: nobody → ankit.goyal90
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
(Assignee)

Comment 3

4 years ago
Created attachment 8563222 [details] [diff] [review]
Bug 1131712 - Calling find_element() should use constants from marionette.By instead of hard-coded values. r=whimboo
Attachment #8563222 - Flags: review?(hskupin)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8563222 [details] [diff] [review]
Bug 1131712 - Calling find_element() should use constants from marionette.By instead of hard-coded values. r=whimboo

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

This patch looks fine to me, and you covered all the cases. There are only some minor things to update. With that done you will get my r+. Thanks!

For the future I would suggest that you better create a new pull request on the github repository, as uploading the patch directly to Bugzilla. By that you will get Travis CI test results a couple of minutes after. This is pretty helpful to see if your changes haven't regressed something. For a review request here on Bugzilla simply add a normal text attachment which contains the plain URL to the pull request.

::: firefox_puppeteer/tests/test_l10n.py
@@ +3,4 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from marionette.errors import MarionetteException
> +from marionette import By

Please move this line up so it comes before errors. Reason is that this is the global marionette module.

::: firefox_puppeteer/tests/test_places.py
@@ +3,4 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from firefox_ui_harness.testcase import FirefoxTestCase
> +from marionette import By

Imports of modules which have been installed via pip should come before imports of modules of the same repository.

::: firefox_puppeteer/ui/tabbar.py
@@ +3,4 @@
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from marionette import (
> +    Wait, By

Please sort alphabetical.
Attachment #8563222 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

4 years ago
I am not familiar with git, but would like to learn. Would you tell me the steps?
Flags: needinfo?(hskupin)
(Reporter)

Comment 6

4 years ago
Best would be if you could join us on IRC. I don't want that this bug explodes with not-directly related conversations. THanks.
Flags: needinfo?(hskupin)
(Assignee)

Comment 7

4 years ago
Created attachment 8563999 [details] [review]
Bug 1131712 - Calling find_element() should use constants from marionette.By instead of hard-coded values. r=whimboo
Attachment #8563222 - Attachment is obsolete: true
Attachment #8563999 - Flags: review?(hskupin)
(Reporter)

Updated

4 years ago
Attachment #8563999 - Flags: review?(hskupin) → review+
(Reporter)

Comment 8

4 years ago
https://github.com/mozilla/firefox-ui-tests/commit/c3ffa4036bfc255f8437b74bbdb3c4d93806c217
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
(Reporter)

Comment 9

4 years ago
Hi Ankit, are you interested to work on something else? Please let me know and we can certainly find something for you.
Flags: needinfo?(ankit.goyal90)
(Assignee)

Comment 10

4 years ago
Hi Henrik, I am caught into something. Won't be able to contribute for some time. I will tell you when I will be ready to work again.
Flags: needinfo?(ankit.goyal90)
Product: Mozilla QA → Testing
You need to log in before you can comment on or make changes to this bug.