[webkit-reviews] review granted: [Bug 63832] new-run-webkit-tests should support --leaks : [Attachment 101254] Fixed errors pointed out by dirk

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 63832: new-run-webkit-tests should support --leaks
https://bugs.webkit.org/show_bug.cgi?id=63832

Attachment 101254: Fixed errors pointed out by dirk
https://bugs.webkit.org/attachment.cgi?id=101254&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101254&action=review


change basically looks fine. R+-ing / CQ-'ing assuming you will clean this
stuff up.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:291
> +    def check_for_leaks(self, process_name, process_pid):

Please add comments to this and print_leaks_summary() about what the routines
should do (what the contracts are); you answered in the bug comments, but I
would like those answers to be in the code.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:42
> +# If other ports/platforms decide to support --leaks, we should see about
sharing as much of this code as possible.

You are going to move this into a different file, right?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:136
> +	       _log.info("%s leaks (%s bytes including %s excluded leaks) were
found, details in %s" % (adjusted_count, bytes, excluded, leaks_output_path))

Were you going to change these to log.debug() also? These are printed out by
the worker while the driver is actively running, right? Do you want these to be
printed mid-run? (I'm not sure if this is intentional or not). I don't really
have a problem with this being printed mid-run (it's kind of like an unexpected
failure), but it would be good to figure out how to send this data from worker
to manager to not have to log from the worker. You might want this to be a
FIXME or something. Also note that you might want to log a blank line first in
order to deal with the overlapping of progress-meter log messages from the
manager.