Closed Bug 1213800 Opened 6 years ago Closed 6 years ago

Rewrite screen capture tests

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

(Keywords: pi-marionette-server)

Attachments

(3 obsolete files)

The screen capture tests for Marionette are not testing that the screenshots are of what they’re meant to be of.  In order to confidently fix bug 1202663, we should address this issue first.
Assignee: nobody → ato
Blocks: 1202663
Bug 1213800: More exhaustive Marionette screen capture tests

The screen capture tests were not testing that the screenshots are of
what they're meant to be of.  In order to confidently fix bug 1202663,
these new tests will ensure we do not have any regressions.

r=jgriffin
Attachment #8672671 - Flags: review?(jgriffin)
https://reviewboard.mozilla.org/r/21767/#review19527

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:71
(Diff revision 1)
> -                         self.marionette.screenshot(element=el))
> +        print rect

Debug code
https://reviewboard.mozilla.org/r/21767/#review19531

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:68
(Diff revision 1)
> -        self.marionette.navigate(test_url)
> -        el = self.marionette.find_element('id', 'red')
> -        self.assertEqual(RED_ELEMENT_BASE64,
> -                         self.marionette.screenshot(element=el))
> -
> +    def get_bounding_box_dimensions(self, el):
> +        rect = self.marionette.execute_script(
> +            "return arguments[0].getBoundingClientRect()", script_args=[el])
> +        print rect
> +        return (rect["width"], rect["height"])

Unnecessary.  Can use HTMLElement.rect for this.
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

https://reviewboard.mozilla.org/r/21767/#review19529

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:101
(Diff revision 1)
> -                          element=el,
> +        doc_el = self.document_element

this line is not needed
Attachment #8672671 - Flags: review?(jgriffin) → review+
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

Bug 1213800: More exhaustive Marionette screen capture tests

The screen capture tests were not testing that the screenshots are of
what they're meant to be of.  In order to confidently fix bug 1202663,
these new tests will ensure we do not have any regressions.

r=jgriffin
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

Bug 1213800: More exhaustive Marionette screen capture tests

The screen capture tests were not testing that the screenshots are of
what they're meant to be of.  In order to confidently fix bug 1202663,
these new tests will ensure we do not have any regressions.

r=jgriffin
It appears that “full” screenshots in chrome context are meant to be of the first <window> object on the <browser>.  The reason this passes on Linux is presumably that the try slaves do not have a window border, whereas the Windows slaves do.

I’ve modified the patch to get the bounding rectangle of the first <window> element on the <browser>.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dd77eb60490
Flags: needinfo?(ato)
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

Bug 1213800: More exhaustive Marionette screen capture tests

The screen capture tests were not testing that the screenshots are of
what they're meant to be of.  In order to confidently fix bug 1202663,
these new tests will ensure we do not have any regressions.

r=jgriffin
Attachment #8672671 - Flags: review?(dburns)
Bug 1202663: Use dispatcher for screen capture command in listener

r=dburns
r=jgriffin
Attachment #8673110 - Flags: review?(jgriffin)
Attachment #8673110 - Flags: review?(dburns)
Bug 1213797: Refactor screen capture and SVG document support

Errors thrown by takeScreenshot used to be silently ignored.  When the
command started using the new dispatching technique in bug 1202663,
it was surfaced we do not support taking screen captures of SVG documents.

Since this is a requirement for Web Platform Tests, this patch corrects
the wrong assumptions about document body and document element.

This patch also significantly refactors the screen capture code, but
only uses the new implementation in contnent space, since some further
modifications are required to use it in chrome.

r=dburns
r=jgriffin
Attachment #8673111 - Flags: review?(jgriffin)
Attachment #8673111 - Flags: review?(dburns)
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

Bug 1213797: Refactor screen capture and SVG document support

Errors thrown by takeScreenshot used to be silently ignored.  When the
command started using the new dispatching technique in bug 1202663,
it was surfaced we do not support taking screen captures of SVG documents.

Since this is a requirement for Web Platform Tests, this patch corrects
the wrong assumptions about document body and document element.

This patch also significantly refactors the screen capture code, but
only uses the new implementation in contnent space, since some further
modifications are required to use it in chrome.

r=dburns
r=jgriffin
https://reviewboard.mozilla.org/r/21845/#review19611

::: testing/marionette/capture.js:22
(Diff revision 2)
> + * @param {Array.<UUID>=} highlights
> + *     Optional array of web element references, around which a border
> + *     will be marked to highlight them in the screenshot.

Should be Array.<Node>.
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

https://reviewboard.mozilla.org/r/21767/#review19609

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:35
(Diff revision 4)
> +        i = base64.decodestring(s)

please use full variable names.

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:41
(Diff revision 4)
> +        w, h = struct.unpack(">LL", i[16:24])

`w` is a special character for pdb please use full variable names

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:61
(Diff revision 4)
> +        s = self.marionette.screenshot()

