Closed
Bug 688604
Opened 13 years ago
Closed 9 years ago
add a 'make talos-remote' option
Categories
(Testing :: Talos, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: jmaher, Assigned: jmaher)
References
Details
(Whiteboard: [mobile_unittests][mozbase] talos-android)
Attachments
(3 files, 7 obsolete files)
1.97 KB,
patch
|
Details | Diff | Splinter Review | |
33.00 KB,
patch
|
Details | Diff | Splinter Review | |
34.11 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
it would be really nice to have a 'make talos-remote' option for mobile developers. This would: * download all packages and repositories to run mobile unittests * setup a webserver to point to this location * run 'python remotePerfConfigurator.py ...' * run 'python run_tests.py ... ' * turn off webserver * clean up directory not sure of all the logistics here, but what makes sense when it comes to checking stuff out of verifying we are up to date?
Assignee | ||
Comment 1•13 years ago
|
||
this patch is the glue for talos to work from a dynamic environment. This adds a webserver, and a --develop option to make the commandline easier for talos. For now, this only works on remote talos, but could be extended without much trouble to desktop talos. Another caveat is that in a non makefile environment this really only works for 1 instance of talos at a time. We discover the IP:PORT during remotePerfConfigurator and there could be a scenario where we run this a few times and generate the same IP:PORT in different .config files then we would have potential problems with running >1 instance of talos at a time. Really an edge case.
Assignee | ||
Comment 2•13 years ago
|
||
ok, this is so hacky, but it is a solution. I would like to polish it up. Does anybody have any overall objections to the work flow?
Comment 3•13 years ago
|
||
I don't really have any comment about the makefile workflow, but I might suggest putting some/most of the logic (downloading extensions, sensible defaults for the configurator, etc.) inside of talos itself. That way we'd simplify the lives of people using talos standalone as well.
Assignee | ||
Comment 4•13 years ago
|
||
maybe we could have a 'setup_remote.sh' script which does a lot of the extension and other copying. Then in the makefile we would just clone talos and run setup_remote.sh. anybody else have feedback?
Comment 5•13 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #4) > maybe we could have a 'setup_remote.sh' script which does a lot of the > extension and other copying. Then in the makefile we would just clone talos > and run setup_remote.sh. > > anybody else have feedback? I would prefer this approach, fwiw
Assignee | ||
Comment 6•13 years ago
|
||
updated for bitrot
Attachment #562241 -
Attachment is obsolete: true
Assignee | ||
Comment 7•13 years ago
|
||
updated for bitrot
Attachment #562437 -
Attachment is obsolete: true
Comment 8•13 years ago
|
||
I've been using a subset of this to do some talos testing on my device. mozhttpd has a minor bug that prevents URLs with a ? parameter from running correctly. You need to replace this function: class MozRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): def translate_path(self, path): # It appears that the default path is '/' and os.path.join makes the '/' return "/%s" % '/'.join([i.strip('/') for i in (DOCROOT, path)]) With this: class MozRequestHandler(SimpleHTTPServer.SimpleHTTPRequestHandler): def translate_path(self, path): # It appears that the default path is '/' and os.path.join makes the '/' o = urlparse(path) return "/%s" % '/'.join([i.strip('/') for i in (DOCROOT, o.path)]) And add this to the top: from urlparse import urlparse
(In reply to Joel Maher (:jmaher) from comment #4) > maybe we could have a 'setup_remote.sh' script which does a lot of the > extension and other copying. Then in the makefile we would just clone talos > and run setup_remote.sh. > > anybody else have feedback? I like the idea of the setup_remote.sh script because we could also package that with the tests and people who download packaged builds could quickly run that to set up their remote talos testing environment and wouldn't have to have a build environment to do this "the easy way". Otherwise, I like the work flow.
Assignee | ||
Comment 10•13 years ago
|
||
updated webserver that works as good as a local apache server for serving webpages.
Comment 11•13 years ago
|
||
Based on attachment 563416 [details] [diff] [review]. I added the final "raise" to debug an "Unknown error"; I'm not sure if it's wanted. Also removed trailing whitespace. I'd like to add a pidfile to be able to kill the webserver on exit; I'm not sure if that belongs in this patch or in the mozharness side.
Updated•13 years ago
|
Whiteboard: [mobile_unittests] → [mobile_unittests][mozbase]
Assignee | ||
Comment 12•13 years ago
|
||
my latest --develop and websever patch, please tear this apart with nit's are feedback comments!
Attachment #563416 -
Attachment is obsolete: true
Attachment #569479 -
Flags: feedback?(jhammel)
Comment 13•13 years ago
|
||
So I'm a bit confused....mozhttpd lives canonically at https://github.com/mozilla/mozbase/blob/master/mozhttpd/mozhttpd.py, right? Is there any reason that this just isn't imported into the diff and developed there? (obviously a step better is to have mozhttpd be a depedency of talos, but for now....). In any case, it would be nice to keep them synchronized. + port = url.port IIRC, this only works in modern versions of urlparse (2.6+?) + if browser_config['develop'] == True and httpd: the condition check is redundant The rest of the talos pieces look good.
Assignee | ||
Comment 14•13 years ago
|
||
correct about where mozhttpd.py lives. When this lands, I intend to synchronize between the mozbase project on github. thanks for the other feedback.
Assignee | ||
Comment 15•13 years ago
|
||
Did some surgery here, but this moves the remote webserver logic from the remote code to mainstream talos. This is required because in order to dynamically create a webserver for desktop talos, we need to find an open port. This patch includes the latest mozhttpd.py code
Attachment #569479 -
Attachment is obsolete: true
Attachment #569479 -
Flags: feedback?(jhammel)
Attachment #570041 -
Flags: review?(wlachance)
Attachment #570041 -
Flags: review?(jhammel)
Comment 16•13 years ago
|
||
Comment on attachment 570041 [details] [diff] [review] add --develop to talos option to start up webserver (1.0) + parts = line.split(' ') guessing this should be just `line.split()` + if ('.html' in part): (etc) should be if 'html' in part: + if (part <> parts[-1]): + newline += ' ' I'm not really sure what the goal of this line is. Assuming you mean: "add a space unless it is the last part", its probably better to do something like for index, part in enumerate(parts): ... if index == len(parts) -1: newline += ' ' or while parts: part = parts.pop(0) ... if parts: newline += '' Also, is the indentation off here? shouldn't this be at the level of the else above it, not indented. (might also want to part.endswith('.html), etc, too) + for line in manifestData.split('\n'): + newHandle.write(line.replace('localhost', self.webServer) + "\n") You use '\n' instead of system newlines here. Should probably use .readlines() to get the manifest lines vs splitting here and print >> newHandle to avoid having to add the system-dependent newline at the end + return "%s" % port This is silly. If you really need the string use str(port), though i'm not sure why you're not returning the numeric value. + #NOTE: this should be sufficient for defining a docroot + scheme = "http://" + if (server.startswith('http://') or + server.startswith('chrome://') or + server.startswith('file:///')): + scheme = "" + elif (server.find('://') >= 0): + raise talosError("Unable to parse user defined webserver: '%s'" % (server)) I am not sure I understand the point of this code. urlparse will do bad if a scheme is not defined. However, I'm not sure why the check just isn't url = server if ':// not in server: url = 'http://' + url url = urlparse.urlparse(url) + if url.port == None: + port = 80 + + if int(port) <= 0: + port = 80 please convert this to a single check: if not url.port or port < 0: port = 80 + #TODO: p2 is hardcoded, how do we determine what prefs.js has hardcoded? What does this mean? r+ with nits addressed
Attachment #570041 -
Flags: review?(jhammel) → review+
Comment 17•13 years ago
|
||
Moving the line endings to bug 697783
Assignee | ||
Comment 18•13 years ago
|
||
updated with comments from jhammel addressed.
Attachment #570041 -
Attachment is obsolete: true
Attachment #570041 -
Flags: review?(wlachance)
Attachment #572066 -
Flags: review?(wlachance)
Assignee | ||
Comment 19•13 years ago
|
||
slight update here to fix remote cases; this is what happens when I test on desktop after making some modifications.
Attachment #572066 -
Attachment is obsolete: true
Attachment #572066 -
Flags: review?(wlachance)
Attachment #572714 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #572714 -
Flags: review? → review?(wlachance)
Comment 20•13 years ago
|
||
Comment on attachment 572714 [details] [diff] [review] add --develop to talos option to start up webserver (2.1) As discussed on irc, this patch currently breaks tpan because of a problem with mozhttpd.py. r+ with jhammel's patch to mozhttpd applied. https://github.com/mozilla/mozbase/blob/86f091bdb0fadd293f19faf99b30e988c00eade8/mozhttpd/mozhttpd.py
Attachment #572714 -
Flags: review?(wlachance) → review+
Assignee | ||
Comment 21•13 years ago
|
||
landed the --develop patch to talos: http://hg.mozilla.org/build/talos/rev/da9b5f2aa1ff
Updated•9 years ago
|
Whiteboard: [mobile_unittests][mozbase] → [mobile_unittests][mozbase] talos-android
Assignee | ||
Comment 22•9 years ago
|
||
moving the remaining android talos tests to autophone this quarter, no need for a talos remote option
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•