Closed Bug 479225 Opened 11 years ago Closed 11 years ago

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

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: lsblakk)

References

()

Details

(Keywords: autotest-issue)

Attachments

(3 files, 10 obsolete files)

1.92 KB, patch
bhearsum
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
2.52 KB, patch
catlee
: checked-in+
Details | Diff | Splinter Review
4.07 KB, patch
catlee
: checked-in+
Details | Diff | Splinter Review
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.
OS: Windows XP → All
Hardware: x86 → All
Assignee: nobody → lukasblakk
Priority: -- → P3
Status: NEW → ASSIGNED
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.
Depends on: 480034
No longer depends on: 480034
Duplicate of this bug: 480034
Depends on: 468913
(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.
Depends on: 469581
Depends on: 417516
Attached patch (Av1) Use runreftest.py (obsolete) — Splinter Review
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.
Attachment #364908 - Flags: review?
Attachment #364908 - Flags: review? → review?(ccooper)
Attachment #364908 - Flags: review?(ccooper) → review-
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.
Attached patch (Av2) Use runreftest.py (obsolete) — Splinter Review
Av1, with comment 5 suggestion(s).

Reminder:
I need you/someone to test it, especially on MacOSX trunk and branch.
Attachment #364908 - Attachment is obsolete: true
Attachment #365051 - Flags: review?(ccooper)
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.
Attachment #365051 - Flags: review?(ccooper) → review-
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.
Attached patch (Av3) Reftests part (obsolete) — Splinter Review
Av2, with comment 1 suggestion(s).

Like this ?

Reminder:
I need you/someone to test it, especially on MacOSX trunk and branch.
Assignee: lukasblakk → sgautherie.bz
Attachment #365051 - Attachment is obsolete: true
Attachment #365164 - Flags: review?(ted.mielczarek)
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?
Attached patch Patch 1 (obsolete) — Splinter Review
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?
Attached patch (Av4) Reftests + Mochitests (obsolete) — Splinter Review
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.
Attachment #365164 - Attachment is obsolete: true
Attachment #365853 - Flags: review?(ted.mielczarek)
Attachment #365164 - Flags: review?(ted.mielczarek)
(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.
Attachment #365747 - Flags: review?(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 ." ?
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
Assignee: sgautherie.bz → lukasblakk
Attachment #365747 - Attachment is obsolete: true
Attachment #366548 - Flags: review?(catlee)
Attachment #365747 - Flags: review?(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.
Attachment #366548 - Flags: review?(catlee) → review-
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.
Attachment #365853 - Flags: review?(ted.mielczarek)
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.
Attachment #367618 - Flags: review?(bhearsum)
Attachment #367618 - Flags: review?(bhearsum) → review+
Comment on attachment 367618 [details] [diff] [review]
Remove brand_name from master.cfg for staging and production
[Checkin: Comment 28]

changeset:   1011:eb1b1fc058d3
Attachment #367618 - Flags: checked‑in+ checked‑in+
Attachment #366548 - Attachment description: Patch 2 → Patch 2 [Superseded by bug 445611]
Attachment #366548 - Attachment is obsolete: true
Depends on: 445611
This has been deployed and is running on production and staging unittests.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #365853 - Attachment is obsolete: true
Attachment #367955 - Flags: review?(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 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.
Attachment #367955 - Flags: review?(bhearsum) → review-
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.
Attachment #369094 - Flags: review?(bhearsum)
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.
Attachment #367955 - Attachment is obsolete: true
Attachment #369129 - Flags: review?(bhearsum)
Ev1 unbitrotted
Attachment #369129 - Attachment is obsolete: true
Attachment #369800 - Flags: review?(bhearsum)
Attachment #369129 - Flags: review?(bhearsum)
Attachment #369094 - Flags: review?(bhearsum) → review+
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 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
Attachment #370970 - Attachment description: (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 → (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 [has r=bhearsum]
Comment on attachment 370482 [details] [diff] [review]
(Dv1a) steps.py: Av4 leftovers after bug 445611
[Checkin: See comment 41]

changeset:   245:a6af2fa8c81c
Attachment #370482 - Flags: checked‑in+ checked‑in+
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
Attachment #370970 - Flags: checked‑in+ checked‑in+
Attachment #370970 - Attachment description: (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 [has r=bhearsum] → (Ev1b) factory.py: Av4 leftovers after bug 445611 and bug 474666 [Checkin: See comment 42]
Attachment #370482 - Attachment description: (Dv1a) steps.py: Av4 leftovers after bug 445611 → (Dv1a) steps.py: Av4 leftovers after bug 445611 [Checkin: See comment 41]
Attachment #367618 - Attachment description: Remove brand_name from master.cfg for staging and production → Remove brand_name from master.cfg for staging and production [Checkin: Comment 28]
Flags: wanted1.9.1?
(In reply to comment #19)
> While we're there: "passed" seems inconsistent with "Errno 2" :-/

I filed bug 487361.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.