573694 - Optionally allow reftest harness to write output to a log file

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[cmtalbert]

Attachment: Reftest Optional log writer v1

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.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 2

[cmtalbert]

(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.

Comment 3

[cmtalbert]

Attachment: Addressing Joel's comments

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.

Comment 4

[cmtalbert]

Attachment: Patch to make reftest log to a file

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.

Comment 5

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

Comment on attachment 458819 [details] [diff] [review]
Patch to make reftest log to a file

r=dbaron

Comment 6

[cmtalbert]

http://hg.mozilla.org/mozilla-central/rev/ebe517d8905c

--> FIXED