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

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 1

[Ankit Goyal]

Henrik, I am working on it.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Awesome! Thanks for letting me know. I will mark you as assignee of the bug then!

Comment 3

[Ankit Goyal]

Attachment: Bug 1131712 - Calling find_element() should use constants from marionette.By instead of hard-coded values. r=whimboo

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 5

[Ankit Goyal]

I am not familiar with git, but would like to learn. Would you tell me the steps?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Best would be if you could join us on IRC. I don't want that this bug explodes with not-directly related conversations. THanks.

Comment 7

[Ankit Goyal]

Attachment: Bug 1131712 - Calling find_element() should use constants from marionette.By instead of hard-coded values. r=whimboo

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

https://github.com/mozilla/firefox-ui-tests/commit/c3ffa4036bfc255f8437b74bbdb3c4d93806c217

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Hi Ankit, are you interested to work on something else? Please let me know and we can certainly find something for you.

Comment 10

[Ankit Goyal]

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.