636641 - remote reftest needs better logic for automatically building the remote manifest

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

when we run reftests remotely, we build a url instead of a file path to the manifest file.  The logic was simple in that it assumed you were running reftest from inside the reftest directory.  Now we can run reftest from outside of the directory and it will detect where the webroot is and create the url based off the webroot and the file path you give it.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Attachment: allow remote reftest manifest to be referenced outside of reftest directory (1.0)

simple patch to add more logic to the automatic creation of the manifest URL.

Comment 2

[cmtalbert]

Comment on attachment 514979 [details] [diff] [review]
allow remote reftest manifest to be referenced outside of reftest directory (1.0)

># HG changeset patch
># Parent 1c15ee9f1e1affa47f3be6ded2695161e3d5f3e6
>
>diff --git a/layout/tools/reftest/remotereftest.py b/layout/tools/reftest/remotereftest.py
>--- a/layout/tools/reftest/remotereftest.py
>+++ b/layout/tools/reftest/remotereftest.py
>@@ -372,20 +372,23 @@ def main():
>     reftest = RemoteReftest(automation, dm, options, SCRIPT_DIRECTORY)
> 
>     # Start the webserver
>     reftest.startWebServer(options)
> 
>     # Hack in a symbolic link for jsreftest
>     os.system("ln -s ../jsreftest jsreftest")
> 
>-    # Dynamically build the reftest URL if possible
>+    # Dynamically build the reftest URL if possible, beware that args[0] should exist 'inside' the webroot
Do we have any means to enforce this and return an error if that is not the case?  This code feels like it is making a lot of assumptions that may or may not be true.

>     manifest = args[0]
>-    if os.path.exists(args[0]):
>+    if os.path.exists(os.path.join(SCRIPT_DIRECTORY, args[0])):
>         manifest = "http://" + str(options.remoteWebServer) + ":" + str(options.httpPort) + "/" + args[0]
So if <SCRIPT_DIRECTORY>/args[0] exists, we create http://<server>:<port>/args[0].  This only works because we were given an args[0] that was inside the webroot.  We should actually ensure that we error if we are given an args[0] that is not in our webroot.  I think that means if we were to have a args[0] who's top level directory is not a direct child of SCRIPT_DIRECTORY. That's kind of what the if here is testing, but it's very opaque.  Add a comment?

>+    elif os.path.exists(args[0]):
>+        manifestPath = os.path.abspath(args[0]).split(SCRIPT_DIRECTORY)[1].strip('/')
>+        manifest = "http://" + str(options.remoteWebServer) + ":" + str(options.httpPort) + "/" + manifestPath
In this case args[0] contains the SCRIPT_DIRECTORY and as such we strip that out and just take everything remaining after the SCRIPT_DIRECTORY and append it to the URL.  That's ok, but it's not real clear that's the case.  I'd prefer we make the elif check explicit: elif os.path.exists(args[0]) and (args[0] contains SCRIPT_DIRECTORY): 

There needs to be an else here.  Since this is remote reftest, we need an else that will enforce that we were given a URL, because as this code stands, if we will fall through these two conditions, we send the test engine whatever was on args[0] as its manifest.  We should ensure we're sending something like "http://foo/bar/baz/blah.list" (and that blah.list exists) and not sending something of the form: "foo/bar/baz/blah.list".  

It probably also wouldn't hurt that we verify that the args[0] actually points to a file.

r+ with improved error checking and a few comments so that it's more clear what you're actually checking in these if statements. (Or better, make the if statements more explicit w.r.t. what they are actually checking for).

Comment 3

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=a95e6e054351

Comment 4

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

Attachment: link the jsreftest subdirectory smarter (1.0)

we have a hacked in symlink for jsreftests so we can access it from the webserver.  Now instead of assuming we are in the reftest directory when doing the symlink, we actually create it in the reftest directory.  This works a lot smoother!

Comment 5

[Joel Maher ( :jmaher ) (UTC -8) (PTO'ish until July 5)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=1512e152385e