Closed Bug 1089753 Opened 8 years ago Closed 8 years ago

Make WindowSnapshot.js's assertSnapshots() print out the number of differing pixels and their maximum difference

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file)

Follow-up from bug 1087224.
Attached patch patchSplinter Review
This makes us use the same format as normal Reftest output.
Attachment #8512105 - Flags: review?(dholbert)
Comment on attachment 8512105 [details] [diff] [review]
patch

>+++ b/testing/mochitest/tests/SimpleTest/WindowSnapshot.js
>   if (!correct) {
>-    var report = "REFTEST TEST-UNEXPECTED-FAIL | " + s1name + " | image comparison (" + sym + ")\n";
>+    var report = "REFTEST TEST-UNEXPECTED-FAIL | " + s1name +
>+                 " | image comparison (" + sym + "), max difference: " +
>+                 numDifferentPixels + ", number of differing pixels: " +
>+                 maxDifference + "\n";

Please add a comment above this saying something like:
  // The language / format in this message should match the failure
  //  messages displayed by reftest.js's "RecordResult()" method.
Comment on attachment 8512105 [details] [diff] [review]
patch

Looks good; r=me with that comment added.
Attachment #8512105 - Flags: review?(dholbert) → review+
A few other tangential thoughts about contextual code:

 (1) In the unlikely event that "gWindowUtils" is undefined/falsey, we should perhaps fail an "ok()" & bail out immediately, rather than silently proceeding and reporting that the canvas-comparison failure was incorrect.

 (2) Similarly: if the canvas sizes are different, we just behave like the compareCanvases call failed.  That seems perhaps broken... seems like we should really treat that as a bad thing (e.g. fail an ok()). If any test actually depends on comparing two canvases of different sizes & relies on them comparing as not-equal, that seems like something's broken in the test. (Not a very useful comparison.)

 (3) If we happened to address (1) and (2) with early-returns, then I think you wouldn't have to pre-initialize "var maxDifference = { value: 0 };" in this patch -- you could leave it as {}.  That may be messier than it's worth, though, since the return statement for this function is a bit complicated.  I do think (1) and (2) are worth spamming an ok()-failure for, though (perhaps in a followup bug).
Comment on attachment 8512105 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2a4049b9e9

(In reply to Daniel Holbert [:dholbert] from comment #4)
>  (1) In the unlikely event that "gWindowUtils" is undefined/falsey, we
> should perhaps fail an "ok()" & bail out immediately, rather than silently
> proceeding and reporting that the canvas-comparison failure was incorrect.

We don't exactly continue and report the canvas-comparison as failing. Rather we go on to get the data URIs and check if they are equal.

I think handling lack of gWindowUtils is probably intentional to allow this helper to work in files that don't have chrome privileges, so I left that check in there.

>  (2) Similarly: if the canvas sizes are different, we just behave like the
> compareCanvases call failed.

I added an ok(fails,...) and early for this.
(In reply to Jonathan Watt [:jwatt] from comment #5)
> We don't exactly continue and report the canvas-comparison as failing.
> Rather we go on to get the data URIs and check if they are equal.

Oh, you're right. I didn't read that code thoroughly enough. Never mind about that, then.

> I added an ok(fails,...) and early for this.

Thanks!
Looks like at least one of those failing tests, test_bug113934.xul, does indeed intentionally try to compare canvases of different sizes (but I haven't poked around far enough to determine whether that's sane/useful):

> 51     function doTheTest() {
> 52       var s1 = snapshotWindow($("f1").contentWindow);
> 53       var s2 = snapshotWindow($("f2").contentWindow);
> 54       var s3 = snapshotWindow($("f3").contentWindow);
> 55 
> 56       ok(!compareSnapshots(s2, s3, true)[0],
> 57          "Should look different due to different sizing");

http://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/bug113934_window.xul#57
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Boris, you added the "Should look different due to different sizing" line in:

http://hg.mozilla.org/mozilla-central/rev/9dd58a43924c

The implementation of compareSnapshots means that it will return false without actually comparing the canvases. Essentially this check just checks that the width/height properties of the canvases are different. Can we just remove this check, or else just change it to a direct dimensions check?
Flags: needinfo?(jwatt) → needinfo?(bzbarsky)
Hmm.  This was intended to be a check for the actual rendering.  In particular, we first claim that f3 and f2 look different, then swap f3 and f1 and then claim that now f1 and f2 look the same, which means we did a relayout after the swap.

It looks like this wasn't actually testing the relayout bit, sadly.  Or at least wasn't testing the control condition of actually having a different size leading to a different layout....
Flags: needinfo?(bzbarsky)
Comment on attachment 8512105 [details] [diff] [review]
patch

Okay. I relanded this with the broken test line commented out and with a comment pointing to bug 1090274 which I opened for the issue.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd72497fcaf0
https://hg.mozilla.org/mozilla-central/rev/fd72497fcaf0
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.