Summary: | Make runtest.py call test-webkitpy with the --json flag when in non-interactive mode | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jake Nielsen <jake.nielsen.webkit> | ||||||||||||
Component: | Tools / Tests | Assignee: | Jake Nielsen <jake.nielsen.webkit> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dbates, dfarler, glenn | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 138160 | ||||||||||||||
Attachments: |
|
Description
Jake Nielsen
2014-10-13 13:09:22 PDT
Created attachment 239764 [details]
Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode
bump Comment on attachment 239764 [details] Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode 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. (In reply to comment #3) > (From update of attachment 239764 [details]) > 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? Yes. While I was touching the runtests code, I thought we decided that I should also move to using run_command(). As it turns out, run_command() doesn't tee its output, but run_and_throw_if_fail does, so I decided to add the functionality to run_command. > > > 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. Will do. > > > 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. It's true, but I thought this is what we agreed on in person. > > > 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. Created attachment 239899 [details]
Reorders lines in runtests.py as per comment
Comment on attachment 239899 [details]
Reorders lines in runtests.py as per comment
Sigh, I need to stop doing this. Wrong patch.
Created attachment 239902 [details]
Intended patch
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 239764 [details] [details]) > > 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? > Yes. While I was touching the runtests code, I thought we decided that I should also move to using run_command(). I didn't mean to give the impression that we should take on this undertaking in this bug. It's sufficient to simply use Executive.run_command(), without modification, for the purposes of this bug. In the long term, we should consider switching callers to Executive.run_command() (if appropriate). I say, "if appropriate", because we'll need to audit all the callers of Executive.run_and_throw_if_fail() and determine whether using Executive.run_command() is sensible. If it turns out that many callers need to tee their output to standard output then we should consider either remove the deprecation comment above Executive.run_and_throw_if_fail(), add such support to Executive.run_command(), or come up with an appropriate API. > As it turns out, run_command() doesn't tee its output, but run_and_throw_if_fail does, so I decided to add the functionality to run_command. With respect to the purpose of this patch, is it necessary to print stdout to the screen when we call test-webkitpy —json. I mean, the stdout for test-webkitpy —json isn’t human readable? Comment on attachment 239902 [details] Intended patch View in context: https://bugs.webkit.org/attachment.cgi?id=239902&action=review > Tools/Scripts/webkitpy/tool/steps/runtests.py:69 > + output = self._tool.executive.run_command(python_unittests_command, cwd=self._tool.scm().checkout_root, return_stderr=False, teed_output=True) Suppose one or more Python unit tests fail. Then we will not generate a JSON file because test-webkitpy will return a non-zero exit code and the default error handler will raise a Python exception, which is not caught in this function. Created attachment 240357 [details]
Addresses comments
Comment on attachment 240357 [details] Addresses comments View in context: https://bugs.webkit.org/attachment.cgi?id=240357&action=review > Tools/Scripts/webkitpy/tool/steps/runtests.py:57 > + def squelch_error_handler(script_error): > + pass Notice that the static method Executive.ignore_error is identical to this function. We should make use of Executive.ignore_error() and remove squelch_error_handler(). > Tools/Scripts/webkitpy/tool/steps/runtests.py:69 > + output = self._tool.executive.run_command(python_unittests_command, cwd=self._tool.scm().checkout_root, error_handler=squelch_error_handler, return_stderr=False) As aforementioned above, we should make use of Executive.ignore_error() for the error handler. Created attachment 240369 [details]
Uses Executive.ignore_error
Comment on attachment 240369 [details] Uses Executive.ignore_error View in context: https://bugs.webkit.org/attachment.cgi?id=240369&action=review > Tools/Scripts/webkitpy/tool/steps/runtests.py:68 > + if python_unittests_command: > + python_unittests_command.append('--json') > + _log.info("Running Python unit tests") > + > + 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) > + output = self._tool.executive.run_command(python_unittests_command, cwd=self._tool.scm().checkout_root, error_handler=Executive.ignore_error, return_stderr=False) > + filesystem.write_text_file(filesystem.join(python_unittest_results_directory, "results.json"), output) > + Is there any way to unify this code with the interactive code path for running the Python unit tests? We could do this in a separate bug. (In reply to comment #13) > Comment on attachment 240369 [details] > Uses Executive.ignore_error > > View in context: > https://bugs.webkit.org/attachment.cgi?id=240369&action=review > > > Tools/Scripts/webkitpy/tool/steps/runtests.py:68 > > + if python_unittests_command: > > + python_unittests_command.append('--json') > > + _log.info("Running Python unit tests") > > + > > + 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) > > + output = self._tool.executive.run_command(python_unittests_command, cwd=self._tool.scm().checkout_root, error_handler=Executive.ignore_error, return_stderr=False) > > + filesystem.write_text_file(filesystem.join(python_unittest_results_directory, "results.json"), output) > > + > > Is there any way to unify this code with the interactive code path for > running the Python unit tests? We could do this in a separate bug. Jake Nielsen and I discussed this issue on IRC last Thursday (10/23) and Jake suggested that we file a separate clean up bug to unify the code. (In reply to comment #14) > [...] > Jake Nielsen and I discussed this issue on IRC last Thursday (10/23) and > Jake suggested that we file a separate clean up bug to unify the code. Filed bug #138160. Comment on attachment 240369 [details] Uses Executive.ignore_error Clearing flags on attachment: 240369 Committed r175294: <http://trac.webkit.org/changeset/175294> All reviewed patches have been landed. Closing bug. |