Closed Bug 573694 Opened 11 years ago Closed 11 years ago

Optionally allow reftest harness to write output to a log file

Categories

(Testing :: Reftest, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Reftest Optional log writer v1 (obsolete) — Splinter Review
On Android, we don't have access to the stdout stream.  So, we need reftest to write to a logfile.  This obviously doesn't need to be the case on desktop, so this feature should be optional for normal reftest operation.

This patch re-uses the mochitest log writer in a way that reftest expects and does not hamper "normal" reftest logging at all.  When the log file is specified, we write to both the stdout stream and the log file simply to maintain behavioral expectations with the current reftest behavior.

Joel, what do you think about this approach?  I'd like to get it figured out between you and I before bothering dbaron with a formal review.
Attachment #453007 - Flags: feedback?(jmaher)
Comment on attachment 453007 [details] [diff] [review]
Reftest Optional log writer v1

>diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js
>+// By default we just log to stdout
>+var gWriteLog = dump;
>+

In general, I don't like the name gWriteLog, but a better name is not coming to my mind.  


>@@ -155,16 +158,28 @@ function OnRefTestLoad()
>+      logFile = prefs.getCharPref("reftest.logFile");
>+      if (logFile) {
>+        try {
>+          MozillaFileLogger.init(logFile);
>+          // Set to mirror to stdout as well as the file
>+          gWriteLog = function (msg) {dump(msg); MozillaFileLogger.log(msg);};
>+        }
>+        catch(e) {
>+          // If there is a problem, just use stdout
>+          gWriteLog = dump;
>+        }
>+      }

I really like this approach.  Why are you using log instead of the callback approach?  


>+    MozillaFileLogger.close();

Here I see a close with no conditions around it.  We should verify we have done an init first.  


>diff --git a/layout/tools/reftest/reftest.xul b/layout/tools/reftest/reftest.xul
>     <script type="application/ecmascript" src="quit.js" />
>     <script type="application/ecmascript" src="reftest.js" />
>+    <script type="application/ecmascript" src="MozillaFileLogger.js" />
>     <browser id="browser" flex="1" type="content-primary" style="width: 800px; height: 1000px" />
> </window>

why are we adding MozillaFileLogger.js here?  Is this the only place to include it?


>diff --git a/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js b/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
>+MozillaFileLogger.log = function(msg) {
>+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+  MozillaFileLogger._foStream.write(msg, msg.length);
>+}
>+

why the new .log() function?
Attachment #453007 - Flags: feedback?(jmaher) → feedback+
(In reply to comment #1)
> (From update of attachment 453007 [details] [diff] [review])
> >diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js
> >+// By default we just log to stdout
> >+var gWriteLog = dump;
> >+
> 
> In general, I don't like the name gWriteLog, but a better name is not coming to
> my mind.  
We discussed using dumpLog for this, which I think is a little better.

> 
> 
> >@@ -155,16 +158,28 @@ function OnRefTestLoad()
> >+      logFile = prefs.getCharPref("reftest.logFile");
> >+      if (logFile) {
> >+        try {
> >+          MozillaFileLogger.init(logFile);
> >+          // Set to mirror to stdout as well as the file
> >+          gWriteLog = function (msg) {dump(msg); MozillaFileLogger.log(msg);};
> >+        }
> >+        catch(e) {
> >+          // If there is a problem, just use stdout
> >+          gWriteLog = dump;
> >+        }
> >+      }
> 
> I really like this approach.  Why are you using log instead of the callback
> approach?  
The callback approach assumes that the message is an object (this is the way the output formatting is done in mochitest - the message is actually an object with various bits of information in it, that is then joined to create the "standard" log format).  In reftest, all the messages are just strings and the "standard" log format is enforced by hand.
> 
> 
> >+    MozillaFileLogger.close();
> 
> Here I see a close with no conditions around it.  We should verify we have done
> an init first.  
> 
Doh.  Good catch.  Thanks.
> 
> >diff --git a/layout/tools/reftest/reftest.xul b/layout/tools/reftest/reftest.xul
> >     <script type="application/ecmascript" src="quit.js" />
> >     <script type="application/ecmascript" src="reftest.js" />
> >+    <script type="application/ecmascript" src="MozillaFileLogger.js" />
> >     <browser id="browser" flex="1" type="content-primary" style="width: 800px; height: 1000px" />
> > </window>
> 
> why are we adding MozillaFileLogger.js here?  Is this the only place to include
> it?
> 
yep, this ensures that the code is available to every other piece of javascript running in that window (which is only reftest.js and quit.js)  I should look through quit.js and make sure it doesn't use any dump statements -- I'll look over that.
> 
> >diff --git a/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js b/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
> >+MozillaFileLogger.log = function(msg) {
> >+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> >+  MozillaFileLogger._foStream.write(msg, msg.length);
> >+}
> >+
> 
> why the new .log() function?
As above, this enables us to print a simple string to the log file.  Not at all necessary (or desired) for mochitest, but it's simpler to allow reftest to continue operating the way it has always operated than enforcing the mochitest logging format on reftest.  It seems like a lot of overhead for about 15 hand formatted strings.  Since this is only an optional setting, I'd prefer to leave reftest.js as intact as possible w.r.t. fallout from the change.
Attached patch Addressing Joel's comments (obsolete) — Splinter Review
This addresses Joel's comments about the ability to call MozillaFileLogger's close without actually init'ing the object and having things break.  I fixed this by making it so that MozillaFileLogger won't ever call one of its objects should that object be null.  This way the callers of MozillaFileLogger don't have to worry about its state.
Assignee: nobody → ctalbert
Attachment #453007 - Attachment is obsolete: true
Status: NEW → ASSIGNED
This patch adds an optional ability to log the reftest output to a log file.  This is needed on Android as we do not have access to scrape stdout. For the actual act of writing to the log file, it reuses the Mochitest file logging code, with a simple function for dumping a string straight to the file.

This differs from the previous patches here as the previous patches also contained a bunch of remotereftest code for handling webservers and logging options.  I've split all that out to bug 580418 to make reviews simpler.

Note that the standard behavior of reftest is not affected by these changes.
Attachment #458518 - Attachment is obsolete: true
Attachment #458819 - Flags: review?(dbaron)
Comment on attachment 458819 [details] [diff] [review]
Patch to make reftest log to a file

r=dbaron
Attachment #458819 - Flags: review?(dbaron) → review+
http://hg.mozilla.org/mozilla-central/rev/ebe517d8905c

--> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.