Closed Bug 544385 Opened 12 years ago Closed 12 years ago

allow reftest to run tests and read manifests from http instead of file system


(Testing :: Reftest, defect)

Not set


(Not tracked)



(Reporter: jmaher, Assigned: jmaher)




(1 file, 2 obsolete files)

in order for running tests on a remote system (specifically for windows mobile), we need to read manifest and data files over the network instead of file system.
attaching patch that allows for reading data from the network.  This will work if you give it a manifest with http://.

old way:
python tests/layout/retests/reftest.list

optional http way:
Assignee: nobody → jmaher
Attachment #425342 - Flags: review?(dbaron)
Comment on attachment 425342 [details] [diff] [review]
reftest.jar supports file or http

>-/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- /
>-/* vim: set shiftwidth=4 tabstop=8 autoindent cindent expandtab: */
>-/* ***** BEGIN LICENSE BLOCK *****
>+/* -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- / /* vim: set shiftwidth=4 tabstop=8 autoindent cindent expandtab: */ /* 

Please leave this as it is.

(Or, if there's a reason you're changing it, please explain.)

>+  var factory = CC[";1"];
>+  var sis = factory.createInstance(CI.nsIScriptableInputStream);

Local style is:

var sis = CC[";1"].

(no "var factory")

>+    while (true)
>+    {
>+      var chunk =;
>+      if (chunk.length == 0)
>+        break;
>+      streamBuf += chunk;
>+    }

Why 512-at-a-time?  Why not instead do:

var available;
while ((available = sis.available()) != 0) {
  streamBuf +=;

Also, I don't see any need for getStreamContent to catch exceptions; if
it just lets the exception propagate up, it should be fine.
ReadManifest throws for all other sorts of errors.

-    if (!url || !url.schemeIs("file"))
-        throw "Expected a file URL for the manifest.";
+    if (!url || !(url.schemeIs("file") || url.schemeIs("http")))
+        throw "Expected a file or http URL for the manifest.";

This should work for any scheme now.  You should just change the
check to
  if (!url)

-    ReadManifest(url);
+    ReadManifest(url, url.scheme);

-function ReadManifest(aURL)
+function ReadManifest(aURL, manifesttype)

No need to add the parameter, ReadManifest can just look at aURL.scheme
(though I don't think it should need to).

>+// Given an http nsIURI, return the parent nsIURI, after
>+// popping 'depth' directories off the end.
>+function GetUrlBase(fileurl, depth) {

You shouldn't need this function at all.  (If you did, there would be
much easier and more accurate ways to write it that didn't involve doing
URL parsing yourself.  In particular, you'd want to depend on nsIURL.)

-                  createInstance(CI.nsIFileInputStream);
-    fis.init(listURL.file, -1, -1, false);
-    var lis = fis.QueryInterface(CI.nsILineInputStream);
+    if (manifesttype == "file") {
+      var line = {value:null};
+      var listURL = aURL.QueryInterface(CI.nsIFileURL);
+                    createInstance(CI.nsIFileInputStream);
+      fis.init(listURL.file, -1, -1, false);
+      var lis = fis.QueryInterface(CI.nsILineInputStream);
+      var lines = [];
+      do {
+        var more = lis.readLine(line);
+        lines.push(line.value);
+      } while (more);
+    } 
+    else if (manifesttype == "http") {
+      var listURL = aURL;
+      var channel = gIOService.newChannelFromURI(aURL);
+      var inputStream =;
+      if (channel instanceof Components.interfaces.nsIHttpChannel
+          && channel.responseStatus != 200) {
+          channel.responseStatus + "\n");
+      }
+      var streamBuf = getStreamContent(inputStream);
+      inputStream.close();
+      var lines = streamBuf.split("\n");
+    }

You should get rid of the manifesttype checks here and just use the http
path in all cases.

Additionally, just printing "TEST-UNEXPECTED-FAIL" doesn't actually cause
a failure.  Given that this is in the code to read the manifest, you
should probably just *throw* the string instead of dumping it.  That will
cause ReadManifest to fail.

Also, the call to split should really be splitting on the regex
/\(\n\|\r\|\r\n\)/ (not sure whether I escaped that correctly; it
might need less escaping).

>+    dump("url: " + aURL + ", total lines: " + lines.count)

Please remove this dump.

>+    for each (line in lines) {
>         ++lineNo;
>-        var str = line.value;
>+        var str = line;

Just make this:

for each (var str in lines) {

(This removes the unneeded |line| and also makes it not be a global variable.)

+            if (manifesttype != "http") {
+              runHttp = true;
+            }

This should just be (twice):

runHttp = (aURL.scheme == "file"); // We can't yet run the local HTTP server
                                   // for non-local reftests.

But this is something that will need to be fixed eventually, since there
are tests that depend on features of the local HTTP server.  (See
layout/reftests/object/ for some examples.)

>+            var parent = runHttp ? GetUrlBase(aURL, httpDepth) : listURL

I can't even tell what you're trying to do here, but the HTTP(..) code
works fine without this.  I think you've misunderstood 'HTTP(..)', which
currently works fine, and which it looks like this patch would break.
See layout/tools/reftest/README.txt, which says:
      With "HTTP", HTTP tests have the restriction that any resource an HTTP
      test accesses must be accessed using a relative URL, and the test and
      the resource must be within the directory containing the reftest
      manifest that describes the test (or within a descendant directory).
      The variants "HTTP(..)", etc., can be used to relax this restriction by
      allowing resources in the parent directory, etc.
I think the line quoted above should be removed and this one below:

>-                            : [gIOService.newURI(items[1], null, listURL)];
>+                            : [gIOService.newURI(items[1], null, parent)];

should instead be like the change above it:

>-            var incURI = gIOService.newURI(items[1], null, listURL);
>+            var incURI = gIOService.newURI(items[1], null, aURL);

Same thing again for the EQUAL and NOTEQUAL Code.

r=dbaron with those things fixed, but I'd like to look at the revised patch

Sorry for the delay in getting to this.
Attachment #425342 - Flags: review?(dbaron) → review-
updated patch to fix feedback in comment #2.  The feedback was straightforward to integrate and this works well (in linux) for regular reftest and remote http reftest.

asking for review from :dbaron for a sanity check on this patch.  Also running patch through try server overnight.
Attachment #425342 - Attachment is obsolete: true
Attachment #431273 - Flags: review?(dbaron)
as a  note, this passed on all my try server tests.
dbaron: can I get your review on this patch?
Comment on attachment 431273 [details] [diff] [review]
reftest.jar supports file or http (2)

>+var gRemote = 0;

0 -> false

>+      gRemote = prefs.getIntPref("reftest.remote");

getIntPref -> getBoolPref

>+    dump("afileurl input: " + aFileURL);

remove this

r=dbaron with those changes
Attachment #431273 - Flags: review?(dbaron) → review+
updated with minor nits, carry over the r+.

Thanks for reviewing this!
Attachment #431273 - Attachment is obsolete: true
Attachment #432050 - Flags: review+
Landed as 3b33c4c63856
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.