Closed Bug 1213008 Opened 4 years ago Closed Last year

Add option to save screenshot to a file

Categories

(Testing :: Marionette, defect, P5)

defect

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: maja_zf, Assigned: vkatsikaros)

Details

(Keywords: good-first-bug, pi-marionette-client, Whiteboard: [lang=py])

Attachments

(1 file, 1 obsolete file)

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.
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)
OS: Unspecified → All
Priority: -- → P5
Hardware: Unspecified → All
Summary: [marionette-driver] Add option to save screenshot to a file → Add option to save screenshot to a file
Whiteboard: [lang=py]
I would like to work on this.

How I can start please let me know more.

Thanks
Flags: needinfo?(mjzffr)
Flags: needinfo?(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
Flags: needinfo?(ato)
Sounds good to me.
Flags: needinfo?(mjzffr)
Assignee: nobody → vkatsikaros
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.
Attachment #8999366 - Flags: review?(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.)
Attachment #8999366 - Flags: review?(ato) → review-
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?
Flags: needinfo?(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.
Flags: needinfo?(ato)
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
Attachment #8999366 - Attachment is obsolete: true
Attachment #9003708 - Flags: review?(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.
Attachment #9003708 - Flags: review?(ato) → review+
Reminder for myself to push to try when it opens.
Flags: needinfo?(ato)
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cae8cdc1e24a
Add Marionette client option to save screenshot to a file. r=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.
https://hg.mozilla.org/mozilla-central/rev/cae8cdc1e24a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.