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

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

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

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.

Comment 1

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

Attachment: reftest.jar supports file or http

attaching patch that allows for reading data from the network.  This will work if you give it a manifest with http://.

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

optional http way:
python runreftests.py http://joelmaher.com/reftest/tests/layout/reftests/reftest.list

Comment 2

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

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: */ /* 
>+***** BEGIN LICENSE BLOCK *****

Please leave this as it is.

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

>+  var factory = CC["@mozilla.org/scriptableinputstream;1"];
>+  var sis = factory.createInstance(CI.nsIScriptableInputStream);

Local style is:

var sis = CC["@mozilla.org/scriptableinputstream;1"].
              createInstance(CI.nsIScriptableInputStream);

(no "var factory")

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

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

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

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

-    var fis = CC[NS_LOCALFILEINPUTSTREAM_CONTRACTID].
-                  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);
+
+      var fis = CC[NS_LOCALFILEINPUTSTREAM_CONTRACTID].
+                    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 = channel.open();
+      if (channel instanceof Components.interfaces.nsIHttpChannel
+          && channel.responseStatus != 200) {
+        dump("REFTEST TEST-UNEXPECTED-FAIL | | HTTP ERROR : " + 
+          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) {
    ++lineNo;

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

Comment 3

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

Attachment: reftest.jar supports file or http (2)

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.

Comment 4

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

as a  note, this passed on all my try server tests.

Comment 5

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

dbaron: can I get your review on this patch?

Comment 6

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

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

Comment 7

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

Attachment: reftest.jar supports file or http (2.1)

updated with minor nits, carry over the r+.

Thanks for reviewing this!

Comment 8

[cmtalbert]

Landed as 3b33c4c63856