Closed Bug 636641 Opened 11 years ago Closed 11 years ago
remote reftest needs better logic for automatically building the remote manifest
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.
simple patch to add more logic to the automatic creation of the manifest URL.
Assignee: nobody → jmaher
Attachment #514979 - Flags: review?(ctalbert)
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 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 >- if os.path.exists(args): >+ if os.path.exists(os.path.join(SCRIPT_DIRECTORY, args)): > manifest = "http://" + str(options.remoteWebServer) + ":" + str(options.httpPort) + "/" + args So if <SCRIPT_DIRECTORY>/args exists, we create http://<server>:<port>/args. This only works because we were given an args that was inside the webroot. We should actually ensure that we error if we are given an args that is not in our webroot. I think that means if we were to have a args 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): >+ manifestPath = os.path.abspath(args).split(SCRIPT_DIRECTORY).strip('/') >+ manifest = "http://" + str(options.remoteWebServer) + ":" + str(options.httpPort) + "/" + manifestPath In this case args 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) and (args 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 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 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).
Attachment #514979 - Flags: review?(ctalbert) → review+
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!
Attachment #517460 - Flags: review?(ctalbert)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.