Closed Bug 688604 Opened 13 years ago Closed 9 years ago

add a 'make talos-remote' option

Categories

(Testing :: Talos, defect)

ARM
Android
defect
Not set
normal

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?
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.
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?
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.
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?
(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
updated for bitrot
Attachment #562241 - Attachment is obsolete: true
updated for bitrot
Attachment #562437 - Attachment is obsolete: true
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.
updated webserver that works as good as a local apache server for serving webpages.
Assignee: nobody → jmaher
Attachment #562500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Whiteboard: [mobile_unittests] → [mobile_unittests][mozbase]
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)
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.
correct about where mozhttpd.py lives.  When this lands, I intend to synchronize between the mozbase project on github.

thanks for the other feedback.
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 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+
Moving the line endings to bug 697783
Blocks: 699128
updated with comments from jhammel addressed.
Attachment #570041 - Attachment is obsolete: true
Attachment #570041 - Flags: review?(wlachance)
Attachment #572066 - Flags: review?(wlachance)
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?
Attachment #572714 - Flags: review? → review?(wlachance)
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+
landed the --develop patch to talos:
http://hg.mozilla.org/build/talos/rev/da9b5f2aa1ff
Whiteboard: [mobile_unittests][mozbase] → [mobile_unittests][mozbase] talos-android
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.