736105 - robocop does a lot of redundant copying of profile and sleeps, we can cut runtime almost in half

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Robocop looks through mochitest for each test case.  This involves:
* create profile
* remove old profile and bits from device
* copy profile to device
* install extensions
* copy profile to device
* run test
** here we launch process, then sleep for 30 seconds to ensure we get the application running
* get log file
* search for crashes
* cleanup profile on device

I found that we can push the profile once and cleanup some files.  this saves about 150 seconds per test case.  Also not cleaning up the entire profile saves us about 20 seconds per test case. 

Lastly, when we run the robocop tests, we launch and wait until it is done before checking for the process.  This could be a limitation of how we are using am instrument or sutagent.  Nevertheless, we spin for 30 seconds after completing just to return.

All in all, we can remove about 210 seconds/testcase (10+ cases already) where we have an average runtime/testcase of about 60 seconds.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: optimize the harness parts of robocop (1.0)

Comment 2

[William Lachance (:wlach)]

Comment on attachment 606229 [details] [diff] [review]
optimize the harness parts of robocop (1.0)

r- because I'm not sure about resetting options.robocop at runtime. Looks good otherwise.

>diff -r 936ef50fa498 build/mobile/devicemanagerSUT.py
>--- a/build/mobile/devicemanagerSUT.py	Wed Mar 14 11:18:15 2012 -0400
>+++ b/build/mobile/devicemanagerSUT.py	Thu Mar 15 11:35:36 2012 -0400
>@@ -543,6 +543,10 @@
>     except AgentError:
>       return None
> 
>+    if re.search('am instrument -w -e class org.mozilla', appname):
>+      print "terminating early due to am instrument vs org.mozilla.f*"
>+      return 1
>+

This is pretty gross, but as discussed, fixing this properly is more work than we want to take on right now. 
Let's mark this as a hack with a comment. Something like:

# HACK: When using "am instrument", the process disappears almost immediately as it is
# only launching another process.

>     # wait up to 30 seconds for process to start up
>     timeslept = 0
>     while (timeslept <= 30):
>diff -r 936ef50fa498 testing/mochitest/runtestsremote.py
>--- a/testing/mochitest/runtestsremote.py	Wed Mar 14 11:18:15 2012 -0400
>+++ b/testing/mochitest/runtestsremote.py	Thu Mar 15 11:35:36 2012 -0400
>@@ -223,6 +223,7 @@
>     _dm = None
>     localProfile = None
>     logLines = []
>+    _profileExists = False
> 
>     def __init__(self, automation, devmgr, options):
>         self._automation = automation
>@@ -236,6 +237,29 @@
>     def cleanup(self, manifest, options):
>         self._dm.getFile(self.remoteLog, self.localLog)
>         self._dm.removeFile(self.remoteLog)
>+
>+        if options.robocop != "":
>+            print "Not removing profile since we need it in the next run!"
>+            self._dm.removeFile(self.remoteProfile + '/Cache');
>+            self._dm.removeFile(self.remoteProfile + '/cert9.db');
>+            self._dm.removeFile(self.remoteProfile + '/compatibility.ini');
>+            self._dm.removeFile(self.remoteProfile + '/cookies.sqlite');
>+            self._dm.removeFile(self.remoteProfile + '/cookies.sqlite-shm');
>+            self._dm.removeFile(self.remoteProfile + '/cookies.sqlite-wal');
>+            self._dm.removeFile(self.remoteProfile + '/extensions.ini');
>+            self._dm.removeFile(self.remoteProfile + '/extensions.sqlite');
>+            self._dm.removeFile(self.remoteProfile + '/extensions.sqlite-journal');
>+            self._dm.removeFile(self.remoteProfile + '/key4.db');
>+            self._dm.removeFile(self.remoteProfile + '/localstore.rdf');
>+            self._dm.removeFile(self.remoteProfile + '/.parentlock');
>+            self._dm.removeFile(self.remoteProfile + '/pkcs11.txt');
>+            self._dm.removeFile(self.remoteProfile + '/prefs.js');
>+            self._dm.removeFile(self.remoteProfile + '/search.json');
>+            self._dm.removeFile(self.remoteProfile + '/search.sqlite');
>+            self._dm.removeFile(self.remoteProfile + '/sessionstore.bak');
>+            self._dm.removeFile(self.remoteProfile + '/sessionsstore.js');
>+            return

This strategy of resetting the options.robocop variable to get the desired behaviour in cleanup() doesn't seem right to me. "options" basically contains the intention of the user who called the script, and should IMO be treated as immutable. I think it would be clearer/better/more general to add a parameter to cleanup called partial which can be True or False. If it's true, then do the behaviour above. If false, then do a full cleanup.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 606229 [details] [diff] [review]
optimize the harness parts of robocop (1.0)

Review of attachment 606229 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/runtestsremote.py
@@ +256,5 @@
> +            self._dm.removeFile(self.remoteProfile + '/prefs.js');
> +            self._dm.removeFile(self.remoteProfile + '/search.json');
> +            self._dm.removeFile(self.remoteProfile + '/search.sqlite');
> +            self._dm.removeFile(self.remoteProfile + '/sessionstore.bak');
> +            self._dm.removeFile(self.remoteProfile + '/sessionsstore.js');

I have a slight concern that removing these files one-by-one will cause problems later when the profile contents change. Would it be possible to delete all files except for those that you know we need? Just a suggestion...

Also I agree with :wlach about controlling the partial/full deletion with a parameter to cleanup.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: optimize the harness parts of robocop (1.1)

I fixed the use of a state variable inside of mochitestremote class.  To fix cleanup() it is not as straightforward since we are calling cleanup() from the mochitest class itself and don't know the state or other conditions.  I solved this with  a _reuseProfile flag that I manage in runtestsremote.py and this seems to work well.

