461041 - Allow Leak and Bloat tests to have custom prefixes

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Philippe M. Chiasson (:gozer)]

Attachment: 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.

Comment 2

[Philippe M. Chiasson (:gozer)]

Attachment: 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)

Comment 3

[Philippe M. Chiasson (:gozer)]

Locally applied to our buildbot master, and it's correctly reporting the modified names to the MozillaTest tinderbox.

Comment 4

[[e-mail] (:bhearsum)]

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.

Comment 5

[[e-mail] (:bhearsum)]

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.

Comment 6

[Philippe M. Chiasson (:gozer)]

(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

[[e-mail] (:bhearsum)]

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

Comment 8

[Philippe M. Chiasson (:gozer)]

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

Comment 9

[[e-mail] (: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.

Comment 10

[[e-mail] (:bhearsum)]

Oh, i just to apply this patch and it's now bitrotted :(.

Comment 11

[Philippe M. Chiasson (:gozer)]

Attachment: add testnameprefix params to Compare{Leak|Bloat}Logs v2

Recreated the patch against HEAD, including the stylistic nit

Comment 12

[[e-mail] (:bhearsum)]

Attachment: 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.

Comment 13

[Philippe M. Chiasson (:gozer)]

Attachment: add testnameprefix params to Compare{Leak|Bloat}Logs v3

Fix a typo in the previous patch. s/prefix/self.testnameprefix/g

Comment 14

[Philippe M. Chiasson (:gozer)]

removing checkin-needed, as the v2 patch had a typo, v3 should be the one.

Comment 15

[Philippe M. Chiasson (:gozer)]

Attachment: add testnameprefix params to Compare{Leak|Bloat}Logs v4

another fine typo was hiding in there aTestname != aTestMame

Comment 16

[[e-mail] (: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.

Comment 17

[Philippe M. Chiasson (:gozer)]

Sure, not a problem, I am running our production buildbot master with that patch locally applied already. So no rush at all.

Comment 18

[Philippe M. Chiasson (:gozer)]

Attachment: 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.

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 360931 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v5

Looks fine to me.

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 360931 [details] [diff] [review]
add testnameprefix params to Compare{Leak|Bloat}Logs v5

changeset:   188:0b03d75e9d45

Comment 21

[[e-mail] (:bhearsum)]

Looks like this change took.