I've added top-level makefile targets for running reftest, crashtest, and all 4 mochitest variants. We should use these in the buildbot scripts so that we have more freedom to change things internally without changing the automation.

Note, you'll want to pass NO_FAIL_ON_TEST_ERRORS=1 when calling these, as by default they summarize failures and exit with an error code if there are any. Passing this variable will make them just execute as normal.

(In reply to comment #2) > *** Bug 480034 has been marked as a duplicate of this bug. *** My main need, for bug 469518, is to have 'runreftest.py' called, one way or another.

This should solve comment 3, though I need "you" to test it, especially on MacOSX trunk and branch. NB: I manually call runreftest.py successfully on my Win2K. This gets us closer to what |make reftest/crashtest| will do.

Comment on attachment 364908 [details] [diff] [review] (Av1) Use runreftest.py >-class MozillaOSXReftest(MozillaReftest): >- # Add support for |brand_name| argument. >- def __init__(self, brand_name, **kwargs): >- MozillaReftest.__init__(self, **kwargs) >- self.command = ["../../objdir/dist/%s.app/Contents/MacOS/firefox" % brand_name, >- "-console", >- "-P", >- "default", >- "-reftest", >- "reftest.list"] >+class MozillaOSXReftest(MozillaUnixReftest): >+ # Inheritance is enough. Let's get rid of classes like this and update the buildbot configs to call the base class instead.

Av1, with comment 5 suggestion(s). Reminder: I need you/someone to test it, especially on MacOSX trunk and branch.

Comment on attachment 365051 [details] [diff] [review] (Av2) Use runreftest.py This isn't what I want at all. I want them to use "make reftest" et al.

ted, who/when is 'make reftest' going to happen ? 'runreftest.py' is the blocking bug 469518 part and (I hope) I know how to do it (now).

"make reftest" exists on trunk already. It calls runreftest.py.

I know, I use it locally now: I meant who/when will do a patch to use them ?

Well, the bug is assigned to Lukas, so I would assume she's going to do it.

Serge, I still don't know why you aren't going the full way to just call the make target there, and <I guess Ted has the same confusion there as me.

What comment 12 said! See also: the bug summary.

Av2, with comment 1 suggestion(s). Like this ? Reminder: I need you/someone to test it, especially on MacOSX trunk and branch.

make: Entering directory `/builds/moz2_slave/mozilla-central-linux-unittest/build/objdir' rm -f ./reftest.log && /tools/python/bin/python _tests/reftest/runreftest.py /builds/moz2_slave/mozilla-central-linux-unittest/build/layout/reftests/reftest.list | tee ./reftest.log /tools/python/bin/python: can't open file '_tests/reftest/runreftest.py': [Errno 2] No such file or directory reftest passed make: Leaving directory `/builds/moz2_slave/mozilla-central-linux-unittest/build/objdir' Am I doing something wrong here?

This is what I'm working with on staging right now, for reference.

Does that file (objdir/_tests/reftest/runreftest.py) exist? Did you do a full build before you ran this?

Av3, with updated |workdir| values (which I missed), plus: *|brand_name| parameter actual removal. *|mochitest_leak_threshold| parameter removal: see bug 460548 comment 20. *|# Can use the regular build step here. Perl likes the PATHs that way anyway.| removal: I guess it does not apply/matter (anymore), at least not at that specific line. *Added mochitests part.

(In reply to comment #15) > /tools/python/bin/python: can't open file '_tests/reftest/runreftest.py': > [Errno 2] No such file or directory > reftest passed While we're there: "passed" seems inconsistent with "Errno 2" :-/

Comment on attachment 365747 [details] [diff] [review] Patch 1 >diff --git a/process/factory.py b/process/factory.py >- def __init__(self, platform, brand_name, config_repo_path, config_dir, >+ def __init__(self, platform, config_repo_path, config_dir, >- self.brand_name = brand_name Merged: I wasn't sure if I should do that or not ;-) >@@ -1863,78 +1862,60 @@ class UnittestBuildFactory(MozillaBuildF >- if self.platform == 'linux': >+ if self.platform == 'win32': Ted, I'll swap them before check-in: I prefer not to for now, to ease review. ;-) >- workdir="build/layout/reftests", >+ workdir="build/objdir", Merged: Thanks, I missed these ;-< >diff --git a/unittest/steps.py b/unittest/steps.py > class MozillaReftest(ShellCommandReportTimeout): > name = "reftest" >+ command = ["make", "-C", ".", name] Not merged: *Even if this seems easy to do, I think |command| should not depend on |name|. *Do we want |"-C", "."|, or is it for testing purpose only ?

Comment on attachment 365747 [details] [diff] [review] Patch 1 I've tested this on staging-master and it works on both 1.9.1 and m-c.

Comment on attachment 365747 [details] [diff] [review] Patch 1 >- if self.platform == 'linux': >- self.addStep(unittest_steps.MozillaUnixReftest, warnOnWarnings=True, >- workdir="build/layout/reftests", >+ if self.platform == 'win32': >+ self.addStep(unittest_steps.MozillaReftest, warnOnWarnings=True, >+ workdir="build\\objdir", I'd like to know if we can get away with a single series of addSteps with workdir='build/objdir', instead of having the separate section for windows with workdir='build\\objdir" > class MozillaReftest(ShellCommandReportTimeout): > warnOnFailure = True > name = "reftest" >+ command = ["make", "-C", ".", name] What's the purpose of "-C ." ?

So when I said "tested" - I had missed that the command was not being updated with the correct name. This patch fixes that by setting the command in _init_ and also I did get rid of the extra steps that were windows specific (this worked on try staging) as well as the -C

Comment on attachment 366548 [details] [diff] [review] Patch 2 [Superseded by bug 445611] >+ command = ["make", ".", self.name] This won't do what you want. You probably want command = ["make", self.name] instead.

Comment on attachment 366548 [details] [diff] [review] Patch 2 [Superseded by bug 445611] >diff --git a/unittest/steps.py b/unittest/steps.py > def createSummary(self, log): >+ command = ["make", ".", self.name] > def createSummary(self, log): >+ command = ["make", ".", self.name] These are unwanted. (And I still think |command| should not depend on |name|.)

Comment on attachment 365853 [details] [diff] [review] (Av4) Reftests + Mochitests Lukas is working on a patch here.

The adjustments to unittest/steps.py and process/factory.py are being checked in via bug 445611 This patch just removes one line from the initialization of the UnittestBuildFactory that refers to brand_name which no longer exists in the factory.

Comment on attachment 367618 [details] [diff] [review] Remove brand_name from master.cfg for staging and production [Checkin: Comment 28] changeset: 1011:eb1b1fc058d3

This has been deployed and is running on production and staging unittests.

Comment on attachment 367955 [details] [diff] [review] (Av5) Av4 leftovers after bug 445611 Serge, can you please provide a short description of the purpose of the patch? The attachment name is unhelpful.

Comment on attachment 367955 [details] [diff] [review] (Av5) Av4 leftovers after bug 445611 (In reply to comment #31) *Removes obsolete |mochitest_leak_threshold|. (See bug 460548 comment 20 and bug 469581.) *Also disable |leakThreshold| ftb. *Synchronize MozillaCheck step with the (new) Reftest and Mochitest ones. *Removes obsolete |MOZ_CO_PROJECT|. (See bug 451825.) *Some nits.

Comment on attachment 367955 [details] [diff] [review] (Av5) Av4 leftovers after bug 445611 Most of this patch is unwanted, see below for details. >diff --git a/process/factory.py b/process/factory.py >--- a/process/factory.py >+++ b/process/factory.py >@@ -1747,19 +1747,20 @@ class ReleaseFinalVerification(ReleaseFa > ReleaseFactory.__init__(self, repoPath='nothing', **kwargs) > self.addStep(ShellCommand, > command=['bash', 'final-verification.sh', > linuxConfig, macConfig, win32Config], > description=['final-verification.sh'], > workdir='tools/release' > ) > >+ >+ This hunk is fine. > class UnittestBuildFactory(MozillaBuildFactory): >- def __init__(self, platform, config_repo_path, config_dir, >- mochitest_leak_threshold=None, **kwargs): >+ def __init__(self, platform, config_repo_path, config_dir, **kwargs): Please don't disable leak threshold here, we may use it in the future. >- # TODO: Do we need this special windows rule? >- if self.platform == 'win32': >- self.addStep(unittest_steps.MozillaCheck, warnOnWarnings=True, >- workdir="build\\objdir", >- timeout=60*5 >- ) >- else: >- self.addStep(unittest_steps.MozillaCheck, >- warnOnWarnings=True, >- timeout=60*5, >- workdir="build/objdir" >- ) >+ self.addStep(unittest_steps.MozillaCheck, >+ warnOnWarnings = True, >+ workdir = "build/objdir", >+ timeout = 5 * 60, # 5 mn. >+ ) I like the unification, don't change format of the arguments, though. And please use '5 minutes' or '5 min.'. >- timeout=60*5 >+ timeout = 5 * 60, # 5 mn. Same thing here and for all of the other ones below re: spacing and the comment. >+ # Don't run the a11y tests on MacOSX. (Bug 342989) This comment can stay. > if not self.platform == 'macosx': > self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True, > test_name="mochitest-a11y", > workdir="build/objdir" > ) >+ This newline can stay. > def addStepNoEnv(self, *args, **kw): > return BuildFactory.addStep(self, *args, **kw) > > >+ And this one. >diff --git a/unittest/steps.py b/unittest/steps.py None of the changes to this file are wanted, see below for details. >- env = {} >- if 'env' in kwargs: >- env = kwargs['env'].copy() >- env['MOZ_CO_PROJECT'] = self.project >- kwargs['env'] = env This class requires MOZ_CO_PROJECT to function, don't remove it. > class MozillaMochitest(ShellCommandReportTimeout): > warnOnFailure = True >- >- def __init__(self, test_name, leakThreshold=None, **kwargs): >+ >+ # |leakThreshold| feature is disabled. (Bug 469581 then bug 460548) >+ def __init__(self, test_name, leakThreshold = None, **kwargs): As mentioned above please don't disable the leak threshold option.

steps.py part of Av5, with comment 33 suggestion(s). (In reply to comment #33) > This class requires MOZ_CO_PROJECT to function, don't remove it. Added your irc answer as a comment instead. Ftr, I wondered because this variable was removed from m-c and m-1.9.1, and looked to be set only (not used) per http://mxr.mozilla.org/build/search?string=MOZ_CO_PROJECT&case=on > As mentioned above please don't disable the leak threshold option. It seemed odd to leave an obsolete option active. Now updated after bug 469581.

factory.py part of Av5, with comment 33 suggestion(s). (In reply to comment #33) > Please don't disable leak threshold here, we may use it in the future. Added a comment instead. Ftr, this was used for 'mochitest-plain' only; in the future, we would need to support the 4 'mochitest-*', the 2 'reftest/crashtest' and maybe 1 'check'. > >+ # Don't run the a11y tests on MacOSX. (Bug 342989) I updated the bug number to the new bug.

Ev1 unbitrotted

Comment on attachment 369094 [details] [diff] [review] (Dv1) steps.py: Av4 leftovers after bug 445611 >diff --git a/unittest/steps.py b/unittest/steps.py >- env = {} >- if 'env' in kwargs: >- env = kwargs['env'].copy() >- env['MOZ_CO_PROJECT'] = self.project >- kwargs['env'] = env >+ # |MOZ_CO_PROJECT|: "used in the try server cvs trunk builders, not used by the unittests on tryserver though" supernit: limit your lines to 80 characters >- >- def __init__(self, test_name, leakThreshold=None, **kwargs): >+ >+ def __init__(self, test_name, leakThreshold = None, **kwargs): style nit: be consistent with the rest of this file - no spaces between arg and default (eg, leakThreshold=None). r=bhearsum with those changes.

Dv1, with comment 37 suggestion(s).

Comment on attachment 369800 [details] [diff] [review] (Ev1a) factory.py: Av4 leftovers after bug 445611 and bug 474666 > self.addStep(unittest_steps.MozillaMochitest, warnOnWarnings=True, > test_name="mochitest-plain", > workdir="build/%s" % self.objdir, >- leakThreshold=mochitest_leak_threshold, As I mentioned last time, we don't want to disable this code. Please add this line back in. r=bhearsum with that change

Ev1a, with comment 39 suggestion(s).

Comment on attachment 370482 [details] [diff] [review] (Dv1a) steps.py: Av4 leftovers after bug 445611 [Checkin: See comment 41] changeset: 245:a6af2fa8c81c

Comment on attachment 370970 [details] [diff] [review] (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 [Checkin: See comment 42] changeset: 244:961354158b72

(In reply to comment #19) > While we're there: "passed" seems inconsistent with "Errno 2" :-/ I filed bug 487361.