Bug 65931 – Leaks bot shows python logging prefixes as part of summary display

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

Leaks bot shows python logging prefixes as part of summary display

Right now the summary line looks like this:
#18494	Failed 2011-08-09 11:05:43,602 34458 mac.py:275 info leaks found for a total of 150,368 bytes! 2011-08-09 11:05:43,602 34458 mac.py:276 info 1 unique leaks found! 4 failures 1 flakes run-api-tests

When ORWT looked like this:
Failed 1400 total leaks found! 25 test cases (<1%) had incorrect layout 1 test case (<1%) crashed run-api-tests

After this patch it will look like:
#18494	Failed leaks found for a total of 150,368 bytes! 1 unique leaks found! 4 failures 1 flakes run-api-tests

Which isn't perfect, but much better than before.


In writing this change, I was sure I would make a typo or two, so I had to invent a way to test master.cfg changes.

That turned out to be ridiculously involved, as loading master.cfg not only required a buildbot install (I tried mocking it out at first, but that didn't work), but also required the ability to load/execute python files which have dots in their names and do not end in .py!

It's unclear if we really wanted to add buildbot to autoinstalled.  However normal webkitpy users won't notice any change, only ones running mastercfg_unittest.py will ever cause buildbot to autoinstall.

[Eric Seidel (no email)]

Created attachment 103378 [details]
Patch

[William Siegrist]

Comment on attachment 103378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103378&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:270
> +

You might want to consider pre-compiling the regex via re.compile() above this and pass it as a parameter to _strip_python_logging_prefix() so we're not parsing it for every line of log text

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:729
> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))

FWIW, the master runs Python 2.6 which has a built-in json module which could be used to replace simplejson.

[Eric Seidel (no email)]

Comment on attachment 103378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103378&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:270
>> +
> 
> You might want to consider pre-compiling the regex via re.compile() above this and pass it as a parameter to _strip_python_logging_prefix() so we're not parsing it for every line of log text

Currently it only runs on lines which have already matched the "exiting early" or "leaks found" regexp (up to 2 per log), but I'm happy to pre-compile it in preparation for any future use.

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:729
>> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))
> 
> FWIW, the master runs Python 2.6 which has a built-in json module which could be used to replace simplejson.

For whatever (probably just historical) reason, master.cfg uses simplejson explicitly.  Built-in json also spits out unicode strings in newer versions of python, but the most recent versions of 2.6/2.7 have this bug fixed.  I figured this was a relatively easy fix for now, but I'm happy to do the:

try:
   import json
except:
   import simpljson as json

trick that the rest of the webkitpy code does if you'd prefer.

[William Siegrist]

I don't have a strong desire for either of my comments to be acted upon, but you can assume the master always runs on python 2.6 or newer now. Simplejson was needed back when the master ran python 2.5.

[Eric Seidel (no email)]

Created attachment 103390 [details]
Pre-compile regexp per bill's suggestion

[Eric Seidel (no email)]

(In reply to comment #4)
> I don't have a strong desire for either of my comments to be acted upon, but you can assume the master always runs on python 2.6 or newer now. Simplejson was needed back when the master ran python 2.5.

Good to know.  I just tried moving everything to 'json' and hit the same unicode problem.  It seems that json in Python 2.6.1 returns unicode, but the unicode-keywords bug hadn't been fixed yet.  So I've left the code as-posted for now.

[Adam Barth]

Comment on attachment 103390 [details]
Pre-compile regexp per bill's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=103390&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:731
> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))

Can you break this out into a free function?

[Eric Seidel (no email)]

Comment on attachment 103390 [details]
Pre-compile regexp per bill's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=103390&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:731
>> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))
> 
> Can you break this out into a free function?

I can.  I'm not sure what you're looking for though.  Could you suggest a signature?  Unless you feel strongly I'm inclined to just land it as-is.

[Adam Barth]

> I can.  I'm not sure what you're looking for though.  Could you suggest a signature?  Unless you feel strongly I'm inclined to just land it as-is.

convert_dictionary_keys_to_strings

[Eric Seidel (no email)]

Comment on attachment 103390 [details]
Pre-compile regexp per bill's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=103390&action=review

>>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:731
>>> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))
>> 
>> Can you break this out into a free function?
> 
> I can.  I'm not sure what you're looking for though.  Could you suggest a signature?  Unless you feel strongly I'm inclined to just land it as-is.

I tried various flavors just now (both lambda and truly free function) and both read worse to me.  Since this is the only use for now, I'd like to leave it as is.

[Adam Roben (:aroben)]

Comment on attachment 103390 [details]
Pre-compile regexp per bill's suggestion

View in context: https://bugs.webkit.org/attachment.cgi?id=103390&action=review

>>>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:731
>>>> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))
>>> 
>>> Can you break this out into a free function?
>> 
>> I can.  I'm not sure what you're looking for though.  Could you suggest a signature?  Unless you feel strongly I'm inclined to just land it as-is.
> 
> I tried various flavors just now (both lambda and truly free function) and both read worse to me.  Since this is the only use for now, I'd like to leave it as is.

Seems like you can at least use argument unpacking to make this a little more palatable:

scheduler = dict(map(lambda (k, v): (str(k), v), scheduler.items()))

Or maybe a generator expression would be even more readable:

scheduler = dict((str(k), v) for (k, v) in scheduler.iteritems())

[WebKit Review Bot]

Comment on attachment 103390 [details]
Pre-compile regexp per bill's suggestion

Clearing flags on attachment: 103390

Committed r92772: <http://trac.webkit.org/changeset/92772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

(In reply to comment #11)
> (From update of attachment 103390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103390&action=review
> 
> >>>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:731
> >>>> +        scheduler = dict(map(lambda key_value_pair: (str(key_value_pair[0]), key_value_pair[1]), scheduler.items()))
> >>> 
> >>> Can you break this out into a free function?
> >> 
> >> I can.  I'm not sure what you're looking for though.  Could you suggest a signature?  Unless you feel strongly I'm inclined to just land it as-is.
> > 
> > I tried various flavors just now (both lambda and truly free function) and both read worse to me.  Since this is the only use for now, I'd like to leave it as is.
> 
> Seems like you can at least use argument unpacking to make this a little more palatable:
> 
> scheduler = dict(map(lambda (k, v): (str(k), v), scheduler.items()))

Oh, I tried:

dict(map(lambda k, v: (str(k), v), scheduler.items()))

And it complained about lambda only getting one argument when it expected 2.  But it's possible I needed to wrap my lamba arguments in () to make it work.

> Or maybe a generator expression would be even more readable:
> 
> scheduler = dict((str(k), v) for (k, v) in scheduler.iteritems())

Agreed.