479225 - run reftest/crashtest with "make {reftest,crashtest}" and mochitest with "make mochitest-{plain,chrome,browser-chrome,a11y}"

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Serge Gautherie (:sgautherie)]

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

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Use runreftest.py

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 5

[Chris Cooper [:coop] (he/him)]

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.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Use runreftest.py

Av1, with comment 5 suggestion(s).

Reminder:
I need you/someone to test it, especially on MacOSX trunk and branch.

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Serge Gautherie (:sgautherie)]

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).

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Serge Gautherie (:sgautherie)]

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

Comment 11

[(not currently active) Ted Mielczarek]

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

Comment 12

[Robert Kaiser]

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.

Comment 13

[Jeff Walden [:Waldo]]

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

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Reftests part

Av2, with comment 1 suggestion(s).

Like this ?

Reminder:
I need you/someone to test it, especially on MacOSX trunk and branch.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Patch 1

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

Comment 17

[(not currently active) Ted Mielczarek]

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

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Av4) Reftests + Mochitests

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.

Comment 19

[Serge Gautherie (:sgautherie)]

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

[Serge Gautherie (:sgautherie)]

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 21

[Lukas Blakk [:lsblakk] use ?needinfo]

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 22

[Chris AtLee [:catlee]]

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 ." ?

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Patch 2 [Superseded by bug 445611]

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 24

[Chris AtLee [:catlee]]

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 25

[Serge Gautherie (:sgautherie)]

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 26

[(not currently active) Ted Mielczarek]

Comment on attachment 365853 [details] [diff] [review]
(Av4) Reftests + Mochitests

Lukas is working on a patch here.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: Remove brand_name from master.cfg for staging and production [Checkin: Comment 28]

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 28

[[e-mail] (:bhearsum)]

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

changeset:   1011:eb1b1fc058d3

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Av5) Av4 leftovers after bug 445611

Comment 31

[[e-mail] (:bhearsum)]

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 32

[Serge Gautherie (:sgautherie)]

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 33

[[e-mail] (:bhearsum)]

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.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) steps.py: Av4 leftovers after bug 445611

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.

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) factory.py: Av4 leftovers after bug 445611

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.

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1a) factory.py: Av4 leftovers after bug 445611 and bug 474666

Ev1 unbitrotted

Comment 37

[[e-mail] (:bhearsum)]

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.

Comment 38

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) steps.py: Av4 leftovers after bug 445611 [Checkin: See comment 41]

Dv1, with comment 37 suggestion(s).

Comment 39

[[e-mail] (:bhearsum)]

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

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 [Checkin: See comment 42]

Ev1a, with comment 39 suggestion(s).

Comment 41

[Chris AtLee [:catlee]]

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

changeset:   245:a6af2fa8c81c

Comment 42

[Chris AtLee [:catlee]]

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

Comment 43

[Serge Gautherie (:sgautherie)]

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

I filed bug 487361.