488116 - Move CCUnittestBuildFactory into buildbotcustom

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Kaiser]

I created a unit test factory for comm-central builds, which is in a state now that should be ready to be shared with Thunderbird and tested to work well for SeaMonkey, so we should move it from the SeaMonkey buildbot config to buildbotcustom.

Comment 1

[Robert Kaiser]

Attachment: add CCUnittestBuildFactory to factory.py

This adds CCUnittestBuildFactory, which is based on UnittestBuildFactory but with everything the comm-central unittest builds need.
Notable differences:
- running client.py checkout and printing Mozilla revision to waterfall
- abstract app name for CreateProfile
- run -bin on Mac for CreateProfile, as SeaMonkey doesn't have the more or less
  useless start script there
- support leak thresholds for mochitest suites
- support tunring off reftest-based and mochitest-based suites (Thunderbird
  cannot run the latter and only wants to run the former quite rarely)

We should probably end up basing the test factories on the Mercurial build factories at one point, which could possibly reduce code duplication in some way or the other.

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 372396 [details] [diff] [review]
add CCUnittestBuildFactory to factory.py

>diff --git a/process/factory.py b/process/factory.py
>--- a/process/factory.py
>+++ b/process/factory.py
>@@ -2051,6 +2051,271 @@
>     def addPostTestSteps(self):
>         pass
> 
>+class CCUnittestBuildFactory(MozillaBuildFactory):
>+    def __init__(self, platform, config_repo_path, config_dir, objdir, mozRepoPath,
>+            productName=None, brandName=None, mochitest_leak_threshold=None,
>+            mochichrome_leak_threshold=None, mochibrowser_leak_threshold=None,
>+            exec_reftest_suites=True, exec_mochi_suites=True, **kwargs):
>+        self.env = {}
>+        MozillaBuildFactory.__init__(self, **kwargs)
>+        self.config_repo_path = config_repo_path
>+        self.mozRepoPath = mozRepoPath
>+        self.config_dir = config_dir
>+        self.objdir = objdir
>+        self.productName = productName
>+        if brandName:
>+            self.brandName = brandName
>+        else:
>+            self.brandName = productName.capitalize()
>+        if mochitest_leak_threshold:
>+            self.mochitest_leak_threshold = mochitest_leak_threshold
>+        if mochichrome_leak_threshold:
>+            self.mochichrome_leak_threshold = mochichrome_leak_threshold
>+        if mochibrowser_leak_threshold:
>+            self.mochibrowser_leak_threshold = mochibrowser_leak_threshold

I'm not sure why you need to check these args. Can't you just set them to a reasonable default and drop the 'if' part?

>+        if platform == 'win32':
>+            self.addStep(unittest_steps.CreateProfileWin,
>+             warnOnWarnings=True,
>+             workdir="build",
>+             command = r'python mozilla\testing\tools\profiles\createTestingProfile.py --clobber --binary %s\mozilla\dist\bin\%s.exe' %
>+                          (self.objdir, self.productName),
>+             clobber=True

This parameter doesn't actually exist. Passing it will break you when you upgrade to 0.7.10p1. May as well drop it now.

>+            )
>+        else:
>+            if platform == 'macosx':
>+                app_run = '%s.app/Contents/MacOS/%s-bin' % (self.brandName, self.productName)
>+            else:
>+                app_run = 'bin/%s' % self.productName
>+            self.addStep(unittest_steps.CreateProfile,
>+             warnOnWarnings=True,
>+             workdir="build",
>+             command = r'python mozilla/testing/tools/profiles/createTestingProfile.py --clobber --binary %s/mozilla/dist/%s' %
>+                          (self.objdir, app_run),
>+             clobber=True

Same thing here.

>+            )
>+
>+        if self.exec_reftest_suites:
>+            self.addStep(unittest_steps.MozillaReftest, warnOnWarnings=True,
>+             test_name="reftest",
>+             workdir="build/%s" % self.objdir,
>+             timeout=5*60,
>+            )
>+            self.addStep(unittest_steps.MozillaReftest, warnOnWarnings=True,
>+             test_name="crashtest",
>+             workdir="build/%s" % self.objdir,
>+            )
>+
>+        if self.exec_mochi_suites:
>+            if self.mochitest_leak_threshold:
>+                extra_args = "EXTRA_TEST_ARGS='--leak-threshold=%s'" % self.mochitest_leak_threshold
>+            else:
>+                extra_args = ""
>+            self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
>+             test_name="mochitest-plain",
>+             command = ["make", "mochitest-plain", extra_args],
>+             workdir="build/%s" % self.objdir,
>+             timeout=5*60,
>+            )
>+            if self.mochichrome_leak_threshold:
>+                extra_args = "EXTRA_TEST_ARGS='--leak-threshold=%s'" % self.mochichrome_leak_threshold
>+            else:
>+                extra_args = ""
>+            self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
>+             test_name="mochitest-chrome",
>+             command = ["make", "mochitest-chrome", extra_args],
>+             workdir="build/%s" % self.objdir,
>+            )
>+            if self.mochibrowser_leak_threshold:
>+                extra_args = "EXTRA_TEST_ARGS='--leak-threshold=%s'" % self.mochibrowser_leak_threshold
>+            else:
>+                extra_args = ""
>+            self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
>+             test_name="mochitest-browser-chrome",
>+             command = ["make", "mochitest-browser-chrome", extra_args],
>+             workdir="build/%s" % self.objdir,
>+            )
>+            if not self.platform == 'macosx':
>+              self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True,
>+               test_name="mochitest-a11y",
>+               workdir="build/%s" % self.objdir,
>+              )

