Closed Bug 1213008 Opened 6 years ago Closed 3 years ago
Add option to save screenshot to a 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)
I would like to work on this. How I can start please let me know more. Thanks
I feel the Marionette.screenshot API  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.  https://searchfox.org/mozilla-central/rev/0e8eb01368600eb552dd558c83c64a3b6a0b89b8/testing/marionette/client/marionette_driver/marionette.py#1950-1951
Sounds good to me.
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?
(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.
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 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.
Pushed by firstname.lastname@example.org: 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.
You need to log in before you can comment on or make changes to this bug.