Closed
Bug 450983
Opened 16 years ago
Closed 16 years ago
use leakThreshold for SeaMonkey testers
Categories
(SeaMonkey :: Build Config, defect)
SeaMonkey
Build Config
Tracking
(Not tracked)
VERIFIED
FIXED
seamonkey2.0b1
People
(Reporter: kairo, Assigned: kairo)
References
(Depends on 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
Callek
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kairo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details |
Filing this as a new bug to use what bug 448416 implemented on the SeaMonkey tester buildbots, as this doesn't actually fix bug 448125 - it only hides the leaks but doesn't actually fix them.
![]() |
Assignee | |
Comment 1•16 years ago
|
||
This might seem over-engineered but it makes the task easy to later reduce the thresholds until we come down to zero. It also codifies the currently seems numbers of leaks as thresholds for the moment. We surely want to bring those down, that's what bug 448125 is still here for.
Comment 2•16 years ago
|
||
Comment on attachment 334205 [details] [diff] [review] make thresholds configurable in config.py [Checkin: Comment 4] My only nit is I think it would be easier to visualize and edit if you group the platform leak Values together in the config.py rather than by test type.
Updated•16 years ago
|
Attachment #334205 -
Flags: review?(bugspam.Callek) → review+
Comment 3•16 years ago
|
||
My review did not include actually verifying the leak values being entered. I will double check soon anyway, and I do feel that tweaking these by you can have an rs+ from me for initial green; and rs+ for tuning them down to 0.
![]() |
Assignee | |
Comment 4•16 years ago
|
||
OK, checked in the current state, and did pull + reconfig on the master. The next cycle should pick this up, let's see how green it turns.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 5•16 years ago
|
||
I'll at least reopen this as the values don't get through to the actual mochitest runs. It may be that it's actually bug 448416 that wasn't fixed correctly though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 6•16 years ago
|
||
From some investigation Bug 448416 was fixed correctly, but sadly it does not really fix us, as we currently need to set command manually for all these ourselves, (to pass in correct --appname) The easy fix is to manually append each command in our buildbot config with a --leakThreshold option for now. The slightly harder and probably more correct fix is to extend the buildbotcustom code to allow specifying an appname as well. I'll eventually get this done but I suspect at least a week before I can develop the patch, not counting review. my suggestion is to manually set/hack in the leakThreshold option to these and once I get the appname patch done/approved/etc we can revert that hack and do it the right way.
Comment 7•16 years ago
|
||
I don't think passing in appName should be necessary at all, as automation.py already knows this information. I think its presence in the unittest configs is an accident. http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80
![]() |
Assignee | |
Comment 8•16 years ago
|
||
I actually think we should change the implementation of all this over to the new make targets and pass the leakThreshold as a make variable.
Comment 9•16 years ago
|
||
(In reply to comment #7) > I don't think passing in appName should be necessary at all, as automation.py > already knows this information. I think its presence in the unittest configs is > an accident. > http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80 > We don't run pgo and I don't see how those are easily available to other portions of the tree, perhaps they should be? (In reply to comment #8) > I actually think we should change the implementation of all this over to the > new make targets and pass the leakThreshold as a make variable. > I could be wrong as I haven't studied those build targets heavily yet, but I don't remember leakThreshold being an option using that method. Either way, I still feel that my hackier way is best for the moment (we really need to catch for increases in leaks sooner than later)
Comment 10•16 years ago
|
||
(In reply to comment #9) > (In reply to comment #7) > > I don't think passing in appName should be necessary at all, as automation.py > > already knows this information. I think its presence in the unittest configs is > > an accident. > > http://mxr.mozilla.org/mozilla-central/source/build/pgo/automation.py.in#80 > > > > We don't run pgo and I don't see how those are easily available to other > portions of the tree, perhaps they should be? automation.py is used by runtests.py, it's just a library. It happens to live in build/pgo because it's also used by the pgo profiler script.
Comment 11•16 years ago
|
||
After Bug 451812 is fixed, this should turn our buildbots green.
Attachment #335147 -
Flags: review?(kairo)
Updated•16 years ago
|
Attachment #334205 -
Attachment description: make thresholds configurable in config.py → make thresholds configurable in config.py
[Checkin: Comment 4]
Comment 12•16 years ago
|
||
(In reply to comment #11) > Created an attachment (id=335147) [details] > After Bug 451812 is fixed, this should turn our buildbots green. (Dumb question: exact same patch file as in that other bug !?) (I'm wondering further as it looks like a Hg patch to a cvs repository: http://mxr.mozilla.org/mozilla/source/tools/buildbotcustom/unittest/steps.py)
Comment 13•16 years ago
|
||
...this should be the right patch, thanks Gerv.
Attachment #335147 -
Attachment is obsolete: true
Attachment #335169 -
Flags: review?(kairo)
Attachment #335147 -
Flags: review?(kairo)
![]() |
Assignee | |
Comment 15•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] looks good to me - if bug 451812 gets r+ Have you verified that automation.py looks for the right out of "seamonkey-bin", "seamonkey.exe" and "seamonkey" (Mac/Win/unix)? I'm mostly concerned about the Mac case as FF may actually ship and possibly use a "firefox" script there even if it's unneeded.
Attachment #335169 -
Flags: review?(kairo) → review+
Comment 16•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] Hmm, I actually have not tested mac case... stefanh can you please test that |python runtests.py --autorun --console-level=INFO --close-when-done| works from |build/objdir/mozilla/_tests/testing/mochitest| on Mac for me and if so, grant review here.
Attachment #335169 -
Flags: review?(stefanh)
Comment 17•16 years ago
|
||
For the record, before this bug is fixed, there are already some leaks reduced to warning only: Linux comm-central dep unit test on 2008/08/23 23:31:34 mochitest TEST-PASS | runtests-leaks | WARNING leaked 143568 bytes during test execution Linux / MacOSX / Windows browser TEST-PASS | runtests-leaks | WARNING leaked 8... bytes during test execution NB: Though I don't know why. I guess there already some settings somewhere !?
Comment 18•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] rbgray over IRC verified that what I asked stefanh to do works, great!
Attachment #335169 -
Flags: review?(stefanh)
Comment 19•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] $ hg push pushing to ssh://hg.mozilla.org/build/buildbot-configs searching for changes remote: adding changesets remote: adding manifests remote: adding file changes remote: added 1 changesets with 1 changes to 1 files $ hg tip -v changeset: 246:7682755245d4 tag: tip user: Justin Wood <Callek@gmail.com> date: Mon Aug 25 18:53:31 2008 -0400 files: seamonkey-unittest/master.cfg description: Bug 450983, followup -- do not pass command to buildsteps for mochikit steps r=KaiRo
Attachment #335169 -
Attachment description: real patch for followup → real patch for followup [checked in]
Comment 20•16 years ago
|
||
(leaving bug open until KaiRo pushes these changes to buildbots themselves)
![]() |
Assignee | |
Comment 21•16 years ago
|
||
the change did not work, we ended up with strange commandlines like in http://cn-sea-qm-centos5-01.nl.mozilla.org:8010/builders/MacOSX%2010.4%20comm-central%20dep%20unit%20test/builds/590/steps/mochitest/logs/stdio so I went back to the changeset before this was checked in to get the buildbots running again. Could you back out your patch and try to figure out the problem?
Comment 22•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] backed out per buildbots failing oddly, KaiRo has had them running without this changeset for a bit now.
Attachment #335169 -
Attachment description: real patch for followup [checked in] → real patch for followup [checked in] [backed out]
![]() |
Assignee | |
Comment 23•16 years ago
|
||
I checked in http://hg.mozilla.org/build/buildbot-configs/rev/e2205fdd23c7 for now, setting leakThreshold directly in the commands we're calling. I hope it works, but it's only a temporary fix until we get a real one. Maybe the buildbotcustom instance should always set --leaThreshold and just default the actual value to 0 unless something else is handed to the buildstep?
![]() |
Assignee | |
Comment 24•16 years ago
|
||
We gained 240 bytes on mac and 232 bytes on Windows in mochitest leaks since comment #1 (Linux has some much higher value indicating a larger additional problem there). On mochichrome we gained 140 bytes on Linux, 132 on Windows, and also 140 on Mac. mochibrowser has a real test failure, so leak reports might not be accurate. I'm upping the thresholds for the mochitest/mochichrome test, but the difference numbers reported here might give a pointer to what actually got added, so I wanted to note them here.
Comment 25•16 years ago
|
||
(In reply to comment #24) NB: "gain" meaning "more" leaks newly happening (or simply detected). > a pointer to what actually got added I think unless we would have a graphic (or even text) report [hint !] like nye has, chasing in (tinderbox waterfall, 3 weeks) history would be too painful. As these leaks are not even reported on the waterfall itself [hint !], I think this will help for now on (only). Yet, that's just what I've been hoping for all along ;-)
Comment 27•16 years ago
|
||
Apart from the 2 usual MacOSX failing tests, we have 1 new failing test (bug 454513) and 1 new leak (bug 454524), which keep the boxes Orange even after comment 23 checkin; I would suggest to not workaround them immediately, until we see if we can get them fixed.
![]() |
Assignee | |
Comment 28•16 years ago
|
||
I added the current amount of mochibrowser leaks to the config.py and will reconfig soon to make it live, as it's better to actually see boxes going from green to orange when something new is introduced as to stay in indefinite orange.
Comment 29•16 years ago
|
||
(In reply to comment #25) > I think unless we would have a graphic (or even text) report [hint !] As Callek asked me, I filed Tb bug 456274, Tb bug 456275 and SM bug 456279.
Comment 30•16 years ago
|
||
Thanks to fixing bug 433132, we can reduce the threshold on the Linux test box: { mochitest http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1222418644.1222423001.26272.gz&fulltext=1 Linux comm-central dep unit test on 2008/09/26 01:44:04 TEST-PASS | runtests-leaks | WARNING leaked 143116 bytes during test execution http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1222426721.1222431311.32673.gz&fulltext=1 Linux comm-central dep unit test on 2008/09/26 03:58:41 TEST-PASS | runtests-leaks | WARNING leaked 21416 bytes during test execution } Now, we have (what seems to be) the same leak reports on all 3 platforms :-) *** The MacOSX and Windows boxes didn't report bug 433122 :-/ (Which I could reproduce on my local Windows.) Do they have a (dumb) printer set up ? ...
![]() |
Assignee | |
Comment 31•16 years ago
|
||
They probably have no printer set up - none of our boxes is actually connected to one. The reason why Linux catches the bug might be that Linux always has the "print to PDF" stuff that GNOME gives us. :) I'll do the threshold change when I'm back on my normal machine, probably Monday.
Comment 32•16 years ago
|
||
(In reply to comment #30) > The MacOSX and Windows boxes didn't report bug 433122 :-/ Bug 433132 ! (In reply to comment #31) > The reason why Linux catches the bug might be that Linux always has the > "print to PDF" stuff that GNOME gives us. :) Actually, that's the reason why I couldn't work on that bug until recently, when I installed the (Windows) doPDF utility ;-)
Updated•16 years ago
|
Comment 33•16 years ago
|
||
(In reply to comment #31) > I'll do the threshold change when I'm back on my normal machine, probably > Monday. Done by bug 457440 comment 1.
Comment 34•16 years ago
|
||
I noticed that we have a small but consistent improvement on mochi+browser+chrome mochitests: Linux -80, Mac and Win -76.
Comment 35•16 years ago
|
||
(In reply to comment #34) > I noticed that we have a small but consistent improvement on > mochi+browser+chrome mochitests: > Linux -80, Mac and Win -76. Robert, Justin, could you update these chrome and browser thresholds ? mochi has regressed in the meantime: see bug 463355.
Depends on: 463355
Flags: wanted1.9.1?
Updated•16 years ago
|
![]() |
Assignee | |
Comment 36•16 years ago
|
||
I adjusted our thresholds for this now, see http://hg.mozilla.org/build/buildbot-configs/rev/58af505b0e9e
Comment 37•16 years ago
|
||
(In reply to comment #21) > http://cn-sea-qm-centos5-01.nl.mozilla.org:8010/builders/MacOSX%2010.4%20comm-central%20dep%20unit%20test/builds/590/steps/mochitest/logs/stdio This link is HTTP 404 now :-( > try to figure out the problem? Guessing: maybe related to bug 456281 comment 10 ??
Comment 38•16 years ago
|
||
See bug 391318 comment 45 for a brief explanation of what got fixed. KaiRo, can you update our thresholds per "Current state" ? Thanks.
![]() |
Assignee | |
Comment 39•16 years ago
|
||
http://hg.mozilla.org/build/buildbot-configs/rev/88ee32f431b0 adjusts thresholds to new levels after fixes for both bug 465556 and bug 391318. OS X leaks 1112+8 on mochitest and 1112+0 on mochichrome, and even still leaks slightly more than the others on browser chrome tests - the 1112 seems to be some magtc number across the machines ;-)
Comment 40•16 years ago
|
||
(In reply to comment #39) I filed bug 470709 about the remaining BrowserChrome leak. > OS X leaks [...] more [...] I filed bug 470710 about the MacOSX case.
![]() |
Assignee | |
Comment 41•16 years ago
|
||
We're using leak thresholds correctly with current configs.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Attachment #335147 -
Attachment description: followup, don't pass command → followup, don't pass command
[Bug 451812 patch]
Comment 42•16 years ago
|
||
Comment on attachment 335169 [details] [diff] [review] real patch for followup [Checkin: See comment 42] Ftr, checkin: comment 19, backout: comment 22; eventually done by http://hg.mozilla.org/build/buildbot-configs/rev/86561671f4cc and http://hg.mozilla.org/build/buildbotcustom/rev/4a337a624db2
Attachment #335169 -
Attachment description: real patch for followup [checked in] [backed out] → real patch for followup
[Checkin: See comment 42]
Comment 43•16 years ago
|
||
V.Fixed
Status: RESOLVED → VERIFIED
Flags: wanted1.9.1? → in-testsuite-
Target Milestone: --- → seamonkey2.0b1
You need to log in
before you can comment on or make changes to this bug.
Description
•