Last Comment Bug 594018 - Selectively unpack test suites for unittests
: Selectively unpack test suites for unittests
Status: RESOLVED FIXED
:
Product: Release Engineering
Classification: Other
Component: Other (show other bugs)
: other
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Syed Albiz [:salbiz]
:
Mentors:
Depends on: 603133
Blocks: releng-downtime
  Show dependency treegraph
 
Reported: 2010-09-07 07:43 PDT by Syed Albiz [:salbiz]
Modified: 2013-08-12 21:54 PDT (History)
8 users (show)
bhearsum: needs‑treeclosure+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
selectiveUnpack (6.13 KB, patch)
2010-09-07 09:10 PDT, Syed Albiz [:salbiz]
catlee: feedback+
Details | Diff | Splinter Review
selectUnpack (now with context) (8.37 KB, patch)
2010-09-07 09:19 PDT, Syed Albiz [:salbiz]
no flags Details | Diff | Splinter Review
revised_selective_unzip (7.88 KB, patch)
2010-09-07 12:16 PDT, Syed Albiz [:salbiz]
catlee: review+
bhearsum: checked‑in+
Details | Diff | Splinter Review

Description Syed Albiz [:salbiz] 2010-09-07 07:43:48 PDT
Based on the comments in bug 586418, adding functionality to selectively unpack only the required tests should speed up unpacking time, since the entire suite does not need to unzipped.
Comment 1 Armen Zambrano [:armenzg] (EDT/UTC-4) 2010-09-07 07:57:24 PDT
Woohoo! Yay for optimization!
Comment 2 Syed Albiz [:salbiz] 2010-09-07 09:10:39 PDT
Created attachment 472647 [details] [diff] [review]
selectiveUnpack

Based on [:ctalbert] 's original patch, with modifications to use directory selection of files to unzip (rather than directory exclusions of stuff to not unzip), and a few minor fixes.
Comment 3 Syed Albiz [:salbiz] 2010-09-07 09:19:01 PDT
Created attachment 472649 [details] [diff] [review]
selectUnpack (now with context)
Comment 4 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-09-07 09:20:53 PDT
We need to make sure that whatever selective unpacking we do here is also used when developers run tests locally.  Otherwise they will forget to update the unpacking controls when they add inter-test dependencies, and we will have a lot of wasteful oranges as they push to try and/or m-c.  We have a long and painful history of this, such as with some-platforms-only REQUIRES and the various packaging manifests.
Comment 5 Chris AtLee [:catlee] 2010-09-07 09:24:24 PDT
Comment on attachment 472647 [details] [diff] [review]
selectiveUnpack

