Closed
Bug 1364349
Opened 7 years ago
Closed 7 years ago
Remove `Puppeteer.platform` wrapper in favor of `marionette.session_capabilities['platformName']`
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: whimboo, Assigned: vedantc98, Mentored)
Details
(Keywords: good-first-bug, pi-marionette-firefox-puppeteer, Whiteboard: [lang=py])
Attachments
(1 file, 1 obsolete file)
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•7 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•7 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•7 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•7 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•7 years ago
|
Assignee: nobody → dsmith16.mozilla
Status: NEW → ASSIGNED
Reporter | ||
Comment 6•7 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•7 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•7 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•7 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•7 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•7 years 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•7 years 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•7 years ago
|
Attachment #8869938 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
Reporter | ||
Comment 16•7 years 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•7 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49c4cf5d041d Removed platform property of Firefox Puppeteer. r=whimboo
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49c4cf5d041d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•