[webkit-reviews] review denied: [Bug 44902] new-run-webkit-tests: add more unit tests : [Attachment 66151] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 44902: new-run-webkit-tests: add more unit tests
https://bugs.webkit.org/show_bug.cgi?id=44902

Attachment 66151: Patch
https://bugs.webkit.org/attachment.cgi?id=66151&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
> +    def cleanup(self):
> +	   """Restore logging configuration to its initial settings."""
> +	   _restore_logging(self._old_handler)
> +	   self._old_handler = None
> +
> +    def __del__(self):
> +	   if self._old_handler:
> +	       _restore_logging(self._old_handler)

Maybe __del__ should just call cleanup?  I don't feel strongly about this.

> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
b/WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py

> +    try:
> +	   # Configure the printing subsystem for printing output, logging
debug
> +	   # info, and tracing tests.

As discussed on IRC, in a follow up change, let's try to break this method into
smaller methods so we can reduce the scope of the try/finally block.

> +	   if not options.child_processes:
> +	       # FIXME: Investigate perf/flakiness impact of using cpu_count +
1.
> +	       options.child_processes = port_obj.default_child_processes()

Can we move this to before the try/finally?

> +	   printer = printing.Printer(port_obj, options,
regular_output=regular_output,
> +	       buildbot_output=buildbot_output,
> +	       child_processes=int(options.child_processes),
> +	       is_fully_parallel=options.experimental_fully_parallel)

Can we move this before the try too?  Then in the finally you don't have to
check if printer is None.

> +	   if not options.configuration:
> +	       options.configuration = port_obj.default_configuration()
> +
> +	   if options.pixel_tests is None:
> +	       options.pixel_tests = True
> +
> +	   if not options.use_apache:
> +	       options.use_apache = sys.platform in ('darwin', 'linux2')
> +
> +	   if options.results_directory.startswith("/"):
> +	       # Assume it's an absolute path and normalize.
> +	       options.results_directory = port_obj.get_absolute_path(
> +		   options.results_directory)
> +	   else:
> +	       # If it's a relative path, make the output directory relative to

> +	       # Debug or Release.
> +	       options.results_directory = port_obj.results_directory()

You could move all this options stuff to before the try too.

> +    finally:
> +	   port_obj.stop_helper()
> +	   if test_runner:
> +	       test_runner.cleanup()
> +	   _log.debug("Exit status: %d" % num_unexpected_results)

When this method does a return 0, won't this output -1?  That seems confusing.


Perhaps I have made enough code restructuring suggestions for run() that you
can refactor that in a separate change before this one?  Anyway, your call.