add a 'make talos-remote' option

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
4 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mobile_unittests][mozbase] talos-android)

Attachments

(3 attachments, 7 obsolete attachments)

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
(Assignee)

Description

8 years ago
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

8 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

8 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?
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

8 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

8 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

8 years ago
updated for bitrot
Attachment #562241 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
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

Comment 9

8 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 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

8 years ago
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.

Updated

8 years ago
Whiteboard: [mobile_unittests] → [mobile_unittests][mozbase]
(Assignee)

Comment 12

8 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

8 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

8 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

8 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

8 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

8 years ago
Moving the line endings to bug 697783
(Assignee)

Comment 18

8 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

8 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

8 years ago
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+
(Assignee)

Comment 21

8 years ago
landed the --develop patch to talos:
http://hg.mozilla.org/build/talos/rev/da9b5f2aa1ff
Whiteboard: [mobile_unittests][mozbase] → [mobile_unittests][mozbase] talos-android
(Assignee)

Comment 22

4 years ago
moving the remaining android talos tests to autophone this quarter, no need for a talos remote option
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.