[webkit-reviews] review denied: [Bug 137667] Make runtest.py call test-webkitpy with the --json flag when in non-interactive mode : [Attachment 239764] Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jake Nielsen
<jake.nielsen.webkit at gmail.com>'s request for review:
Bug 137667: Make runtest.py call test-webkitpy with the --json flag when in
non-interactive mode
https://bugs.webkit.org/show_bug.cgi?id=137667

Attachment 239764: Makes runtest.py call test-webkitpy with the --json flag
when in non-interactive mode
https://bugs.webkit.org/attachment.cgi?id=239764&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239764&action=review


> Tools/Scripts/webkitpy/common/system/executive.py:454
> +	   if teed_output:
> +	       string_io_output = StringIO.StringIO()
> +	       child_output = Tee(string_io_output, sys.stdout)
> +	       print "child_output is of type: "
> +	       print type(child_output)
> +	       exit_code = self._run_command_with_teed_output(args,
child_output, stderr=stderr, stdin=stdin, cwd=cwd, env=env)
> +	       output = string_io_output.getvalue()
> +	   else:
> +	       process = self.popen(args,
> +				    stdin=stdin,
> +				    stdout=self.PIPE,
> +				    stderr=stderr,
> +				    cwd=cwd,
> +				    env=env,
> +				    close_fds=self._should_close_fds())
> +
> +	       output = process.communicate(string_to_communicate)[0]
> +
> +	       # run_command automatically decodes to unicode() unless
explicitly told not to.
> +	       if decode_output:
> +		   output = output.decode(self._child_process_encoding())
> +
> +	       # wait() is not threadsafe and can throw OSError due to:
> +	       # http://bugs.python.org/issue1731717
> +	       exit_code = process.wait()

Can you elaborate on this change? Is this change motivated by the decision to
use run_command() for running the Perl and JavaScript tests?

> Tools/Scripts/webkitpy/tool/steps/runtests.py:65
> +		   filesystem = self._tool.filesystem
> +		   python_unittest_results_directory =
self._tool.port_factory.get().python_unittest_results_directory()
> +		  
filesystem.maybe_make_directory(python_unittest_results_directory)

For your consideration, I suggest moving these lines closer to where we
actually make use of the variable python_unittests_command to improve the
readability of this function, among other benefits.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:85
>		   _log.info("Running Perl unit tests")
> -		  
self._tool.executive.run_and_throw_if_fail(perl_unittests_command,
cwd=self._tool.scm().checkout_root)
> +		   self._tool.executive.run_command(perl_unittests_command,
cwd=self._tool.scm().checkout_root, error_handler=throw_error_handler,
teed_output=True)
>  

Is this change necessary? I mean, it seems outside the scope of this bug
according to the bug description and bug title.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:89
> -		  
self._tool.executive.run_and_throw_if_fail(javascriptcore_tests_command,
quiet=True, cwd=self._tool.scm().checkout_root)
> +		  
self._tool.executive.run_command(javascriptcore_tests_command,
cwd=self._tool.scm().checkout_root, error_handler=throw_error_handler,
teed_output=True)

Ditto.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:96
> -		   self._tool.executive.run_and_throw_if_fail(args,
cwd=self._tool.scm().checkout_root)
> +		   self._tool.executive.run_command(args,
cwd=self._tool.scm().checkout_root, error_handler=throw_error_handler)

Ditto.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:105
> -		   self._tool.executive.run_and_throw_if_fail(args,
cwd=self._tool.scm().checkout_root)
> +		   self._tool.executive.run_command(args,
cwd=self._tool.scm().checkout_root, error_handler=throw_error_handler)

Ditto.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:130
> -	   self._tool.executive.run_and_throw_if_fail(args,
cwd=self._tool.scm().checkout_root)
> +	   self._tool.executive.run_command(args,
cwd=self._tool.scm().checkout_root, error_handler=throw_error_handler)

Ditto.