Closed Bug 466372 Opened 12 years ago Closed 12 years ago

Add a global result summary to RefTest output

Categories

(Testing :: Reftest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

(3) Mochitest already have this.
(Though I didn't see where it is done.)

RefTest has TinderboxPrint only.
Attached patch (Av1) Add summary to reftest.js (obsolete) — Splinter Review
Unrelated nit:
*Removes an extra space too.

***

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/5d37678a2482
+http://hg.mozilla.org/comm-central/rev/892e9f783d9e)

Output example:
{
[...]
REFTEST TEST-PASS | file:///[...] | 
REFTEST INFO | Pass: 2320
REFTEST INFO | Fail: 2
REFTEST INFO | RandomAndKF: 109
REFTEST INFO | LoadOnly: 4
REFTEST INFO | Skip: 2
REFTEST INFO | FailedLoad: 0
REFTEST INFO | Exception: 0
REFTEST FINISHED: Slowest test took 2012ms (file:///[...])
}

I'd like to vertically align the numbers (as Mochitest does) but I don't know how to do it (simply).

Fwiw, the figures are a little different from the (Windows SeaMonkey) waterfall:
{
TinderboxPrint: reftest<br/>2326/0/93
}
I would guess
2326 = Pass + Fail (which don't on the tindrboxes) + LoadOnly
93 = +/- "Known Fail" (without the "Random only") + Skip
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #349644 - Flags: review?
Attachment #349644 - Flags: review? → review?(ted.mielczarek)
Component: Testing → Layout: Misc Code
QA Contact: testing → layout.misc-code
Attachment #349644 - Flags: review?(ted.mielczarek) → review?(dbaron)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122
SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/5d37678a2482
+http://hg.mozilla.org/comm-central/rev/892e9f783d9e)

Av1, with:
*Skip dumping results with no tests;
 and reorder results accordingly.
*Unrelated nit: gSlowestTestURL initialization.
Attachment #349644 - Attachment is obsolete: true
Attachment #349730 - Flags: review?(dbaron)
Attachment #349644 - Flags: review?(dbaron)
Av1a, with another unrelated extra space removal.
Attachment #349730 - Attachment is obsolete: true
Attachment #349740 - Flags: review?(dbaron)
Attachment #349730 - Flags: review?(dbaron)
Comment on attachment 349740 [details] [diff] [review]
(Av1b) Add dynamic summary to reftest.js


>+var gTestResults = {Exception: 0, FailedLoad: 0, Fail: 0,
>+                    Pass: 0, RandomAndKF : 0, LoadOnly: 0, Skip: 0};

Please write this using the style:

var gTestResults = {
  Exception: 0,
...
};

>-var gSlowestTestURL;
>+var gSlowestTestURL = null;

This isn't needed.

>+        ++gTestResults["Exception"];

Please write all increments of this form as
  ++gTestResults.Exception;

>+    for (var result in gTestResults)
>+      if (gTestResults[result] != 0)
>+        dump("REFTEST INFO | " + result + ": " + gTestResults[result] + "\n");
>     dump("REFTEST FINISHED: Slowest test took " + gSlowestTestTime +
>          "ms (" + gSlowestTestURL + ")\n");

Please put your new information after the "REFTEST FINISHED" message.

>-            var result = "REFTEST " + outputs[expected][test_passed] + " | ";
>-            result += gURLs[0].prettyPath + " | "; // the URL being tested
>-            if (!gURLs[0].equal) {
>-                result += "(!=) ";
>-            }
>+            var result = "REFTEST " + outputs[expected][test_passed] + " | " +
>+                         gURLs[0].prettyPath + " |"; // the URL being tested
>+            if (!gURLs[0].equal)
>+                result += " (!=)";
>             dump(result + "\n");

Please don't add unnecessary changes.  (Some log parsers might even depend on the trailing space.)

>             if (!test_passed && expected == EXPECTED_PASS ||
>                 test_passed && expected == EXPECTED_FAIL) {
>+                ++gTestResults["Fail"];
>                 if (!equal) {
>                     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 {
>                     dump("REFTEST   IMAGE: " + gCanvas1.toDataURL() + "\n");
>                 }
>+            } else {
>+                if (test_passed && expected == EXPECTED_PASS)
>+                    ++gTestResults["Pass"];
>+                else
>+                    ++gTestResults["RandomAndKF"];
>             }

Please keep the what-to-increment logic separate from the whether-to-dump-images logic.


"KF" is unclear, you should call it "KnownFail".  And I'm not sure why you want to count Random and Known Fail together.  (Or why you want to count Unexpected Fail and Unexpected Pass together and call them "Fail".  If you want to lump them together you should call them "Unexpected".)
Attachment #349740 - Flags: review?(dbaron) → review-
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081129 Minefield/3.1b3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/73d9cfe0174c)

Av1b, with comment 4 suggestion(s);
plus added summary title line.
Attachment #349740 - Attachment is obsolete: true
Attachment #350887 - Flags: review?(dbaron)
Comment on attachment 350887 [details] [diff] [review]
(Av2) Add dynamic summary to reftest.js
[Checkin: Comment 9 & 10]

r=dbaron
Attachment #350887 - Flags: review?(dbaron) → review+
Nit, while here:
Shouldn't
{
            var outputs = {};
            const randomMsg = "(EXPECTED RANDOM)";
            ...
}
be global to the file rather than local to the function ?
(They are invariant and I guess that should improve/speed things up a little...)
No.  They're not needed outside the function.  |outputs| could probably be written as a const rather than a var, and in a single expression, though, but that should NOT be done as part of this bug.
Keywords: checkin-needed
Whiteboard: [c-n: 1.9.2 then 1.9.1]
Comment on attachment 350887 [details] [diff] [review]
(Av2) Add dynamic summary to reftest.js
[Checkin: Comment 9 & 10]

http://hg.mozilla.org/mozilla-central/rev/0f32649fe11b
Attachment #350887 - Attachment description: (Av2) Add dynamic summary to reftest.js → (Av2) Add dynamic summary to reftest.js [Checkin: Comment 9]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: 1.9.2 then 1.9.1] → [c-n: baking for 1.9.1]
Comment on attachment 350887 [details] [diff] [review]
(Av2) Add dynamic summary to reftest.js
[Checkin: Comment 9 & 10]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7c76bca7adb
Attachment #350887 - Attachment description: (Av2) Add dynamic summary to reftest.js [Checkin: Comment 9] → (Av2) Add dynamic summary to reftest.js [Checkin: Comment 9 & 10]
Keywords: fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1]
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1b3
Blocks: 468023
(In reply to comment #1)
> Fwiw, the figures are a little different from the (Windows SeaMonkey)
> waterfall:

I filed bug 468023.

(In reply to comment #8)
> |outputs| could probably be written as a const rather than a var, and in a single expression,

I filed bug 468024.
Component: Layout: Misc Code → Reftest
Product: Core → Testing
Target Milestone: mozilla1.9.1b3 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.