This block gets simplified a lot if you can just always pass it in by setting a reasonable default. If you set the default to '0' that should give the same effect as not passing it in, right?


Other than the leak threshold stuff this looks fine. I think it would improve this code to do as suggested, but it's up to you. r=bhearsum if you want to leave it as is. I'm happy to review an updated patch if you want to change it.

Comment 3

[Robert Kaiser]

Attachment: add CCUnittestBuildFactory to factory.py, v2

Ben, this version is updated for your comments as well as some more changes Serge randomly attached to bug 485820 after making the leakThreshold param work on the steps in bug 479225.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 373640 [details] [diff] [review]
add CCUnittestBuildFactory to factory.py, v2

>+        if brandName:
>+            self.brandName = brandName
>+        else:
>+            self.brandName = productName.capitalize()

Misses my (optional) rewrite, unless you prefer it that way.

>+        self.mochitest_leak_threshold = mochitest_leak_threshold
>+        self.mochichrome_leak_threshold = mochichrome_leak_threshold
>+        self.mochibrowser_leak_threshold = mochibrowser_leak_threshold

Misses my (more explicit) renamings, but I could understand if you don't want them.
(Though I would rather rename them in config.py too...)


NB: Remember to check (bug 485820 patch Av2) config.py part in at the same time!

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 373640 [details] [diff] [review]
add CCUnittestBuildFactory to factory.py, v2

Looks good to me

Comment 6

[Robert Kaiser]

(In reply to comment #4)
> NB: Remember to check (bug 485820 patch Av2) config.py part in at the same
> time!

Doesn't need to be checked in at the same time, only when the SeaMonkey config switches to actually using that class (will do that in a followup patch).

Comment 7

[[e-mail] (:bhearsum)]

Hmmm. While looking at a patch in bug 469518 I realized that all of our existing unittest code uses None as a default for leak threshold. I don't really like that, as 0 makes much more sense, but I'm mentioning it in case you want to change it for the sake of consistency. (Sorry for not realizing this earlier).

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> Hmmm. While looking at a patch in bug 469518 I realized that all of our
> existing unittest code uses None as a default for leak threshold. I don't
> really like that, as 0 makes much more sense, but I'm mentioning it in case you
> want to change it for the sake of consistency. (Sorry for not realizing this
> earlier).

You're right, we can be consistent here now that we're using the generic param of the step and that one knows how to deal with None correctly.

Pushed as http://hg.mozilla.org/build/buildbotcustom/rev/4a337a624db2 with that change.

Comment 9

[Robert Kaiser]

Will hook up SeaMonkey to use the class from buildbotcustom in bug 485820, by the way (probably tomorrow).

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Hmmm. While looking at a patch in bug 469518 I realized that all of our
> existing unittest code uses None as a default for leak threshold. I don't
> really like that, as 0 makes much more sense, but I'm mentioning it in case you
> want to change it for the sake of consistency. (Sorry for not realizing this
> earlier).

Ftr, as I wrote in other bugs, we could have s/None/0/g.
Anyway, keeping None is fine too.