`s` is a special character for pdb please use full variable name

::: testing/marionette/client/marionette/tests/unit/test_screenshot.py:100
(Diff revision 4)
> -                         base64.b64encode(binary_data))
> +        s = self.marionette.screenshot()

`s` as mentioned above
Attachment #8672671 - Flags: review?(dburns)
Comment on attachment 8673110 [details]
MozReview Request: Bug 1202663: Use dispatcher for screen capture command in listener

https://reviewboard.mozilla.org/r/21843/#review19617
Attachment #8673110 - Flags: review?(dburns) → review+
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

https://reviewboard.mozilla.org/r/21845/#review19613

::: testing/marionette/listener.js:2019
(Diff revision 2)
> -    node = elementManager.getKnownElement(id, curContainer)
> +  let hs = [];

Can we use a more descriptive var name here? highlightEls, or something maybe.
Attachment #8673111 - Flags: review?(jgriffin) → review+
https://reviewboard.mozilla.org/r/21767/#review19609

> please use full variable names.

In transitive code like this, spelling out temporary variable names is really just an exercise in typing veryLongAndAbsolutelyDescriptivePrivate variable names in the Java tradition.  Length is not a virtue in a name; clarity of expression _is_.

I agree it’s important that output variable names and arguments are made understandable, but I generally buy into the arguments made in https://www.lysator.liu.se/c/pikestyle.html.
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

https://reviewboard.mozilla.org/r/21845/#review19619

::: testing/marionette/capture.js:140
(Diff revision 2)
> +  let u = canvas.toDataURL(PNG_MIME);

please give these variables more meaningful names.
Attachment #8673111 - Flags: review?(dburns)
https://reviewboard.mozilla.org/r/21845/#review19619

> please give these variables more meaningful names.

I don’t see how it adds more clarity in the case of `u`, and with some goodwill it can be argued that `e` isn’t obvious because it’s an output variable.
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

Bug 1213797: Refactor screen capture and SVG document support

Errors thrown by takeScreenshot used to be silently ignored.  When the
command started using the new dispatching technique in bug 1202663,
it was surfaced we do not support taking screen captures of SVG documents.

Since this is a requirement for Web Platform Tests, this patch corrects
the wrong assumptions about document body and document element.

This patch also significantly refactors the screen capture code, but
only uses the new implementation in contnent space, since some further
modifications are required to use it in chrome.

r=dburns
r=jgriffin
Attachment #8673111 - Flags: review?(dburns)
https://reviewboard.mozilla.org/r/21845/#review19619

> I don’t see how it adds more clarity in the case of `u`, and with some goodwill it can be argued that `e` isn’t obvious because it’s an output variable.

Addressed this by returning the result from String.substring directly.
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

Bug 1213800: More exhaustive Marionette screen capture tests

The screen capture tests were not testing that the screenshots are of
what they're meant to be of.  In order to confidently fix bug 1202663,
these new tests will ensure we do not have any regressions.

r=jgriffin
r=dburns
Attachment #8672671 - Flags: review?(dburns)
Comment on attachment 8673110 [details]
MozReview Request: Bug 1202663: Use dispatcher for screen capture command in listener

Bug 1202663: Use dispatcher for screen capture command in listener

r=dburns
r=jgriffin
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

Bug 1213797: Refactor screen capture and SVG document support

Errors thrown by takeScreenshot used to be silently ignored.  When the
command started using the new dispatching technique in bug 1202663,
it was surfaced we do not support taking screen captures of SVG documents.

Since this is a requirement for Web Platform Tests, this patch corrects
the wrong assumptions about document body and document element.

This patch also significantly refactors the screen capture code, but
only uses the new implementation in contnent space, since some further
modifications are required to use it in chrome.

r=dburns
r=jgriffin
https://reviewboard.mozilla.org/r/21767/#review19609

> In transitive code like this, spelling out temporary variable names is really just an exercise in typing veryLongAndAbsolutelyDescriptivePrivate variable names in the Java tradition.  Length is not a virtue in a name; clarity of expression _is_.
> 
> I agree it’s important that output variable names and arguments are made understandable, but I generally buy into the arguments made in https://www.lysator.liu.se/c/pikestyle.html.

Renamed to `string` and `image`, respectively.
Attachment #8672671 - Flags: review?(dburns) → review+
Comment on attachment 8672671 [details]
MozReview Request: Bug 1213800: More exhaustive Marionette screen capture tests

https://reviewboard.mozilla.org/r/21767/#review19659
Comment on attachment 8673111 [details]
MozReview Request: Bug 1213797: Refactor screen capture and SVG document support

https://reviewboard.mozilla.org/r/21845/#review19667
Attachment #8673111 - Flags: review?(dburns) → review+
Attachment #8673110 - Flags: review?(jgriffin)
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/eac15c44cf39
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Attachment #8672671 - Attachment is obsolete: true
Attachment #8673110 - Attachment is obsolete: true
Attachment #8673111 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.