[webkit-reviews] review denied: [Bug 63832] new-run-webkit-tests should support --leaks : [Attachment 101245] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied 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 101245: Patch
https://bugs.webkit.org/attachment.cgi?id=101245&action=review

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


I'm sure you've heard this before, but (as I've frequently been given the
feedback myself), mixing whitespace diffs, and code cleanup along with actual
changes makes reviewing the actual change harder. It'd be great if you could
split the changes into two different patches.

R-'ing because there's at least (I think) one out-and-out typo, and apart from
that it's hard for me to tell what the intended logic of the mods to the base
class are (i.e., what are the semantics of checking for leaks, when can those
methods be called, etc.).

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

What are the actual semantics of this routine? I assume pid must be alive? What
is this supposed to do or return? Is it totally arbitrary?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:296
> +	   # while running the layout tests.

Print to where? Is this a log call? what level?

It might be better if the information was just returned so that the caller
could decide what to do with it (e.g., maybe this should also be stored in
results.json?))

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:43
> +class LeakDetector(object):

I would probably move this into a separate module, e..g, mac_leak_detector.py
or something.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:105
> +	   return "leaks-%s-%s-%s.txt" % (process_name, process_pid,
len(self._leaks_filenames))

Do you need to keep the actual list of filenames? It looks like you're mostly
just using this as a counter and maybe that would be clearer?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:107
> +    def parse_leak_files(self):

don't you need to pass in leak_files here?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:121
> +	   _log.info(" ? checking for leaks in %s" % process_name)

This might want to be log.debug() ... I think this is called in every worker,
right? We probably don't want that being logged all the time.

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:141
> +	       _log.warn(" + %s leaks (%s bytes) were found, details in %s" %
(count, bytes, leaks_output_path))

I'm not sure if you want this to be treated as a warning, or if this should
just be info() ?

warnings are used to indicate that something went wrong with NRWT itself,
usually (but it was a survivable error).

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:250
> +	   _log.warn("%s unique leaks found!" % unique_leaks)

Ugh :(

Fixing this "right" could be annoying, all right. I don't know anything about
run-leaks (and have never used the feature in ORWT); does it run when the
process is still alive, or after the process exits? Does it pick up incremental
leaks since the last run, or anything smart like that?

I can't tell if you are checking for leaks per-test, or per-driver-instance, or
something else, so it's hard to say what the "right" way to fix this is.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:456
>      def restart(self):

Is restart() actually called by anything()? I didn't see any callers in the
currently checked-in code. Does this patch call it from somewhere? If not, you
can probably just delete this whole function.