594018 - Selectively unpack test suites for unittests

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Syed Albiz [:salbiz]]

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 [:armenzg]]

Woohoo! Yay for optimization!

Comment 2

[Syed Albiz [:salbiz]]

Attachment: 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]]

Attachment: selectUnpack (now with context)

Comment 4

[Mike Shaver (:shaver emeritus)]

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

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 [:armenzg]]

(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 emeritus)]

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

Attachment: 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

[(not currently active) Ted Mielczarek]

(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]]

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

Also tested on mac, and again, no new oranges to report.

Comment 12

[cmtalbert]

(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]]

(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

[[e-mail] (:bhearsum)]

Landing on Monday.

Comment 15

[[e-mail] (:bhearsum)]

Comment on attachment 472719 [details] [diff] [review]
revised_selective_unzip

Landed in 06c9c8b9f9f0. Appears to be working well.

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Syed, have you had a change to check the overall timings since this landed ?