this is sort of a chicken and egg scenario, but we need an option to specify a remote server where the tests live. This will need to be fixed in the profile creation that automation.py does. The problem is that the profile needs to be created on a machine and the tests will live on another machine. This sort of means that runtests.py will not always run the tests end to end. For devices that do (or should) not run python, such as windows mobile/ce, we need to run remotely and copy a generated profile to the device. We could assume this would work in such a fashion where you would copy the tests to machine and server, then call runtests.py with the --remote-webserver=192.168.1.1:8888 option and the tests would run.

patch to add two cli options to mochitest: 1) --remote-webserver 2) --setup-only This allows for me to run: python runtests.py --appname=<path>/firefox.exe --remote-webserver=192.168.55.100:8888 --setup-only Then copy mochitestprofile/* <device>:/tests/ Not asking for review on this yet. Still want to verify this is the right end to end solution for both tegra and windows mobile.

Note that some Mochitests rely on the server on which they're run being 127.0.0.1. It may be necessary to use PAC to override 127.0.0.1:8888, but to be honest I don't even know if that's possible. http://hg.mozilla.org/mozilla-central/file/d2b9effde892/dom/tests/mochitest/dom-level0/test_setting_document.domain_to_shortened_ipaddr.html

Thanks Jeff, that is one of the nasty mochitests for me to figure out. I plan to have an analysis of all the problematic tests before too long. Unfortunately I can not even pass the MochiKit_Unit_Tests with the remote server...one step at a time.

ability to add a remote web server by setting up the profile correctly.

This patch may or may not be any good. I can't really compare it to trunk as the same merge issues apply there and if I made a mistake merging it to branch, chances are good I'll make the same mistake merging it to trunk. So, Joel, take a look here and see what you think. These are my questions: * In automation.py.in looks like the "God Access" was granted to all URLs that are used in mochitest so part of this patch is likely redundant. See: http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#278 * After getting everything merged in, I ran a test on a normal mochitest setup (with a remote server serving the tests). I ran: python runtests.py --remote-webserver=10.250.5.119:8888 and when the mochitests started up they were using localhost:8888. So, perhaps I missed something or perhaps the original patch had the same behavior. What do you expect to happen in this case? I'd expect the mochitests to start up and point at the given server.

updated patch tested on 1.9.2 and trunk (for bitrot and functionality) that allows for adding a --remote-webserver option to the cli which takes an ip address (--remote-webserver=192.168.1.1), or optionally an ip:port (--remote-webserver=192.168.1.1:3141). This does a few things logically: 1) configures the profile (user.js) to use the PAC and avoid the "remote script is trying to execute unsafe code" warning. 2) ignore the localhost:8888 in the server-locations.txt if this is specified 3) replace the global variables in runtests.py that have localhost:8888 hardcoded

Please toss this in my review queue as well when you've got something that you think is about ready.

Comment on attachment 406076 [details] [diff] [review] remote proxy autoconfig and hooks for using a remote web server ted, please take some time to review these changes. They are ready for prime time and will help us out greatly once they are checked in.

Comment on attachment 406076 [details] [diff] [review] remote proxy autoconfig and hooks for using a remote web server I think Waldo needs to take a pass at this as well, since it touches the PAC stuff so heavily. Just a thought--if we defined the primary server location as a fake hostname called 'localserver' or something, and search-and-replaced localhost with that, and then just kept the s/127.0.0.1/remoteip/ parts of this, would that make most of your problems go away? More specific comments: +defaultWebServer = "localhost:8888" Use all caps for file-global constant-type variables like this. +def readLocations(locationsPath = "server-locations.txt", remoteServer = defaultWebServer): I'm not sure how I feel about these changes. I'd rather Waldo chimed in on it, though. -user_pref("geo.wifi.uri", "http://localhost:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs"); +user_pref("geo.wifi.uri", "http://%(webserver)s/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs"); I'd rather see you just mass change these all to "example.com" or something. They don't need to have the remote host hardcoded in there. + rserver = remoteWebServer.split(':') + if (len(rserver) == 2): You do this in a few places, I think you should just state that the option takes values of the form "IP:port" and enforce that once after the commandline options are parsed. + part = """ +user_pref("capability.principal.codebase.p%(i)d.granted", + "UniversalXPConnect UniversalBrowserRead UniversalBrowserWrite \ + UniversalPreferencesRead UniversalPreferencesWrite \ + UniversalFileRead"); +user_pref("capability.principal.codebase.p%(i)d.id", "%(origin)s"); +user_pref("capability.principal.codebase.p%(i)d.subjectName", ""); +""" % { "i": i+1, + "origin": (l.scheme + "://" + remoteIP + ":" + remotePort) } + prefs.append(part) Shouldn't this host already be included in the list that's generated right before this? If I look in my mochitesttestingprofile/user.js, I see: user_pref("capability.principal.codebase.p1.id", "http://localhost:8888"); - var origins = [%(origins)s]; + var origins = ['http://%(remote)s:%(port)s', %(origins)s]; Couldn't you just change the definition of "origins" up above to include the primary server? This is the only place it's used. if (isHttps) - return 'PROXY 127.0.0.1:4443'; + return 'PROXY %(remote)s:4443'; This is a little weird. You let the commandline option set the remote port for HTTP, but hardcode the SSL port. I think you ought to either let that port be configurable, or just declare both ports to be hardcoded, and only let the host be configured. + self.add_option("--setup-only", action = "store_true", + dest = "setupOnly", help = "only setup the profile, do not run tests") + defaults["setupOnly"] = False I think you should also add a "--profile-path" argument, so the caller can specify where the profile should be stored. (I'd like to make the normal mochitest profile wind up in a temp dir like reftest at some point.) + if (options.remoteWebServer != TEST_SERVER_HOST): + self._shutdownPath = "http://" + options.remoteWebServer + "/server/shutdown" Why bother with the if? Just always set this to include options.remoteWebServer, and remove the definition of SERVER_SHUTDOWN_URL completely. + if (len(options.remoteWebServer.split(':')) == 1): + options.remoteWebServer += ":8888" I hadn't noticed this check when I made that earlier comment. In light of this, I wouldn't bother with all the extra checks for IP:port in other code. Does remoteWebServer need to be an IP address, or do hostnames work as well? Your variable names are not clear on this point. You should explicitly state what's accepted in the help for the option. + if (options.setupOnly == True): return I don't like the single-line style. Put the return on a new line. - testURL = TESTS_URL + options.testPath + if (options.remoteWebServer != TEST_SERVER_HOST): Again, I don't think you need an if here, just always use options.remoteWebServer. Overall most of the concept looks good here, just a little quibbling over details.

This patch fixes the majority of the comments from Ted in my last patch. In addition I add the ability for a --profile-path cli option which defaults to a mkdtemp() call. Thanks for the feedback so far and for taking another look at this newly cleaned up patch.

I should note this patch is for mozilla-central only as there is a conflict with 1.9.2 in the initializeProfile directory.

updated patch for bitrot.

Comment on attachment 410322 [details] [diff] [review] mochitest remote + profile options patch >diff --git a/build/automation.py.in b/build/automation.py.in >-def fillCertificateDB(profileDir, certPath, utilityPath, xrePath): >+def fillCertificateDB(profileDir, certPath, utilityPath, xrePath, remoteWebServer = DEFAULT_WEB_SERVER): > pwfilePath = os.path.join(profileDir, ".crtdbpw") > > pwfile = open(pwfilePath, "w") >@@ -368,7 +375,7 @@ def fillCertificateDB(profileDir, certPa > sslTunnelConfig.write("listen:*:4443:pgo server certificate\n") > > # Configure automatic certificate and bind custom certificates, client authentication >- locations = readLocations() >+ locations = readLocations(remoteServer=remoteWebServer) > locations.pop(0) > for loc in locations: > if loc.scheme == "https" and "nocert" not in loc.options: I don't think you need this change to fillCertificateDB. It doesn't actually care what the main server IP is, it just wants to know about all the HTTPS hosts in the server list so it can tell ssltunnel what certificates it needs to serve. I'd just leave out the remoteServer part of this completely. >-def runApp(testURL, env, app, profileDir, extraArgs, >+def runApp(testURL, env, app, profileDir, extraArgs, remoteWebServer = DEFAULT_WEB_SERVER, > runSSLTunnel = False, utilityPath = DIST_BIN, > xrePath = DIST_BIN, certPath = CERTS_SRC_DIR, > debuggerInfo = None, symbolsPath = None, >@@ -514,7 +521,7 @@ def runApp(testURL, env, app, profileDir > > if IS_TEST_BUILD and runSSLTunnel: > # create certificate database for the profile >- certificateStatus = fillCertificateDB(profileDir, certPath, utilityPath, xrePath) >+ certificateStatus = fillCertificateDB(profileDir, certPath, utilityPath, xrePath, remoteWebServer) > if certificateStatus != 0: > log.info("TEST-UNEXPECTED FAIL | automation.py | Certificate integration failed") > return certificateStatus I think if you leave that parameter out of fillCertificateDB, then you dno't need it in runApp either, which is nice. >diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in >--- a/testing/mochitest/runtests.py.in >+++ b/testing/mochitest/runtests.py.in >+ self.add_option("--remote-webserver", action = "store", >+ type = "string", dest = "remoteWebServer", >+ help = "ip address where the remote web server is hosted at") >+ defaults["remoteWebServer"] = "localhost:8888" The help text says IP address, but the default is a hostname. I think this should default to 127.0.0.1:8888. >+ if (options.remoteWebServer == "localhost:8888"): >+ server = MochitestServer(options) >+ server.start() >+ >+ # If we're lucky, the server has fully started by now, and all paths are >+ # ready, etc. However, xpcshell cold start times suck, at least for debug >+ # builds. We'll try to connect to the server for awhile, and if we fail, >+ # we'll try to kill the server and exit with an error. >+ server.ensureReady(SERVER_STARTUP_TIMEOUT) Actually, after reading this, I think remoteWebServer should default to None, and then you can override it manually somewhere if need be. Otherwise you wind up doing checks like this, which are kind of crappy. >- testURL = TESTS_URL + options.testPath >+ tests_url = "http://" + options.remoteWebServer + "/tests/" >+ chrometests_url = "http://" + options.remoteWebServer + CHROME_PATH >+ a11ytests_url = "http://" + options.remoteWebServer + A11Y_PATH No real reason to keep these intermediate vars, is there? Just set the URLs directly in the if block below. >- processLeakLog(LEAK_REPORT_FILE, options.leakThreshold) >+ processLeakLog(leak_report_file, options.leakThreshold) > automation.log.info("\nINFO | runtests.py | Running tests: end.") > > # delete the profile and manifest Despite this comment, it doesn't look like we actually delete the profile directory. You should do so here, since it's using a temp directory by default. Otherwise we'll leave a bunch of temporary directories lying around. For extra caution, you can put the whole thing in a try/finally block like runreftest does: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#191 Looks good, r=me with those few things cleaned up.

carry over r+ from Ted on previous patch. This contains some minor cleanup. Tested in both localhost and remote web server on mozilla-central. NOTE: this patch does not apply to 1.9.2, but we can easily create a similar patch that will apply.

ctalbert, can you check this into m-c?

(In reply to comment #15) > ctalbert, can you check this into m-c? Attempted landing as changeset b3c18f150531, failed on windows, backed out

2 issues here: 1) on windows passing _PROFILE_PATH via -e to xpcshell.exe was causing issues with the slashes. Since we are doing a initWithPath() followed by an append(), we were getting a mix of slashes. By default we are running in mingw32 shell so we had our _PROFILE_PATH = 'c:/blah/blah/blah'. Then when we append(server-alive.txt), it has a \. Using mkdtemp() to get a _PROFILE_PATH (the new default), it was getting c:\docume~1\userna~1\locals~1\tempdir, which needs to be escaped via replace('\', '\\\\'). Not sure how much I like this 2) in toolkit/components/passwordmgr/test/test_prompt_async.html there is are some prompts that use moz-proxy://127.0.0.1 and this doesn't work since my implementation proxies through localhost:8888 by default. Adjusting everything to 127.0.0.1 doesn't work, but special casing the PAC file to change localhost to 127.0.0.1 works for a full test pass on linux. This seems reasonable.

This patch is updated to work as a 4th patch to bug 530475. There is no functionality difference from the original. NOTE: there are still two pending issues, one can be fixed in a test case, the other (xpcshell and windows paths) needs to be fixed in this patch. No review on this yet.

updated to be 2 space indent level and work with refactored runtests.py and automation.py scripts. Also fix the xpcshell problem on windows. Will ask for rereview once we get some of the other pre-requisite patches landed.

just a WIP, will submit for review once I am in the clear on dependent bugs.

updated patch for bitrot and some new functionality. This patch will allow for specification of --remote-webserver and port as well as profile. This runs on all platforms and depends on bug 544097.

Comment on attachment 427636 [details] [diff] [review] mochitest options to use a remote server (3) Reading through here, it looks good.

Comment on attachment 427636 [details] [diff] [review] mochitest options to use a remote server (3) >diff --git a/build/automation.py.in b/build/automation.py.in >--- a/build/automation.py.in >+++ b/build/automation.py.in >@@ -46,16 +46,20 @@ import re > import select > import shutil > import signal > import subprocess > import sys > import threading > import tempfile > >+DEFAULT_WEB_SERVER = "127.0.0.1" >+DEFAULT_HTTP_PORT = "8888" >+DEFAULT_SSL_PORT = "4443" Make the PORT variables actual integers, not strings. >@@ -223,28 +233,33 @@ class Automation(object): > lineno += 1 > if line.startswith("#") or line == "\n": > continue > > match = lineRe.match(line) > if not match: > raise SyntaxError(lineno) > >+ host = match.group("host") >+ port = match.group("port") > options = match.group("options") > if options: > options = options.split(",") > if "primary" in options: > if seenPrimary: > raise SyntaxError(lineno, "multiple primary locations") >+ >+ #we can have only 1 primary, make it the remoteServer >+ host = self.webServer >+ port = self.httpPort This is weird. I think instead, you should just do: if "primary" in options: continue And add your special remote primary address elsewhere. It's strange that you accept the primary entry here, but then totally subvert it. >@@ -316,17 +331,17 @@ user_pref("capability.principal.codebase > # We need to proxy every server but the primary one. > origins = ["'%s://%s:%s'" % (l.scheme, l.host, l.port) > for l in filter(lambda l: "primary" not in l.options, locations)] > origins = ", ".join(origins) > > pacURL = """data:text/plain, > function FindProxyForURL(url, host) > { >- var origins = [%(origins)s]; >+ var origins = ['http://%(remote)s:%(httpport)s', %(origins)s]; Isn't this going to confuse things? By putting the remote IP in here, you're telling the proxy PAC that you want to proxy connectiosn to that IP. If you leave it out, you'll just hit the DIRECT branch below, which seems to be what you want. >@@ -387,38 +402,38 @@ user_pref("camino.use_system_proxy_setti > pwfile.close() > > # Create head of the ssltunnel configuration file > sslTunnelConfigPath = os.path.join(profileDir, "ssltunnel.cfg") > sslTunnelConfig = open(sslTunnelConfigPath, "w") > > sslTunnelConfig.write("httpproxy:1\n") > sslTunnelConfig.write("certdbdir:%s\n" % certPath) >- sslTunnelConfig.write("forward:127.0.0.1:8888\n") >- sslTunnelConfig.write("listen:*:4443:pgo server certificate\n") >+ sslTunnelConfig.write("forward:127.0.0.1:" + self.httpPort + "\n") >+ sslTunnelConfig.write("listen:*:" + self.sslPort + ":pgo server certificate\n") Please use % string interpolation here, not string concatenation. >diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in >--- a/testing/mochitest/runtests.py.in >+++ b/testing/mochitest/runtests.py.in >@@ -194,57 +194,127 @@ class MochitestOptions(optparse.OptionPa > "(requires a debug build to be effective)") > defaults["fatalAssertions"] = False > > self.add_option("--extra-profile-file", > action = "append", dest = "extraProfileFiles", > help = "copy specified files/dirs to testing profile") > defaults["extraProfileFiles"] = [] > >+ self.add_option("--remote-webserver", action = "store", >+ type = "string", dest = "remoteWebServer", >+ help = "ip address where the remote web server is hosted at") I think this help text needs to be clearer. "remote web server" isn't very explanatory. It should probably mention that this is where server.js is running. I'm curious, does your Mochitest subclass handle all of this directly? Do we really need to add cmdline options for this, or can we just use the hooks you have in the rest of this file, and have your subclass detect and set the IP? It seems like this stuff wouldn't be very useful without the rest of your machinery to run tests remotely. >+ def verifyOptions(self, options, mochitest): >+ """ verify correct options and cleanup paths """ >+ if (options.remoteWebServer): >+ if (len(options.remoteWebServer.split(':')) != 1): Lose the parentheses here, you don't need them in Python if statements. >+ print "Error: --remote-webserver needs to be in the form of an IP address" This error message is sort of confusing. You're not actually testing that it's an IP address, just that it doesn't contain a colon. If that's really what you want to test, you should do it more thoroughly. > class MochitestServer: > "Web server used to serve Mochitests, for closer fidelity to the real web." > >- def __init__(self, automation, options, profileDir, shutdownURL): >+ def __init__(self, automation, options): > self._automation = automation > self._closeWhenDone = options.closeWhenDone > self._utilityPath = options.utilityPath > self._xrePath = options.xrePath >- self._profileDir = profileDir >- self.shutdownURL = shutdownURL >- self.webServer = "127.0.0.1" >+ self._profileDir = options.profilePath >+ self.shutdownURL = "http://" + options.webServer + ":" + options.httpPort + "/server/shutdown" % substitution, please. > def start(self): > "Run the Mochitest server, returning the process ID of the server." > > env = self._automation.environment(xrePath = self._xrePath) > env["XPCOM_DEBUG_BREAK"] = "warn" > if self._automation.IS_WIN32: > env["PATH"] = env["PATH"] + ";" + self._xrePath > > args = ["-g", self._xrePath, > "-v", "170", > "-f", "./" + "httpd.js", >- '-e', 'const _SERVER_ADDR="' + self.webServer + '";', >+ "-e", "const _PROFILE_PATH = '" + self._profileDir.replace('\\', '\\\\') + "'", >+ "-e", "const _SERVER_PORT = '" + self.httpPort + "'", >+ "-e", "const _SERVER_ADDR ='" + self.webServer + "';", You could just combine these all into a single -e argument, like -e 'const SERVER_ADDR="%s"; const PROFILE_PATH="%s"; ...' >diff --git a/testing/mochitest/server.js b/testing/mochitest/server.js >--- a/testing/mochitest/server.js >+++ b/testing/mochitest/server.js >@@ -44,17 +44,17 @@ > // Disable automatic network detection, so tests work correctly when > // not connected to a network. > let (ios = Cc["@mozilla.org/network/io-service;1"] > .getService(Ci.nsIIOService2)) { > ios.manageOfflineStatus = false; > ios.offline = false; > } > >-const SERVER_PORT = 8888; >+var SERVER_PORT = 8888; > var gServerAddress = "127.0.0.1"; > > var server; // for use in the shutdown handler, if necessary > > // > // HTML GENERATION > // > var tags = ['A', 'ABBR', 'ACRONYM', 'ADDRESS', 'APPLET', 'AREA', 'B', 'BASE', >@@ -169,25 +169,35 @@ function runServer() > if (!invalid) > gServerAddress = _SERVER_ADDR; > else > dumpn("WARNING: invalid server address ('" + _SERVER_ADDR + "'), using localhost"); > } > } > } > >+ if (typeof(_SERVER_PORT) != "undefined") { >+ if (parseInt(_SERVER_PORT) > 0 && parseInt(_SERVER_PORT) < 32000) >+ SERVER_PORT = _SERVER_PORT; >+ } >+ Just make this stuff all mandatory, and drop the defaults in this file. It's not like server.js is particularly useful without runtests.py. r- for some cleanup and some answers, but mostly it looks good.

updated patch to cleanup a lot of code from the patch and move to subclass. Addressed all concerns with review in comment #23.

had one typo in this patch that cropped up during a try server run

Comment on attachment 430703 [details] [diff] [review] mochitest options to use a remote server (4) >diff --git a/build/automation.py.in b/build/automation.py.in >--- a/build/automation.py.in >+++ b/build/automation.py.in >@@ -142,16 +146,22 @@ class Automation(object): > def __init__(self): > > # We use the logging system here primarily because it'll handle multiple > # threads, which is needed to process the output of the server and application > # processes simultaneously. > handler = logging.StreamHandler(sys.stdout) > self.log.setLevel(logging.INFO) > self.log.addHandler(handler) >+ self.setServerPort() >+ >+ def setServerPort(self, webServer = DEFAULT_WEB_SERVER, httpPort = DEFAULT_HTTP_PORT, sslPort = DEFAULT_SSL_PORT): nit: I'd probably name this "setServerInfo" or something like that, since it sets more than the port. >@@ -337,21 +347,21 @@ function FindProxyForURL(url, host) > if (isHttp) matches[3] = '80'; > if (isHttps) matches[3] = '443'; > } > > var origin = matches[1] + '://' + matches[2] + ':' + matches[3]; > if (origins.indexOf(origin) < 0) > return 'DIRECT'; > if (isHttp) >- return 'PROXY 127.0.0.1:8888'; >+ return 'PROXY %(remote)s:%(httpport)s'; > if (isHttps) >- return 'PROXY 127.0.0.1:4443'; >+ return 'PROXY %(remote)s:%(sslport)s'; > return 'DIRECT'; >-}""" % { "origins": origins } >+}""" % { "origins": origins, "remote":self.webServer, "httpport":self.httpPort, "sslport":self.sslPort } You should split this long dict literal into multiple lines, you can just write it like }""" % { "origins": origins, "remote": self.webServer, "httpport": self.httpPort, "sslport": self.sslPort } >diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in >--- a/testing/mochitest/runtests.py.in >+++ b/testing/mochitest/runtests.py.in > >+ def verifyOptions(self, options, mochitest): >+ """ verify correct options and cleanup paths """ >+ options.webServer = "127.0.0.1" >+ options.httpPort = 8888 >+ options.sslPort = 4443 Shouldn't you be using the constants you defined in automation.py? > args = ["-g", self._xrePath, > "-v", "170", > "-f", "./" + "httpd.js", >- '-e', 'const _SERVER_ADDR="' + self.webServer + '";', >+ "-e", "const _PROFILE_PATH = '" + self._profileDir.replace('\\', '\\\\') + "'; \ >+ const _SERVERPORT = '" + str(self.httpPort) + "'; const _SERVER_ADDR ='" + self.webServer + "';", % substitution would probably make this cleaner. Just sayin'. > def startWebServer(self, options): >+ if (options.webServer <> '127.0.0.1'): >+ return >+ Did mfinkle get to you with his VB-style Python? Use !=, not <>. (Also, remember you don't need the parentheses in Python if statements.) >- def stopWebServer(self): >+ def stopWebServer(self, options): > """ Server's no longer needed, and perhaps more importantly, anything it might > spew to console shouldn't disrupt the leak information table we print next. > """ >+ if (options.webServer <> '127.0.0.1'): >+ return Same here. >- def cleanup(self, manifest): >+ def cleanup(self, manifest, options): > """ remove temporary files and profile """ > os.remove(manifest) >- shutil.rmtree(self.PROFILE_DIRECTORY) >+ shutil.rmtree(options.profilePath) Will this be unexpected behavior if someone runs with --profile-path? You should probably document that the directory gets removed somewhere, maybe in the help text for the --profile-path option. r=me with those nits fixed.

Comment on attachment 431070 [details] [diff] [review] mochitest options to use a remote server (5) Bugzilla's interdiff is failing me here, so I'm going to assume that there's no major changes from the previous revision.

final patch with nits fixed. carryover of r+ from previous patch

Checked in as e939397fc6a5

updated patch for bitrot caused by https://bugzilla.mozilla.org/attachment.cgi?id=431705.

as a note, this was backed out last night due to a failure in bug 544097 which this relies on.

Landed as fbbaf1d62e84