Closed
Bug 469518
Opened 16 years ago
Closed 16 years ago
Enable Reftest leak log in tinderbox (log)
Categories
(Testing :: Reftest, enhancement)
Testing
Reftest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
(Blocks 1 open bug, )
Details
(Keywords: autotest-issue, fixed1.9.1, Whiteboard: [fixed1.9.1b4])
Attachments
(7 files, 6 obsolete files)
2.86 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
15.33 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
3.12 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
6.32 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
6.96 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.67 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
catlee
:
review+
|
Details | Diff | Splinter Review |
Currently, I use XPCOM_MEM_LEAK_LOG just fine on my local Windows. But I don't know how to properly add it to the tinderbox config. I guess Linux and MacOSX "could" simply use |env XPCOM_MEM_LEAK_LOG=1 ...|; but what about Windows, which needs a separate |set XPCOM_MEM_LEAK_LOG=1| command ? NB: This bug focuses on generating the log asap, automated parsing it could be dealt with later.
Assignee | ||
Comment 1•16 years ago
|
||
Jeff, my first impression is that I'm not convinced that bug 468913 blocks this one: see bug 469581 comment 0 for example. If there is to be a relation between the two, I would make it the other way round: first make it work (= commandline) here, then automate it (= make) there. Here, maybe I was thinking more about reusing/porting the mochitest runtests.py to handle reftests (too): see bug 456414 comment 3.
Comment 2•16 years ago
|
||
I speculate we can reuse runtests.py and/or automation.py to do this, but first step is getting a makefile target so we can do whatever we want without involving IT every time, after changing tinderboxen to use the new command. A makefile target is desirable in its own right, so there's not really a good reason not to make that the interface IT sees by just hacking the environment variable in beforehand (not to mention then you're not sharing existing leak detection infra).
Assignee | ||
Comment 3•16 years ago
|
||
(In reply to comment #1) > Jeff, my first impression is that I'm not convinced that bug 468913 blocks this > one Eventually, bug 468913 created runreftest.py :-) (In reply to comment #2) > I speculate we can reuse runtests.py and/or automation.py to do this, Where could we share leak parsing code between mochitests and reftests ? In automation.py ? > after changing tinderboxen to use the new command. Is that switch done yet ?
Severity: normal → enhancement
No longer depends on: 374458
Keywords: autotest-issue
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #0) > NB: This bug focuses on generating the log asap, automated parsing it could be > dealt with later. This patch adds the logging part (only). I'd like to check it in as is, while we figure out the (automated) parsing part(s).
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #362363 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•16 years ago
|
||
(In reply to comment #3) > Where could we share leak parsing code between mochitests and reftests ? In > automation.py ? While there, I want to share runtests.py "# print test run times" part too.
Assignee | ||
Comment 6•16 years ago
|
||
Av1, using the filename. Looking for review from either of you...
Attachment #362363 -
Attachment is obsolete: true
Attachment #363329 -
Flags: review?(ted.mielczarek)
Attachment #363329 -
Flags: review?(jwalden+bmo)
Attachment #362363 -
Flags: review?(jwalden+bmo)
Comment 7•16 years ago
|
||
I don't get the point of just spewing out the log. It doesn't seem to add any value? Why not just wait until you get the log parsing code hooked up, and we can figure out if we're leaking (and how much) and get some thresholds in place until we can drive the leaks to zero.
Comment 8•16 years ago
|
||
Comment on attachment 363329 [details] [diff] [review] (Av1a) Enable (bare) leak log [Checkin: See comment 10 & 15] r- per my last comment.
Attachment #363329 -
Flags: review?(ted.mielczarek) → review-
Comment 9•16 years ago
|
||
Comment on attachment 363329 [details] [diff] [review] (Av1a) Enable (bare) leak log [Checkin: See comment 10 & 15] Ok, after some IRC discussion, I'm ok with this. Serge says this will help him with filing leak bugs since he'll be able to see the output on tinderbox, despite it being purely informative. + if not os.path.exists(leakLogFile): + log.info("REFTEST WARNING | runreftest.py | Refcount logging is off, so leaks can't be detected!") Since this is purely informative at the moment, I wouldn't bother with the warning. Do we really need to check the leaks in the -silent case? r=me with those addressed
Attachment #363329 -
Flags: review?(jwalden+bmo)
Attachment #363329 -
Flags: review-
Attachment #363329 -
Flags: review+
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 363329 [details] [diff] [review] (Av1a) Enable (bare) leak log [Checkin: See comment 10 & 15] http://hg.mozilla.org/mozilla-central/rev/ff4306c8fe81 Av1a, with comment 9 suggestion(s), after further IRC discussion.
Attachment #363329 -
Attachment description: (Av1a) Enable (bare) leak log → (Av1a) Enable (bare) leak log
[Checkin: See comment 10]
Assignee | ||
Comment 11•16 years ago
|
||
(In reply to comment #3) > > I speculate we can reuse runtests.py and/or automation.py to do this, > > Where could we share leak parsing code between mochitests and reftests ? In > automation.py ? Any answer to guide me ? > > after changing tinderboxen to use the new command. > > Is that switch done yet ? I filed bug 480034. While my previous checkin will already help people locally, it's helpless on tinderboxes without that bug :-|
Comment 12•16 years ago
|
||
automation.py is easy for everyone else to use because they're already using it, I say. We'll get the tinderboxen changed, good news is we're making steady progress here. The end is in sight! :-)
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing]
Assignee | ||
Updated•16 years ago
|
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing] → [needs 1.9.1 landing: after bug 468913]
Assignee | ||
Comment 13•16 years ago
|
||
The only code change is s/threshold = options.leakThreshold/threshold = leakThreshold/. I'll update this code (step by step) in following patches. Also, removes remaining trailing whitespaces.
Attachment #365941 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 14•16 years ago
|
||
Bv1, unbitrotted after bug 480279.
Attachment #365941 -
Attachment is obsolete: true
Attachment #366080 -
Flags: review?(jwalden+bmo)
Attachment #365941 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•16 years ago
|
Attachment #363329 -
Attachment description: (Av1a) Enable (bare) leak log
[Checkin: See comment 10] → (Av1a) Enable (bare) leak log
[Checkin: See comment 10 & 15]
Assignee | ||
Comment 15•16 years ago
|
||
Comment on attachment 363329 [details] [diff] [review] (Av1a) Enable (bare) leak log [Checkin: See comment 10 & 15] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/587ef00fc422
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing: after bug 468913] → [fixed1.9.1b4]
Comment 16•16 years ago
|
||
Comment on attachment 366080 [details] [diff] [review] (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in [Checkin: See comment 17 & 29] Don't fix whitespace except in code you're already touching when you fix bugs. Two-thirds of this patch is whitespace-only changes, and you shouldn't make reviewers read through such changes to get to the actual meat of a patch. > ############### > # RUN THE APP # > ############### > >+def processLeakLog(LEAK_REPORT_FILE, leakThreshold): Usual naming convention has all-caps names as constants, so please rename to leakReportFile.
Attachment #366080 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 17•16 years ago
|
||
Comment on attachment 366080 [details] [diff] [review] (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in [Checkin: See comment 17 & 29] http://hg.mozilla.org/mozilla-central/rev/ac355e4d7c61 (Bv1b) Move leak log parsing code to automation.py.in from runtests.py.in Bv1a, without the whitespace fixes.
Attachment #366080 -
Attachment description: (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in → (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17]
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #16) > >+def processLeakLog(LEAK_REPORT_FILE, leakThreshold): > > Usual naming convention has all-caps names as constants, so please rename to > leakReportFile. This is planned for a following patch.
Whiteboard: [fixed1.9.1b4] → [needs 1.9.1 landing, Bv1b: after bug 460515] [fixed1.9.1b4]
Comment 19•16 years ago
|
||
(In reply to comment #18) > This is planned for a following patch. Sorry, I think that's unacceptable -- fix it in this patch, please.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing, Bv1b: after bug 460515] [fixed1.9.1b4] → [needs 1.9.1 landing, Bv1b: after bug 480279] [fixed1.9.1b4]
Assignee | ||
Comment 20•16 years ago
|
||
Attachment #366375 -
Flags: review?(jwalden+bmo)
Updated•16 years ago
|
Attachment #366375 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 21•16 years ago
|
||
Attachment #366531 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•16 years ago
|
Attachment #366375 -
Attachment description: (Cv1) "Fix" processLeakLog() arguments → (Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22]
Assignee | ||
Comment 22•16 years ago
|
||
Comment on attachment 366375 [details] [diff] [review] (Cv1) "Fix" processLeakLog() arguments [Checkin: Comment 22 & 30] http://hg.mozilla.org/mozilla-central/rev/546d72c0e307
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing, Bv1b: after bug 480279] [fixed1.9.1b4] → [needs 1.9.1 landing, Bv1b, Cv1: after bug 480279] [fixed1.9.1b4]
Comment 23•16 years ago
|
||
bug 479225 went live, and I don't see any leaks on OS X: INFO | (automation.py) | Application ran for: 0:01:03.794933 == BloatView: ALL (cumulative) LEAK STATISTICS nsTraceRefcntImpl::DumpStatistics: 986 entries REFTEST INFO | runreftest.py | Running tests: end. From http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237229668.1237232605.9271.gz&fulltext=1 Is that just not reading the log properly, or is that what an empty log looks like?
Comment 24•16 years ago
|
||
Comment on attachment 366531 [details] [diff] [review] (Dv1) Early return in processLeakLog() [Checkin: See comment 26 & 31] Ware conflicts with bug 482236, of course, when you land this and that.
Attachment #366531 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 25•16 years ago
|
||
(In reply to comment #23) > is that what an empty log looks like? It is! Current status: *FF 3.2: no (more) leaks :-) *FF 3.1: bug 458844, expected as not fixed there. *Bug 466376 is then specific to my computer :-< *Bug 478520: I would still want to get it's leak log, even once only...
Assignee | ||
Comment 26•16 years ago
|
||
Comment on attachment 366531 [details] [diff] [review] (Dv1) Early return in processLeakLog() [Checkin: See comment 26 & 31] http://hg.mozilla.org/mozilla-central/rev/3f15160cf850 after fixing context for { patching file build/automation.py.in Hunk #1 FAILED at 406 1 out of 1 hunks FAILED }
Attachment #366531 -
Attachment description: (Dv1) Early return in processLeakLog() → (Dv1) Early return in processLeakLog()
[Checkin: See comment 26]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing, Bv1b, Cv1: after bug 480279] [fixed1.9.1b4] → [needs 1.9.1 landing, Bv1b, Cv1, Dv1: after bug 480279] [fixed1.9.1b4: Av1b]
This just happened on OS X 510.2 mozilla-central unit test: == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->| Per-Inst Leaked Total Rem Mean StdDev Total Rem Mean StdDev 0 TOTAL -5 0 -1136421943 0 (188643.78 +/- 0.00) 1059492278 0 (21041.09 +/- 0.00) nsTraceRefcntImpl::DumpStatistics: 1180 entries TEST-UNEXPECTED-FAIL | runtests-leaks | negative leaks caught! Related to this bug?
Assignee | ||
Comment 28•16 years ago
|
||
(In reply to comment #27) > Related to this bug? No: that is bug 482236 -> bug 483500.
Comment 29•16 years ago
|
||
Comment on attachment 366080 [details] [diff] [review] (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in [Checkin: See comment 17 & 29] Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b6f75e02c3ad
Attachment #366080 -
Attachment description: (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17] → (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17] [1.9.1 Checkin: See comment 29]
Comment 30•16 years ago
|
||
Comment on attachment 366375 [details] [diff] [review] (Cv1) "Fix" processLeakLog() arguments [Checkin: Comment 22 & 30] Pushed to 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b3e8b948c73
Attachment #366375 -
Attachment description: (Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22] → (Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22] [1.9.1 Checkin: Comment 30]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing, Bv1b, Cv1, Dv1: after bug 480279] [fixed1.9.1b4: Av1b] → [needs 1.9.1 landing, Dv1: after bug 480279] [fixed1.9.1b4: Av1b, Bv1b, Cv1]
Assignee | ||
Comment 31•16 years ago
|
||
Comment on attachment 366531 [details] [diff] [review] (Dv1) Early return in processLeakLog() [Checkin: See comment 26 & 31] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/88b434b46b84
Attachment #366531 -
Attachment description: (Dv1) Early return in processLeakLog()
[Checkin: See comment 26] → (Dv1) Early return in processLeakLog()
[Checkin: See comment 26 & 31]
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite-
Whiteboard: [needs 1.9.1 landing, Dv1: after bug 480279] [fixed1.9.1b4: Av1b, Bv1b, Cv1] → [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1]
Assignee | ||
Comment 32•16 years ago
|
||
(In reply to comment #25) > *FF 3.2: no (more) leaks :-) > *FF 3.1: bug 458844, expected as not fixed there. Ted, given current status of bug 458844 and bug 460548, I'm proposing to do what we had for mochitests: hack a threshold value in runreftest.py on 1.9.1 :-| What do you think ?
Comment 33•16 years ago
|
||
As long as Waldo is ok with it, I'm fine with whatever you need to do.
Assignee | ||
Comment 34•16 years ago
|
||
And keep (improved) dumpLeakLog() around, as it might be (re)used in the future.
Attachment #370745 -
Flags: review?(jwalden+bmo)
Updated•16 years ago
|
Attachment #370745 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 35•16 years ago
|
||
Comment on attachment 370745 [details] [diff] [review] (Ev1) Parse the log and support a leak threshold [Checkin: Comment 35] http://hg.mozilla.org/mozilla-central/rev/c7979ff83ad7
Attachment #370745 -
Attachment description: (Ev1) Parse the log and support a leak threshold → (Ev1) Parse the log and support a leak threshold
[Checkin: Comment 35]
Assignee | ||
Comment 36•16 years ago
|
||
Attachment #371133 -
Flags: review?(ccooper)
Assignee | ||
Comment 37•16 years ago
|
||
Same as Ev1, but with a default threshold.
Attachment #371136 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•16 years ago
|
Attachment #371136 -
Attachment description: (Fv1-191) Parse the log, support and use a leak threshold → (Gv1-191) Parse the log, support and use a leak threshold
Assignee | ||
Comment 38•16 years ago
|
||
Comment on attachment 371136 [details] [diff] [review] (Gv1-191) Parse the log, support and use a leak threshold [Checkin: Comment 39+41] 1.9.1 bug 458844 'nsMathML*' "leak": FF/SM L: 725592 / 725592 FF/SM M: 725592 / 726704 (SM has bug 470710 too) FF/SM W: 738120 / 725592
Updated•16 years ago
|
Attachment #371136 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 39•16 years ago
|
||
Comment on attachment 371136 [details] [diff] [review] (Gv1-191) Parse the log, support and use a leak threshold [Checkin: Comment 39+41] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/76822b1b7bf2
Attachment #371136 -
Attachment description: (Gv1-191) Parse the log, support and use a leak threshold → (Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1] → [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1, Gv1-191]
Assignee | ||
Comment 40•16 years ago
|
||
1.9.1 bug 458844 'nsMathML*' crashtest "leak": FF/SM L: 130844 / 130844 FF/SM M: 130612 / 131724 (SM has bug 470710 too) FF/SM W: 130844 / 130612
Attachment #371812 -
Flags: review?(jwalden+bmo)
Updated•16 years ago
|
Attachment #371133 -
Flags: review?(ccooper) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #371812 -
Attachment description: (Hv1-191) Add a specific crashtest leak threshold → (Hv1-191) Add a specific crashtest leak threshold
[Superseded by bug 391976]
Attachment #371812 -
Attachment is obsolete: true
Attachment #371812 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 41•16 years ago
|
||
Comment on attachment 371136 [details] [diff] [review] (Gv1-191) Parse the log, support and use a leak threshold [Checkin: Comment 39+41] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/467ed372abcd (Hv2-191) Remove forced leak threshold now that bug 458844 is fixed.
Attachment #371136 -
Attachment description: (Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39] → (Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39+41]
Assignee | ||
Updated•16 years ago
|
No longer blocks: 466376
Whiteboard: [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1, Gv1-191] → [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1, Gv1-191, Hv2-191]
Comment 42•16 years ago
|
||
I tested this unbitrotted version of Fv1 today and everything went fine. I tried to get rid of the **kwargs digging but because UnittestBuildFactory explicitly passes env in kwargs we can't just name env and catch an override. Seems silly to block this bug on fixing that, so I just left this code as is and added leakThreshold to the addFactoryArguments call.
Attachment #373664 -
Flags: review?(sgautherie.bz)
Attachment #373664 -
Flags: review?(catlee)
Updated•16 years ago
|
Attachment #371133 -
Attachment is obsolete: true
Assignee | ||
Comment 43•16 years ago
|
||
Comment on attachment 373664 [details] [diff] [review] (Fv1a) MozillaReftest leakThreshold support - unbitrotted >diff -r d160c1da3488 unittest/steps.py >@@ -281,15 +281,21 @@ >+ def __init__(self, test_name, leakThreshold=0, **kwargs): I agree to change the default value for leakThreshold to 0 from None, if you do it for MozillaMochitest too. >+ # Because 'env' is explicitly passed into kwargs we cannot catch it >+ # as a regular arg. Thus, we have to dig into kwargs >+ if not "env" in kwargs: >+ kwargs["env"] = {} Why don't you use the same new code you changed in MozillaMochitest? (As in bug 487496, I'm lost as why same feature gets different codes in different classes :-/) >+ kwargs["EXTRA_TEST_ARGS"] = "--leak-threshold=%d" % leakThreshold Shouldn't that be |kwargs["env"]["EXTRA_TEST_ARGS"]|? I think it's wrong not to add(/keep) |if leakThreshold:| (to the whole block): no need to pass this argument on unless it has a useful value. (Nit: I would prefer you fix my bug 487496 comments there, before we proceed here.)
Comment 44•16 years ago
|
||
Apologies for the noise, this is the correct patch, using leakThreshold=None like the Mochitest step.
Attachment #373664 -
Attachment is obsolete: true
Attachment #373691 -
Flags: review?(sgautherie.bz)
Attachment #373691 -
Flags: review?(catlee)
Attachment #373664 -
Flags: review?(sgautherie.bz)
Attachment #373664 -
Flags: review?(catlee)
Assignee | ||
Comment 45•16 years ago
|
||
Comment on attachment 373691 [details] [diff] [review] (Fv1a) reftest leakThreshold, the correct patch >diff -r ed0581866a90 unittest/steps.py >+ def __init__(self, test_name, leakThreshold=None, **kwargs): If you s/None/0/ for MozillaMochitest in bug 487496, then you could do it here/now too... >+ # Because 'env' is explicitly passed into kwargs we cannot catch it >+ # as a regular arg. Thus, we have to dig into kwargs Please add this comment in bug 487496 too: I do prefer to get exact same code and comment everywhere... >+ "--leak-threshold=%d" % leakThreshold I'd use an indentation of 4 spaces, but as you want.
Assignee | ||
Comment 46•16 years ago
|
||
Comment on attachment 373691 [details] [diff] [review] (Fv1a) reftest leakThreshold, the correct patch r=me, +/- with comment 45 nits.
Attachment #373691 -
Flags: review?(sgautherie.bz) → review+
Comment 47•16 years ago
|
||
(In reply to comment #46) > (From update of attachment 373691 [details] [diff] [review]) > r=me, +/- with comment 45 nits. What are you asking me to do? We're using None for the threshold in bug 487496, the second comment isn't an action item in this bug, and 2 spaces indentation is consistent with the rest of buildbotcustom for a continued line.
Assignee | ||
Comment 48•16 years ago
|
||
(In reply to comment #47) > What are you asking me to do? We're using None for the threshold in bug 487496, Agreed, I made that comment before I eventually figured out that you changed your mind about switching to using 0. > the second comment isn't an action item in this bug, and 2 spaces indentation Well, you're adding a comment here that the same code (now checked in) in MozillaMochitest doesn't have, thus I'd (still) like you to add it at the other occurrence too. > is consistent with the rest of buildbotcustom for a continued line. Then, would you care to fix the/my 4 other occurrences in this file, which use 8 instead of 2? Thanks.
Comment 49•16 years ago
|
||
(In reply to comment #48) > > is consistent with the rest of buildbotcustom for a continued line. > > Then, would you care to fix the/my 4 other occurrences in this file, which use > 8 instead of 2? Thanks. I am not currently on a mission to clean up and make consistent all of this code. I fixed what I was touching.
Comment 50•16 years ago
|
||
Comment on attachment 373691 [details] [diff] [review] (Fv1a) reftest leakThreshold, the correct patch >- def __init__(self, test_name, **kwargs): >- ShellCommandReportTimeout.__init__(self, **kwargs) >- self.addFactoryArguments(test_name=test_name) >- >+ def __init__(self, test_name, leakThreshold=None, **kwargs): > self.name = test_name > self.command = ["make", test_name] > self.description = [test_name + " test"] > self.descriptionDone = [self.description[0] + " complete"] > self.super_class = ShellCommandReportTimeout >+ >+ # Because 'env' is explicitly passed into kwargs we cannot catch it >+ # as a regular arg. Thus, we have to dig into kwargs >+ if leakThreshold: >+ if not "env" in kwargs: >+ kwargs["env"] = {} >+ kwargs["env"]["EXTRA_TEST_ARGS"] = \ >+ "--leak-threshold=%d" % leakThreshold >+ ShellCommandReportTimeout.__init__(self, **kwargs) >+ self.addFactoryArguments(test_name=test_name, >+ leakThreshold=leakThreshold) I think we can clean this up a bit by having env=None in __init__, and then calling ShellCommandReportTimeout.__init__(self, env=env, **kwargs). If you have a parameter called 'env' in your function signature, it will grab anything called 'env' from keyword parameters passed in.
Attachment #373691 -
Flags: review?(catlee) → review-
Comment 51•16 years ago
|
||
Attachment #374126 -
Flags: review?(catlee)
Updated•16 years ago
|
Attachment #373691 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #374126 -
Flags: review?(catlee) → review+
Assignee | ||
Updated•16 years ago
|
Comment 52•16 years ago
|
||
Comment on attachment 374126 [details] [diff] [review] (Fv1b) reftest leakThreshold, without kwargs manipulation [Checkin: Comment 52] changeset: 265:e1ad076706f0
Attachment #374126 -
Attachment description: (Fv1b) reftest leakThreshold, without kwargs manipulation → [checked in] (Fv1b) reftest leakThreshold, without kwargs manipulation
Comment 53•16 years ago
|
||
Comment on attachment 374126 [details] [diff] [review] (Fv1b) reftest leakThreshold, without kwargs manipulation [Checkin: Comment 52] production-master has been reconfig'ed for this patch.
Assignee | ||
Updated•16 years ago
|
Attachment #374126 -
Attachment description: [checked in] (Fv1b) reftest leakThreshold, without kwargs manipulation → (Fv1b) reftest leakThreshold, without kwargs manipulation
[Checkin: Comment 52]
Assignee | ||
Updated•16 years ago
|
Attachment #366080 -
Attachment description: (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17] [1.9.1 Checkin: See comment 29] → (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17 & 29]
Assignee | ||
Updated•16 years ago
|
Attachment #366375 -
Attachment description: (Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22] [1.9.1 Checkin: Comment 30] → (Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22 & 30]
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [Leave open till fully resolved] [fixed1.9.1b4: Av1b, Bv1b, Cv1, Dv1, Gv1-191, Hv2-191] → [fixed1.9.1b4]
You need to log in
before you can comment on or make changes to this bug.
Description
•