Allow Leak and Bloat tests to have custom prefixes

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: standard8, Assigned: gozer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
Created attachment 344345 [details] [diff] [review]
Use passed-in testname in CompareBloatLogs as prefix to the metric reported

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

9 years ago
Created attachment 344353 [details] [diff] [review]
Add testnameperfix param to CompareLeakLogs

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

9 years ago
Locally applied to our buildbot master, and it's correctly reporting the modified names to the MozillaTest tinderbox.
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 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

9 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 ?
(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

9 years ago
Created attachment 344632 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs

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 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+
Oh, i just to apply this patch and it's now bitrotted :(.
(Assignee)

Comment 11

9 years ago
Created attachment 345287 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v2

Recreated the patch against HEAD, including the stylistic nit
Attachment #344632 - Attachment is obsolete: true
Attachment #345287 - Flags: review?(bhearsum)
Assignee: nobody → gozer
Status: NEW → ASSIGNED
Attachment #345287 - Flags: review?(bhearsum) → review+
Created attachment 345503 [details] [diff] [review]
pass objdir to CompareLeakLogs

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

9 years ago
Attachment #345503 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

9 years ago
Created attachment 345715 [details] [diff] [review]
 add testnameprefix params to Compare{Leak|Bloat}Logs v3

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

9 years ago
removing checkin-needed, as the v2 patch had a typo, v3 should be the one.
Keywords: checkin-needed
(Assignee)

Comment 15

9 years ago
Created attachment 345717 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v4

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

9 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

9 years ago
Created attachment 360931 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v5

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 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 on attachment 360931 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v5

changeset:   188:0b03d75e9d45
Attachment #360931 - Flags: checked‑in+
Looks like this change took.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.