Remove `Puppeteer.platform` wrapper in favor of `marionette.session_capabilities['platformName']`

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: whimboo, Assigned: vedantc98, Mentored)

Tracking

({good-first-bug, pi-marionette-firefox-puppeteer})

Version 3
mozilla58
good-first-bug, pi-marionette-firefox-puppeteer
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [lang=py])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Firefox Puppeteer has a wrapper for `self.marionette.session_capabilities['platformName']` which can be accessed via `self.puppeteer.platform`. I would propose to get rid of this extra layer which doesn't serve underlying purpose of puppeteer for ui classes. 

The property can be found here:

https://dxr.mozilla.org/mozilla-central/rev/3b96f277325747fe668ca8cd896d2f581238e4ee/testing/marionette/puppeteer/firefox/firefox_puppeteer/puppeteer.py#53

Also the files in /testing/firefox-ui have to be updated for this change.

Please see the following document in how to get started:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

Comment 1

2 years ago
I'm a student from Western Oregon University and my group and in Open Source Software Development would like to claim this bug if possible.

Comment 2

2 years ago
(In reply to dsmith16.mozilla from comment #1)
> I'm a student from Western Oregon University and my group and in Open Source
> Software Development would like to claim this bug if possible.

Correction: I'm a student from Western Oregon University and my group and I in Open Source Software Development would like to claim this bug if possible.
(Reporter)

Comment 3

2 years ago
Feel free to work on it. Once a patch has been submitted via mozreview I will assign you to this bug. Otherwise let me know if you have questions.
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review146084

::: commit-message-9851f:1
(Diff revision 1)
> +Bug1364349  - 'Removed 'Puppeteer.platform' wrapper in favor of 'marionette.session_capabilities['platformName']'' r=hskupin@gmail.com

Please fix this commit message.  There should be a space after “Bug”, not two spaces but one before the dash.  Then the summary should not be wrapped in single quotes, and it should ideally not be longer than ~50 characters.  Finally, the reviewer should be listed as "r=whimboo" or "r?whimboo".  mozreview will rewrite the commit message from "r?" to "r=" when the changeset is sent to Autoland.
(Reporter)

Updated

2 years ago
Assignee: nobody → dsmith16.mozilla
Status: NEW → ASSIGNED
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review146108

Please note that when you want to have a review you should set the reviewer correctly in the commit message, or within mozreview directly.

The patch looks fine, but would need small updates.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:72
(Diff revision 1)
>                             'shortcut',
>                             opener,
>                             )
>  
>          for trigger in open_strategies:
> -            if trigger == 'shortcut' and self.puppeteer.platform == 'windows_nt':
> +            if trigger == 'shortcut' and self.marionette.session_capabilities['platformName'] == 'windows_nt':

This line is too long and will cause a linting failure. Please wrap it appropriately.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:95
(Diff revision 1)
>                              'shortcut',
>                              closer,
>                              )
>          for trigger in close_strategies:
>              # menu only works on OS X
> -            if trigger == 'menu' and self.puppeteer.platform != 'darwin':
> +            if trigger == 'menu' and self.marionette.session_capabilities['platformName'] != 'darwin':

Same here. Please wrap the line.

Comment 7

2 years ago
OK, I'll have a corrected patch up sometime in the next couple days. I'll also make sure the commit message matches the guidelines you provided. Thanks much!
Comment hidden (mozreview-request)
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review147774

Sorry, but the indentation is still wrong. I would suggest that you install a linter (like flake8) locally and let it check the modified files. Also please have a look at the commit message again. It should contain the `what` has been changed in the summary, and `why` in the details. If you need help with the latter please let me know on IRC and we can figure it out.
Attachment #8869938 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)

Comment 11

2 years ago
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8869938 [details]
> Bug 1364349 - Removed Puppeteer.platform and references as requested.
> 
> https://reviewboard.mozilla.org/r/141506/#review147774
> 
> Sorry, but the indentation is still wrong. I would suggest that you install
> a linter (like flake8) locally and let it check the modified files. Also
> please have a look at the commit message again. It should contain the `what`
> has been changed in the summary, and `why` in the details. If you need help
> with the latter please let me know on IRC and we can figure it out.

Updated. I ran flake8 over the files before submission. Hopefully, the commit message is acceptable. I tried to keep it at ~50 characters as recommended by :ato (currently 54 characters with spaces). Let me know if I missed anything or how I can improve the commit message if necessary. 

Thank you so much for all your patience and help with this process!
(Reporter)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review150274

::: commit-message-9851f:1
(Diff revisions 2 - 3)
> -Bug 1364349 - Removed wrapper and references in found firefox-ui r?whimboo
> +Bug 1364349 - Removed Puppeteer.platform and references as requested. r?whimboo

The description of the commit should contain what has been changed. "as requested" doesn't give any value for a person looking just at the commit message.

Please try to make it clearer.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:74
(Diff revisions 2 - 3)
>                             )
>  
>          for trigger in open_strategies:
> -            if (trigger == 'shortcut' 
> -                    and self.marionette.session_capabilities['platformName'] 
> +            if (trigger == 'shortcut'
> +                    and self.marionette.session_capabilities['platformName']
>                      == 'windows_nt'):

I still don't like the wrapping here, and I doubt the linter will be also happy about.

So better define a variable before the `for` loop, and use it here.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:99
(Diff revisions 2 - 3)
>                              )
>          for trigger in close_strategies:
>              # menu only works on OS X
> -            if (trigger == 'menu' 
> -                    and self.marionette.session_capabilities['platformName'] 
> +            if (trigger == 'menu'
> +                    and self.marionette.session_capabilities['platformName']
>                      != 'darwin'):

Same as above.
Attachment #8869938 - Flags: review?(hskupin) → review-
(Reporter)

Comment 13

a year ago
dsmith, do you have an update for us? It would be great to get this finally landed. Thanks.
Flags: needinfo?(dsmith16.mozilla)
(Reporter)

Comment 14

a year ago
No reponse from assignee. I'm going to put this bug back into the pool for someone else to pick up the work.
Assignee: dsmith16.mozilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(dsmith16.mozilla)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8869938 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8911713 [details]
Bug 1364349 - Removed platform property of Firefox Puppeteer.

https://reviewboard.mozilla.org/r/183114/#review188324

Code-wise it looks fine. Just update the commit message and we can get it landed. Meanwhile I will trigger a try build so we can ensure nothing will be broken with the landing.

::: commit-message-7e962:1
(Diff revision 1)
> +Bug 1364349 - Removed the wrapper puppeteer.platform in Firefox UI Tests. r=whimboo

I would say "Bug 1364349 - Removed platform property of Firefox Puppeteer".

::: commit-message-7e962:3
(Diff revision 1)
> +Bug 1364349 - Removed the wrapper puppeteer.platform in Firefox UI Tests. r=whimboo
> +
> +The wrapper puppeteer.platform in Firefox UI tests was removed and replaced with marionette.session_capabilities['platformName'].

For the detailed description we usually explain why it was necessary. So as best just say that this wrapper is not needed anymore, or like in this case we won't need a detailed description at all.
Attachment #8911713 - Flags: review?(hskupin) → review+
Comment hidden (mozreview-request)

Comment 18

a year ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49c4cf5d041d
Removed platform property of Firefox Puppeteer. r=whimboo

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49c4cf5d041d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.