Closed
Bug 461041
Opened 15 years ago
Closed 14 years ago
Allow Leak and Bloat tests to have custom prefixes
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: gozer)
References
Details
Attachments
(2 files, 6 obsolete files)
902 bytes,
patch
|
coop
:
review+
|
Details | Diff | Splinter Review |
8.02 KB,
patch
|
bhearsum
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
We are currently enabling Leak & Bloat tests for Thunderbird. The problem with the current CompareBloatLogs and CompareLeakLogs in buildbotcustom's test.py (link below) is that they hard-code the test names. We want to be able to add a prefix to these tests, e.g. "Mail ". This will enable us to properly distinguish between the different types of test, and allow us to be able to report to the graph server with distinct, separate names. http://mxr.mozilla.org/seamonkey/source/tools/buildbotcustom/steps/test.py
Assignee | ||
Comment 1•15 years ago
|
||
Currently, CompareBloatLogs does take in as an argument the 'name' of the test (testname argument) and uses it only for cosmetic reasons. This patch prefixes that test name to all generated metrics.
Attachment #344345 -
Flags: review?(bhearsum)
Assignee | ||
Comment 2•15 years ago
|
||
Given that CompareLeakLogs already uses testname to derive some logic ('current' vs. 'previous') this patch adds a testnameprefix to CompareLeakLogs that allows renaming all metrics (Lk, MH, A) with the given prefix (i.e. Mail)
Attachment #344353 -
Flags: review?(bhearsum)
Assignee | ||
Comment 3•15 years ago
|
||
Locally applied to our buildbot master, and it's correctly reporting the modified names to the MozillaTest tinderbox.
Comment 4•15 years ago
|
||
Comment on attachment 344345 [details] [diff] [review] Use passed-in testname in CompareBloatLogs as prefix to the metric reported >Index: steps/test.py >=================================================================== >- leaksAbbr = "RLk"; >- leaksTestname = "refcnt_leaks" >- leaksTestnameLabel = "refcnt Leaks" >+ leaksAbbr = "%sRLk" % self.testname >+ leaksTestname = "%srefcnt_leaks" %self.testname >+ leaksTestname.replace(' ', '_') Strings are immutable in Python. ITYM, leaksTestname = leaktsTestname.replace(' ', '_'). >- self.setProperty('testresults', [("RLk", "refcnt_leaks", formatBytes(leaks,3))]) >+ self.setProperty('testresults', [(leaksAbbr, leaksTestname, formatBytes(leaks,3))]) I think this is bitrotted now =\. Please add testname as an explicit, optional parameter, defaulting to an empty string. We have existing lines on the graphs for RLk (and other tests), and I think it's best we don't rename them.
Attachment #344345 -
Flags: review?(bhearsum) → review-
Comment 5•15 years ago
|
||
Comment on attachment 344353 [details] [diff] [review] Add testnameperfix param to CompareLeakLogs >Index: steps/test.py >=================================================================== > kwargs['command'] = ['obj-firefox\\dist\\bin\\leakstats.exe', > kwargs['mallocLog']] > else: >- kwargs['command'] = ['obj-firefox/dist/bin/leakstats', >+ #kwargs['command'] = ['obj-firefox/dist/bin/leakstats', >+ #XXX: Hack-hack-hack >+ kwargs['command'] = ['objdir-tb/mozilla/dist/bin/leakstats', > kwargs['mallocLog']] Oh god, I'd forgotten that this was in there. Let's take this opportunity to fix it by passing in 'objdir' and use kwargs['command'] = ['%s/dist/bin/leakstats' % self.objdir] - (you'll want to pass in objdir-tb/mozilla in your own buildbot, of course). My comments about the other patch apply here, too.
Attachment #344353 -
Flags: review?(bhearsum) → review-
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #5) > (From update of attachment 344353 [details] [diff] [review]) > >Index: steps/test.py > >=================================================================== > > kwargs['command'] = ['obj-firefox\\dist\\bin\\leakstats.exe', > > kwargs['mallocLog']] > > else: > >- kwargs['command'] = ['obj-firefox/dist/bin/leakstats', > >+ #kwargs['command'] = ['obj-firefox/dist/bin/leakstats', > >+ #XXX: Hack-hack-hack > >+ kwargs['command'] = ['objdir-tb/mozilla/dist/bin/leakstats', > > kwargs['mallocLog']] > > Oh god, I'd forgotten that this was in there. And bad on my part, I didn't mean to include that portion into my submitted patch. Still, worth talking about ;-) Let's take this opportunity to > fix it by passing in 'objdir' and use > kwargs['command'] = ['%s/dist/bin/leakstats' % self.objdir] - (you'll want to > pass in objdir-tb/mozilla in your own buildbot, of course). Yup, that works for me. Okay with a patch that defaults objdir to 'obj-firefox' so as not to break existing firefox buildbots ? Or just bite the bullet and make it required ?
Comment 7•15 years ago
|
||
(In reply to comment #6) > Yup, that works for me. Okay with a patch that defaults objdir to 'obj-firefox' > so as not to break existing firefox buildbots ? Or just bite the bullet and > make it required ? Let's just bite the bullet here. Don't worry about our buildbot configs - I'll fix them up.
Assignee | ||
Comment 8•15 years ago
|
||
This cleaned up patch includes all comments on the previous patches, and also adds the required 'objdir' paramater to CompareLeakLogs
Attachment #344345 -
Attachment is obsolete: true
Attachment #344353 -
Attachment is obsolete: true
Attachment #344632 -
Flags: review?(bhearsum)
Comment 9•15 years ago
|
||
Comment on attachment 344632 [details] [diff] [review] add testnameprefix params to Compare{Leak|Bloat}Logs >Index: steps/test.py >=================================================================== >- leaksAbbr = "RLk"; >- leaksTestname = "refcnt_leaks" >- leaksTestnameLabel = "refcnt Leaks" >+ leaksAbbr = "%sRLk" % self.testnameprefix >+ leaksTestname = ("%srefcnt_leaks" %self.testnameprefix).replace(' ', '_') style nit: it's more readable with a space after the '%'. If this pushes it beyond the 80th char just do some of it on the next line. r=bhearsum with that change.
Attachment #344632 -
Flags: review?(bhearsum) → review+
Comment 10•15 years ago
|
||
Oh, i just to apply this patch and it's now bitrotted :(.
Assignee | ||
Comment 11•15 years ago
|
||
Recreated the patch against HEAD, including the stylistic nit
Attachment #344632 -
Attachment is obsolete: true
Attachment #345287 -
Flags: review?(bhearsum)
Updated•15 years ago
|
Assignee: nobody → gozer
Status: NEW → ASSIGNED
Updated•15 years ago
|
Attachment #345287 -
Flags: review?(bhearsum) → review+
Comment 12•15 years ago
|
||
This patch is needed before we update test.py on the Firefox Buildbots. I need to test it on staging-master before landing.
Attachment #345503 -
Flags: review?(ccooper)
Updated•15 years ago
|
Attachment #345503 -
Flags: review?(ccooper) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•15 years ago
|
||
Fix a typo in the previous patch. s/prefix/self.testnameprefix/g
Attachment #345287 -
Attachment is obsolete: true
Attachment #345715 -
Flags: review?(bhearsum)
Assignee | ||
Comment 14•15 years ago
|
||
removing checkin-needed, as the v2 patch had a typo, v3 should be the one.
Keywords: checkin-needed
Assignee | ||
Comment 15•15 years ago
|
||
another fine typo was hiding in there aTestname != aTestMame
Attachment #345715 -
Attachment is obsolete: true
Attachment #345717 -
Flags: review?(bhearsum)
Attachment #345715 -
Flags: review?(bhearsum)
Comment 16•15 years ago
|
||
Comment on attachment 345717 [details] [diff] [review] add testnameprefix params to Compare{Leak|Bloat}Logs v4 Any chance we can hold off landing this until next week? I'd like a chance to verify my patch for the Firefox Buildbots first.
Attachment #345717 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 17•15 years ago
|
||
Sure, not a problem, I am running our production buildbot master with that patch locally applied already. So no rush at all.
Assignee | ||
Comment 18•14 years ago
|
||
Updated diff to buildbotcustom in hg, and included the small change for bug 462540 as they are intermingled.
Attachment #345717 -
Attachment is obsolete: true
Attachment #360931 -
Flags: review?(bhearsum)
Comment 19•14 years ago
|
||
Comment on attachment 360931 [details] [diff] [review] add testnameprefix params to Compare{Leak|Bloat}Logs v5 Looks fine to me.
Attachment #360931 -
Flags: review?(bhearsum) → review+
Comment 20•14 years ago
|
||
Comment on attachment 360931 [details] [diff] [review] add testnameprefix params to Compare{Leak|Bloat}Logs v5 changeset: 188:0b03d75e9d45
Attachment #360931 -
Flags: checked‑in+
Comment 21•14 years ago
|
||
Looks like this change took.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•