Closed
Bug 996236
Opened 10 years ago
Closed 10 years ago
screenshot() should allow decoding from base64 automatically
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: wlach, Assigned: parkouss)
Details
(Keywords: pi-marionette-runner, Whiteboard: [good first bug][mentor=wlach][lang=python])
Attachments
(1 file, 2 obsolete files)
5.31 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
Right now marionette's screenshot() member returns a screenshot as a base64 string: http://marionette-client.readthedocs.org/en/latest/#marionette.Marionette.screenshot This is the right thing for some applications, but it would be nice to be able to automatically decode it to a binary png if (e.g.) saving the screenshot to disk for further inspection. I propose changing the API to include a keyword argument called "format" which can take one of two possible values: "base64" (the default) and "binary". If it is "base64", return the screenshot as a base64-string as we do now. If "binary", convert to binary as follows first using something like this: base64.b64decode(screenshot_data.encode('ascii')) Documentation, tests, etc should also be updated. Pointer to source file: http://hg.mozilla.org/mozilla-central/file/83ae54e18689/testing/marionette/client/marionette/marionette.py#l1378 Pointer to tests: http://hg.mozilla.org/mozilla-central/file/83ae54e18689/testing/marionette/client/marionette/tests/unit/test_screenshot.py Instructions on setting up marionette for development: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Developer_setup
Updated•10 years ago
|
Keywords: ateam-marionette-runner
Assignee | ||
Comment 1•10 years ago
|
||
I have added tests for this feature. The doc must be automatically updated since the screenshot() method was already taken in account in the doc/index.rst file. I just updated docstring and I think it will do the job.
Attachment #8420067 -
Flags: review?(wlachance)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8420067 [details] [diff] [review] add format parameter to screenshot method, allowing to decode b64 automatically Review of attachment 8420067 [details] [diff] [review]: ----------------------------------------------------------------- Awesome. I have some minor requests for updates. Could you make these changes and upload a new patch? ::: testing/marionette/client/marionette/marionette.py @@ +1399,5 @@ > box around in the returned screenshot. > + > + :param format: if "base64" (the default), returns the screenshot > + as a base64-string. If "binary", the data is decoded and > + returned as a raw binary string. a raw binary string -> raw binary ::: testing/marionette/client/marionette/tests/unit/test_screenshot.py @@ +27,5 @@ > + test_url = self.marionette.absolute_url('html5Page.html') > + self.marionette.navigate(test_url) > + el = self.marionette.find_element('id', 'red') > + binary_data = self.marionette.screenshot(element=el, format="binary") > + self.assertEqual('iVBORw0KGgoAAAANSUhEUgAAADIAAAAyCAYAAAAeP4ixAAAAVUlEQVRoge3PsQ0AIAzAsI78fzBwBhHykD2ePev80LweAAGJB1ILpBZILZBaILVAaoHUAqkFUgukFkgtkFogtUBqgdQCqQVSC6QWSC2QWiC1QGp9A7ma+7nyXgOpzQAAAABJRU5ErkJggg==', Could you refactor this test a bit so we define the base64 data as constants in the top of the file? e.g. RED_ELEMENT_BASE64='iVBORw0KGgoAAAANSUhEUgAAAD...' GREEN_ELEMNT_BASE64='iVBORw0KGgoAAAANSUhEUgAAAD...' I think it would make things easier to read. @@ +34,5 @@ > + def testNotAllowedScreenshotFormatRaiseValueError(self): > + test_url = self.marionette.absolute_url('html5Page.html') > + self.marionette.navigate(test_url) > + el = self.marionette.find_element('id', 'red') > + try: I think I'd prefer a simple assertRaises here. I don't think we need to care about the exact string in the exception. http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_execute_async_script.py#22
Attachment #8420067 -
Flags: review?(wlachance) → review+
Assignee | ||
Comment 3•10 years ago
|
||
THanks for the feedback! I have corrected following your instructions.
Attachment #8420067 -
Attachment is obsolete: true
Attachment #8420231 -
Flags: review?(wlachance)
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8420231 [details] [diff] [review] 2: add format parameter to screenshot method, allowing to decode b64 automatically Review of attachment 8420231 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, you can actually carry forward the r+ in this case, since the changes were minor.
Attachment #8420231 -
Flags: review?(wlachance) → review+
Reporter | ||
Comment 5•10 years ago
|
||
Let's run this through try, just to be safe: https://tbpl.mozilla.org/?tree=Try&rev=3e2b64d4e977
Reporter | ||
Comment 6•10 years ago
|
||
Attachment #8420231 -
Attachment is obsolete: true
Attachment #8421033 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 7•10 years ago
|
||
Try run looks good, let's check this in!
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba8e1ed6e710
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
•