Regarding the master list of stuff to delete, I found that just deleting .parentLock allowed me to test.  I am doing that for now.  I realized that all our other tests depend on the same profile throughout a test run, this should be just fine.

Comment 5

[William Lachance (:wlach)]

Comment on attachment 606599 [details] [diff] [review]
optimize the harness parts of robocop (1.1)

Much better. Let's make sure this works on try of course.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

inbound

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f626859690df

Comment 8

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d752af9a809b

because of frequent robocop failures in testAboutPage:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10183506&tree=Mozilla-Inbound

Comment 9

[Aaron Train [:aaronmt]]

I want this :)

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

So do I!  Maybe next week I can hack on this some more to get it green and stable.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix performance and usage issues when running >1 test on fennec (1.0)

this patch does a few things:
1) Doesn't copy 90% of the stuff we normally copy over in the profile.  This is achieved by removing the files from the /tmp directory we create with the profile to push to the device
2) Removes the internal application profile from the device between runs.  In testBookmarklets, I could pass the test on the initial run, but on subsequent runs it would fail.  I suspect this will help greatly in reducing cross test interference (also known as orange).

Comment 12

[William Lachance (:wlach)]

Comment on attachment 693972 [details] [diff] [review]
fix performance and usage issues when running >1 test on fennec (1.0)

f- only because I don't like the direct call to _runCmds in _dm. Even if this code is only used in automation, that's not a good practice to encourage.

Looks good aside from that.

># HG changeset patch
># Parent b887e879e2cc3822925f653a415649ecc1fd9fce
>Bug 736105 - robocop does a lot of redundant copying of profile and sleeps, we can cut runtime almost in half. r=gbrown
>
>diff -r b887e879e2cc testing/mochitest/runtestsremote.py
>--- a/testing/mochitest/runtestsremote.py	Tue Dec 18 14:20:35 2012 -0500
>+++ b/testing/mochitest/runtestsremote.py	Wed Dec 19 14:34:19 2012 -0500
>@@ -289,6 +292,19 @@
>         manifest = Mochitest.buildProfile(self, options)
>         self.localProfile = options.profilePath
>         self._dm.removeDir(self.remoteProfile)
>+
>+        # we do not need this for robotium based tests, lets save a LOT of time
>+        if options.robocop:
>+            shutil.rmtree(os.path.join(options.profilePath, 'webapps'))
>+            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
>+            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
>+            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
>+            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
>+            os.remove(os.path.join(options.profilePath, 'tests.jar'))
>+            os.remove(os.path.join(options.profilePath, 'userChrome.css'))
>+            os.remove(os.path.join(options.profilePath, 'tests.manifest'))


You might want to rewrite this as something like:

shutil.rmtree(os.path.join(options.profilePath, 'webapps'))
for extension in [ '[e-mail]', '[e-mail]', ... ]:
    shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', extension)
for filename in ['tests.jar', ...]:
   os.remove(os.path.join(options.profilePath, filename))

>+            self._dm._runCmds([{'cmd': 'exec su -c "rm -R /data/data/%s/files"' % options.remoteappname}])
>+

This is what I really don't like, as _runCmds is a dmSUT specific interface. Something like the following should work well for dmADB (well, at least if the device has root):

self._dm.shellCheckOutput(['rm', '-R', '/data/data/%s/files"' % options.remoteappname], root=True)

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 693972 [details] [diff] [review]
fix performance and usage issues when running >1 test on fennec (1.0)

Review of attachment 693972 [details] [diff] [review]:
-----------------------------------------------------------------

r- for rm -R.

::: testing/mochitest/runtestsremote.py
@@ +301,5 @@
> +            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
> +            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
> +            os.remove(os.path.join(options.profilePath, 'tests.jar'))
> +            os.remove(os.path.join(options.profilePath, 'userChrome.css'))
> +            os.remove(os.path.join(options.profilePath, 'tests.manifest'))

Very nice - bravo!

@@ +302,5 @@
> +            shutil.rmtree(os.path.join(options.profilePath, 'extensions', 'staged', '[e-mail]'))
> +            os.remove(os.path.join(options.profilePath, 'tests.jar'))
> +            os.remove(os.path.join(options.profilePath, 'userChrome.css'))
> +            os.remove(os.path.join(options.profilePath, 'tests.manifest'))
> +            self._dm._runCmds([{'cmd': 'exec su -c "rm -R /data/data/%s/files"' % options.remoteappname}])

I agree with :wlach on this.

Is there any difference between rm -r and rm -R?

Also, I question if it is necessary. After robocop tests, I only see an empty "Crash Reports" directory, a simple profile.ini, and an empty profile directory created at this location -- I would not expect any of those to affect future tests.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I didn't narrow down the files directory, but all that is in there is mozilla/ and the profile.  After about 10 times verifying that I get failures with the files directory in place and removing it, I get none, I moved forward.  

Unfortunately running on try server yielded results:
https://tbpl.mozilla.org/?tree=Try&rev=a97ff55b5af9

Maybe I should do the profile push part of this and do a followup patch for the cleanup of the files.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: remove extra files from profile before pushing it over (1.0)

this is the same as my other patch with the 'rm -r' line removed.  I was going for the big win and in this case we can reap the rewards of a slim profile.

Comment 16

[Geoff Brown [:gbrown]]

Comment on attachment 695986 [details] [diff] [review]
remove extra files from profile before pushing it over (1.0)

Review of attachment 695986 [details] [diff] [review]:
-----------------------------------------------------------------

Really glad you found this - can't wait to see it running.

Comment 17

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/a0bc627d0af7