Closed Bug 403012 Opened 15 years ago Closed 15 years ago

Make reftests display the output of unexpected passes

Categories

(Testing :: Reftest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smontagu, Assigned: smontagu)

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #287830 - Flags: review?(dbaron)
Attached patch Patch without the tabs (obsolete) — Splinter Review
Attachment #287830 - Attachment is obsolete: true
Attachment #287832 - Flags: review?(dbaron)
Attachment #287830 - Flags: review?(dbaron)
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.
Attachment #287832 - Flags: review?(dbaron) → review-
Attached patch Address review comments (obsolete) — Splinter Review
Assignee: nobody → smontagu
Attachment #287832 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #288725 - Flags: review?(dbaron)
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 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.
Attachment #288725 - Flags: review?(dbaron) → review+
I agree that your version is clearer, and I'll make that change subject to confirmation that the results are the same.
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.
Attachment #288725 - Flags: approval1.9?
Comment on attachment 288725 [details] [diff] [review]
Address review comments

Yep - good stuff!
Attachment #288725 - Flags: approval1.9? → approval1.9+
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)"
Attachment #288725 - Attachment is obsolete: true
in-testsuite-, assuming we aren't yet at the stage of requiring meta-tests for test programs.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Ouch, I checked in with an error: I wrote 
 if (equal)
instead of 
 if (!equal)

Corrected version checked in.
Component: Testing → Reftest
Flags: in-testsuite-
Product: Core → Testing
QA Contact: testing → reftest
You need to log in before you can comment on or make changes to this bug.