403012 - Make reftests display the output of unexpected passes

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Simon Montagu :smontagu]

I've been bitten by this more than once: a test that fails locally passes on tinderbox and I have no idea why, and no recourse except to mark it as passing and move on. This makes it much harder to know what to do when creating future tests for similar issues.

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch without the tabs

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 287832 [details] [diff] [review]
Patch without the tabs

You actually probably want to dump both images, in case it's a != test that unexpectedly passed.  (Or, even better, suppress the doubling in both the cases of an unexpected pass of an == test and an unexpected fail of a != test.)

Also, in reftest-to-html.pl, I don't think $randomresult can be true for any but the first and fourth of the five parts of the if-elsif chain.  The second and third parts should probably error in that case, instead of testing and handling it.  (That's an existing bug, but it's inducing you to add overly-indented code in this patch.)

And could you fix clean-reftest-output.pl to be okay without a space between the "IMAGE" and the ":", at least if you doo the "even better" part of my first comment?

Other than that, this looks good.

Comment 4

[Simon Montagu :smontagu]

Attachment: Address review comments

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 288725 [details] [diff] [review]
Address review comments

>-            if (!test_passed && expected == EXPECTED_PASS) {
>+            if (gURLs[0].equal && !test_passed && expected == EXPECTED_PASS ||
>+                !gURLs[0].equal && test_passed && expected == EXPECTED_FAIL) {
>                 dump("REFTEST   IMAGE 1 (TEST): " + gCanvas1.toDataURL() + "\n");
>                 dump("REFTEST   IMAGE 2 (REFERENCE): " + gCanvas2.toDataURL() + "\n");
>                 dump("REFTEST number of differing pixels: " + differences + "\n");
>+            } else if (gURLs[0].equal && test_passed && expected == EXPECTED_FAIL ||
>+                       !gURLs[0].equal && !test_passed && expected == EXPECTED_PASS) {
>+                dump("REFTEST   IMAGE: " + gCanvas1.toDataURL() + "\n");


This isn't quite right, because you're using |test_passed| for what |equal| really means.

I think what we really want is:

if ((!test_passed && expected == EXPECTED_PASS) ||
    (test_passed && expected == EXPECTED_FAIL)) {
  if (!equal) {
    // dump 2 images here
  } else {
    // dump 1 image here
  }
}

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 288725 [details] [diff] [review]
Address review comments

Er, actually, never mind, they're equivalent.  I think.  Although mine might be a little clearer.

Comment 7

[Simon Montagu :smontagu]

I agree that your version is clearer, and I'll make that change subject to confirmation that the results are the same.

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 288725 [details] [diff] [review]
Address review comments

Requesting approval1.9. This doesn't directly affect the release, but having more information about test failures is going to save developer time and reduce the length of tree closures.

Comment 9

[Mike Schroepfer]

Comment on attachment 288725 [details] [diff] [review]
Address review comments

Yep - good stuff!

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch as checked in

I made the change suggested in comment 5, plus another small change to clean-reftest-output.pl to make it recognize the lines containing "(RESULT EXPECTED TO BE RANDOM)"

Comment 11

[Simon Montagu :smontagu]

in-testsuite-, assuming we aren't yet at the stage of requiring meta-tests for test programs.

Comment 12

[Simon Montagu :smontagu]

Ouch, I checked in with an error: I wrote 
 if (equal)
instead of 
 if (!equal)

Corrected version checked in.