>-            elif suite in ('reftest', 'reftest-d2d', 'crashtest', 'jsreftest', \
>+            elif suite in ('jsreftest'):

('jsreftest') evaluates to 'jsreftest' in python.  It probably doesn't make a difference here, but just to be safe and consistent, I think this should be ('jsreftest',) (note the trailing comma).

>+class UnpackTest(ShellCommand):
>+    def __init__(self, filename, testtype, scripts_dir=".", **kwargs):
>+        self.filename = filename
>+        self.scripts_dir = scripts_dir
>+        self.testtype = testtype
>+        ShellCommand.__init__(self, **kwargs)
>+        self.addFactoryArguments(filename=filename, scripts_dir=scripts_dir)

testtype needs to be added the addFactoryArguments too.  Also, we need to hold on to a reference to the super class to avoid errors that happen during a reconfig.  See SendChangeStep for an example.

>+    def start(self):
>+        filename = self.build.getProperties().render(self.filename)
>+        self.filename = filename
>+        if filename.endswith(".zip"):
>+            # modify the commands to extract only the files we need - the test directory and bin/ and certs/
>+            if testtype == "mochitest":
>+                self.setCommand(['unzip', '-o', filename, 'bin* certs* mochitest*'])
>+            elif testtype == "xpcshell":
>+                self.setCommand(['unzip', '-o', filename, 'bin* certs* xpcshell*'])
>+            elif testtype == "jsreftest":
>+                # jsreftest needs both jsreftest/ and reftest/ in addition to bin/ and certs/
>+                self.setCommand(['unzip', '-o', filename, 'bin* certs* jsreftest* reftest*'])
>+            elif testtype == "reftest":
>+                self.setCommand(['unzip', '-o', filename, 'bin* certs* reftest*'])

We may run into problems with these....The 'bin* certs* mochitest*' string may end up being passed to unzip as a single argument, and it may fail to unpack the files you want.  You could also try something like:

['unzip', '-o', filename, 'bin', 'certs', 'reftest']

Or refactor it a bit like:
testDirs = {'mochitest': ['bin', 'certs', 'mochitest'], ...}
command = ['unzip', '-o', filename]
command.extend(testDirs.get(testtype, []))

The approach looks good enough to start testing though!
Comment 6 Armen Zambrano [:armenzg] (EDT/UTC-4) 2010-09-07 10:00:48 PDT
(In reply to comment #4)
> We need to make sure that whatever selective unpacking we do here is also used
> when developers run tests locally.  Otherwise they will forget to update the
> unpacking controls when they add inter-test dependencies, and we will have a
> lot of wasteful oranges as they push to try and/or m-c.  We have a long and
> painful history of this, such as with some-platforms-only REQUIRES and the
> various packaging manifests.

AFAIK developers don't test by downloading a packaged test but directly from their build. Not sure how we can tackle this. Maybe over-blogging so they know when they hit such problem?
Comment 7 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-09-07 10:02:58 PDT
No, this is not a problem that can be solved with documentation.  We need to make sure that the test harnesses respect the same visibility that the selective unpacking provides.  Any number of ways to do that, probably doesn't matter which we use.
Comment 8 Syed Albiz [:salbiz] 2010-09-07 12:16:35 PDT
Created attachment 472719 [details] [diff] [review]
revised_selective_unzip

Made recommended changes (ended up simply splitting the various test directories into separate list elements for sake of simplicity). Tested on staging. Unpack time drops from ~2-3 min to ~45s or less in most cases.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2010-09-07 12:57:57 PDT
(In reply to comment #4)
> We need to make sure that whatever selective unpacking we do here is also used
> when developers run tests locally.  Otherwise they will forget to update the
> unpacking controls when they add inter-test dependencies, and we will have a
> lot of wasteful oranges as they push to try and/or m-c.  We have a long and
> painful history of this, such as with some-platforms-only REQUIRES and the
> various packaging manifests.

I don't believe this is a problem in practice. This patch is only unpacking each test harness separately. If someone were to introduce inter-harness dependencies, they would already be broken, since the test package doesn't put harnesses in the same place relative to each other as they are in the objdir. As long as we don't start splitting up the individual harness types (mochitest-*, reftest/crashtest, xpcshell), this should continue to not be a problem.
Comment 10 Syed Albiz [:salbiz] 2010-09-10 06:33:10 PDT
Tested selective unzip on linux and windows, just to make sure it didn't cause any new oranges. No new oranges were caused by the change.
Comment 11 Syed Albiz [:salbiz] 2010-09-13 09:00:56 PDT
Also tested on mac, and again, no new oranges to report.
Comment 12 cmtalbert 2010-09-13 15:46:51 PDT
(In reply to comment #10)
> Tested selective unzip on linux and windows, just to make sure it didn't cause
> any new oranges. No new oranges were caused by the change.

Hey guys, I got off on another project and am just catching back up to this bug.  Thanks for making this patch a reality!  Syed, were there any speed differences when you tested it, or do we have to wait for the downtime this week to see the real impact?

I'm waiting with my fingers crossed to see if this helps!
Comment 13 Syed Albiz [:salbiz] 2010-09-13 17:56:13 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > Tested selective unzip on linux and windows, just to make sure it didn't cause
> > any new oranges. No new oranges were caused by the change.
> 
> Hey guys, I got off on another project and am just catching back up to this
> bug.  Thanks for making this patch a reality!  Syed, were there any speed
> differences when you tested it, or do we have to wait for the downtime this
> week to see the real impact?
> 
> I'm waiting with my fingers crossed to see if this helps!

Oh yes, most definitely. I think we shaved off about a minute and a half or so on average. Most tests unpack in under a minute with the patch. Thanks very much for the idea and initial patch.
Comment 14 Ben Hearsum (:bhearsum) 2010-10-01 10:55:03 PDT
Landing on Monday.
Comment 15 Ben Hearsum (:bhearsum) 2010-10-04 07:57:37 PDT
Comment on attachment 472719 [details] [diff] [review]
revised_selective_unzip

Landed in 06c9c8b9f9f0. Appears to be working well.
Comment 16 Nick Thomas [:nthomas] 2010-10-14 18:01:19 PDT
Syed, have you had a change to check the overall timings since this landed ?

Note You need to log in before you can comment on or make changes to this bug.