http://hg.mozilla.org/build/talos/file/52063311813e/talos/remotePerfConfigurator.py#l63 This instead should be done in run_tests.py or similar as it blocks bug 733583

I wouldn't mind trying my hand at this bug. From what I read this is whated: Take the function at line 63 in 'remotePerfConfigurator.py' and move it to the file 'run_tests.py'. Where I am confused: Will the change cause a regression for any program that wants to call the function from 'remotePeafConfigurator.py' instead of 'run_tests.py'? Do I need an android phone to verify/run tests on any of these changes? Thanks

essentially that is what you want to do. The work flow now is: * call *perfConfigurator.py * call run_tests.py So moving it to run_tests.py will be just fine. You don't need an android phone (although that is ideal), you can run a python agent that acts as the same protocol we have for the test sutagent on the android phones under test: http://people.mozilla.org/~jmaher/remotetesting/

(In reply to kanbie from comment #1) > I wouldn't mind trying my hand at this bug. > > From what I read this is whated: > Take the function at line 63 in 'remotePerfConfigurator.py' and move it > to the file 'run_tests.py'. > > Where I am confused: > Will the change cause a regression for any program that wants to call the > function from 'remotePeafConfigurator.py' instead of 'run_tests.py'? Nope; nothing should be calling this function at the moment so it should be safe. You should call the function if config['remote'] is set > Do I need an android phone to verify/run tests on any of these changes? As Joel said, that would be ideal. However, we can help test if you get up a patch.

(In reply to kanbie from comment #4) > Created attachment 635451 [details] [diff] [review] > Moves 'buildRemoteTwin' function from 'remotePerfConfigurator.py' to > 'run_tests.py' So this isn't quite there. While the function is moved over, it isn't called anywhere. It should be called with the same logic as is in remotePerfConfigurator.py : http://hg.mozilla.org/build/talos/file/7d28f2533eb5/talos/remotePerfConfigurator.py#l165 If 'winopen.xul' is in the test's URL and config['remote'] is True then we should call this function from the run_tests function: http://hg.mozilla.org/build/talos/file/7d28f2533eb5/talos/run_tests.py#l139 'self' won't be defined, since the function no longer lives on a class. So the references to self should be replaced and instead the need parameters should be passed to the function (we won't need remote since the run_tests function can call conditionally based on that, but we should find/pass in deviceroot. Also, we should raise a talosError instead of a configurationError I hope this helps! Let me know if this is clear

Points of confusion: Is 'buildRemoteTwinopen' building a virtual device? what is the purpose of that device? 'self' was in the 'buildRemoteTwinopen' function, does it need to change even though it is defined as a input? The comment about the deviceRoot confuses me, possible to elaborate? Thanks for your input!

Submittal for review.

Comment on attachment 635788 [details] [diff] [review] Moves 'buildRemoteTwin' function from 'remotePerfConfigurator.py' to 'run_tests.py', changes 'self' in 'run_tests.py' with 'browser_config' in appropiate spots. Comment on line 301 >diff -r 7d28f2533eb5 talos/remotePerfConfigurator.py >--- a/talos/remotePerfConfigurator.py Thu Jun 21 12:19:24 2012 -0400 >+++ b/talos/remotePerfConfigurator.py Fri Jun 22 11:45:23 2012 -0500 >@@ -163,7 +163,6 @@ > if self.remote == False: > return url > if 'winopen.xul' in url: >- self.buildRemoteTwinopen() > url = 'file://' + self.deviceroot + '/talos/' + url > > # Take care of tpan/tzoom tests >@@ -202,27 +201,6 @@ > self.config['deviceroot'] = self.deviceroot > self.remote = True > >- def buildRemoteTwinopen(self): >- """ >- twinopen needs to run locally as it is a .xul file. >- copy bits to <deviceroot>/talos and fix line to reference that >- """ >- # XXX this should live in run_test.py or similar >- >- if self.remote == False: >- return >- >- files = ['page_load_test/quit.js', >- 'scripts/MozillaFileLogger.js', >- 'startup_test/twinopen/winopen.xul', >- 'startup_test/twinopen/winopen.js', >- 'startup_test/twinopen/child-window.html'] >- >- talosRoot = self.deviceroot + '/talos/' >- for file in files: >- if self.testAgent.pushFile(file, talosRoot + file) == False: >- raise pc.ConfigurationError("Unable to copy twinopen file " >- + file + " to " + talosRoot + file) > > def main(args=sys.argv[1:]): > >diff -r 7d28f2533eb5 talos/run_tests.py >--- a/talos/run_tests.py Thu Jun 21 12:19:24 2012 -0400 >+++ b/talos/run_tests.py Fri Jun 22 11:45:23 2012 -0500 >@@ -298,6 +298,10 @@ > else: > print "WARNING: unable to start web server without custom port configured" > >+ # place files into the remote device (if it exists) for testing >+ if browser_config['remote'] and 'winopen.xul' in url: >+ buildRemoteTwinopen(browser_config) >+ You probably want test['url'] which is around here: http://hg.mozilla.org/build/talos/file/3607fcd7ce14/talos/run_tests.py#l167 You'll want to pass deviceroot and the testAgent to the function, not browser_config > # run the tests > utils.startTimer() > utils.stamped_msg(title, "Started") >@@ -354,5 +358,27 @@ > # run tests > run_tests(parser.config) > >+ >+def buildRemoteTwinopen(self): >+ """ >+ twinopen needs to run locally as it is a .xul file. >+ copy bits to <deviceroot>/talos and fix line to reference that >+ """ >+ >+ if self.remote == False: >+ return You won't need this if we test for remote above >+ files = ['page_load_test/quit.js', >+ 'scripts/MozillaFileLogger.js', >+ 'startup_test/twinopen/winopen.xul', >+ 'startup_test/twinopen/winopen.js', >+ 'startup_test/twinopen/child-window.html'] >+ >+ talosRoot = self.deviceroot + '/talos/' No 'self.' >+ for file in files: >+ if self.testAgent.pushFile(file, talosRoot + file) == False: >+ raise talosError("Unable to copy twinopen file " >+ + file + " to " + talosRoot + file) >+ > if __name__=='__main__': > main()

So something like this should work. I haven't been able to test as I don't have a device with me today. Does this sorta make sense?

we now don't copy these files for remote at all, Bug 798532