Closed Bug 996236 Opened 10 years ago Closed 10 years ago

screenshot() should allow decoding from base64 automatically

Categories

(Remote Protocol :: Marionette, defect)

x86_64
Linux
defect
Not set
normal

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)

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
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)
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+
THanks for the feedback!

I have corrected following your instructions.
Attachment #8420067 - Attachment is obsolete: true
Attachment #8420231 - Flags: review?(wlachance)
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+
Let's run this through try, just to be safe: https://tbpl.mozilla.org/?tree=Try&rev=3e2b64d4e977
Attachment #8420231 - Attachment is obsolete: true
Attachment #8421033 - Flags: review+
Keywords: checkin-needed
Try run looks good, let's check this in!
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8e1ed6e710
Assignee: nobody → j.parkouss
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba8e1ed6e710
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: