1213008 - Add option to save screenshot to a file

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect, P5)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The Marionette class currently has a screenshot method that just returns the resulting PNG image data. This method should optionally save the data to a specific file path.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I was going to close this as WONTFIX, but perhaps having a method
that lets you write to a file handle would be a good idea so that
the following would be possible:

> with open("file.png", "w") as fh:
>     marionette.take_screenshot(fh)

Comment 2

[Manish [:manishkk]]

I would like to work on this.

How I can start please let me know more.

Thanks

Comment 3

[Andreas Tolfsen ❲:ato❳]

I feel the Marionette.screenshot API [1] is pretty overloaded as it is:

>     def screenshot(self, element=None, highlights=None, format="base64",
>                    full=True, scroll=True):

My personal preference would be to add a new method on the class,
for example:

> Marionette.save_screenshot(self, fh, element=None, highlights=None,
>                            full=True, scroll=True)

This would call Marionette.screenshot internally but instead of
returning the response body, it would write it to the file handle
(fh).

I’ll let maja_zf decide whether this is a good idea.

  [1] https://searchfox.org/mozilla-central/rev/0e8eb01368600eb552dd558c83c64a3b6a0b89b8/testing/marionette/client/marionette_driver/marionette.py#1950-1951

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Sounds good to me.

Comment 5

[Vangelis Katsikaros]

Attachment: 0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

I couldn't push to try as I temporarily don't have access to my commit access key, sorry for this. Also, this is the fist time I use git (git-cinnabar) so I might have not formatted the patch properly?

Happy to add a test as well if this looks ok.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8999366 [details] [diff] [review]
0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

Review of attachment 8999366 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me, but I would like to see some tests for it.  Do
you think you could manage that?

When uploading patches to Bugzilla using git, you might be interested
in git-bz from https://github.com/mozilla/moz-git-tools.  I use it
this way:

> % git bz attach -e central..HEAD

(central is my branch point and “master” branch, but generally it
just takes a revset range.)

Comment 7

[Vangelis Katsikaros]

Thanks for the input Andreas! Yes, I'll give a go at test(s).

I would like to ask some questions:
1) I expect the test that needs expansion is testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py ?
2) I plan to compare a) the content of a file saved with 'save_screenshot()' with the data from 'screenshot(format="binary")'. How does that sound?
3) If the above sounds ok, is there some functionality in harness around creating temporary files? 
4) I see that most tests (ie "def test_*" in testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py) do "something" in the beginning, such as:
> dialog = self.open_dialog()
or
> self.marionette.navigate(long)
should I be interested in that?

Comment 8

[Andreas Tolfsen ❲:ato❳]

(In reply to Vangelis Katsikaros from comment #7)

> 1) I expect the test that needs expansion is
> testing/marionette/harness/marionette_harness/tests/unit/test_scre
> enshot.py ?

That seems like a reasonable place to put it.

> 2) I plan to compare a) the content of a file
> saved with 'save_screenshot()' with the data from
> 'screenshot(format="binary")'. How does that sound?

That sounds perfect.  All you need to do is ensure there is data
parity, as the particularities of take_screenshot is already tested.

> 3) If the above sounds ok, is there some functionality in harness
> around creating temporary files?

As the Marionette unit tests are written with unittest
(sigh) you can use tempfile from the standard library:
https://docs.python.org/2/library/tempfile.html

For example:

> with tempfile.TemporaryFile() as fh:
>     fh.write(…)

> 4) I see that most tests (ie "def test_*" in
> testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py)
> do "something" in the beginning, such as:
> 
> > dialog = self.open_dialog()
> 
> or
> 
> > self.marionette.navigate(long)
> 
> should I be interested in that?

The tests that do this are testing that it’s possible to take
screenshots of chrome dialogues in Firefox.  Since this is already
tested by the various take_screenshot tests, you don’t need to
explicitly test for that.

Comment 9

[Vangelis Katsikaros]

Attachment: 0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

Thanks for the tips Andreas. I still haven't got access to my bugzilla related keys, so I provide a patch and can't push to try.

These commands are successful:
> ./mach test testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py
> ./mach lint testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py \
> testing/marionette/client/marionette_driver/marionette.py

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9003708 [details] [diff] [review]
0001-Bug-1213008-Add-option-to-save-screenshot-to-a-file.patch

Review of attachment 9003708 [details] [diff] [review]:
-----------------------------------------------------------------

Look OK to me, pending a successful try run.  It appears all the
test infrastructure is down today, so I’ll push a try run later.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Reminder for myself to push to try when it opens.

Comment 12

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebdefb63c2fd9c9e5cf9abb0e81ffcab4b2c6c37

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae8cdc1e24a
Add Marionette client option to save screenshot to a file. r=ato

Comment 14

[Andreas Tolfsen ❲:ato❳]

Sorry it took me so long to get around to pushing this.  I’ve been
having various email related problem over the last two days.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cae8cdc1e24a

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Moving bugs for Marionette client due to component changes.