Bug 137667

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 / TestsAssignee: 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 Flags
Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode
dbates: review-, dbates: commit-queue-
Reorders lines in runtests.py as per comment
jake.nielsen.webkit: review-, jake.nielsen.webkit: commit-queue-
Intended patch
dbates: review-, dbates: commit-queue-
Addresses comments
dbates: review+, dbates: commit-queue-
Uses Executive.ignore_error none

Description Jake Nielsen 2014-10-13 13:09:22 PDT
runtest.py currently only calls test-webkitpy in the interactive case. Now that test-webkitpy has a --json flag, it should use it in the non-interactive case.
Comment 1 Jake Nielsen 2014-10-13 17:25:37 PDT
Created attachment 239764 [details]
Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode
Comment 2 Jake Nielsen 2014-10-15 13:16:37 PDT
bump
Comment 3 Daniel Bates 2014-10-15 14:26:59 PDT
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.
Comment 4 Jake Nielsen 2014-10-15 15:10:28 PDT
(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.
Comment 5 Jake Nielsen 2014-10-15 15:12:37 PDT
Created attachment 239899 [details]
Reorders lines in runtests.py as per comment
Comment 6 Jake Nielsen 2014-10-15 15:17:25 PDT
Comment on attachment 239899 [details]
Reorders lines in runtests.py as per comment

Sigh, I need to stop doing this. Wrong patch.
Comment 7 Jake Nielsen 2014-10-15 15:20:42 PDT
Created attachment 239902 [details]
Intended patch
Comment 8 Daniel Bates 2014-10-15 15:35:29 PDT
(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 9 Daniel Bates 2014-10-15 15:43:45 PDT
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.
Comment 10 Jake Nielsen 2014-10-23 11:39:17 PDT
Created attachment 240357 [details]
Addresses comments
Comment 11 Daniel Bates 2014-10-23 13:10:47 PDT
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.
Comment 12 Jake Nielsen 2014-10-23 14:48:35 PDT
Created attachment 240369 [details]
Uses Executive.ignore_error
Comment 13 Daniel Bates 2014-10-23 16:23:52 PDT
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.
Comment 14 Daniel Bates 2014-10-28 17:18:24 PDT
(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.
Comment 15 Daniel Bates 2014-10-28 17:19:27 PDT
(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 16 WebKit Commit Bot 2014-10-28 17:58:10 PDT
Comment on attachment 240369 [details]
Uses Executive.ignore_error

Clearing flags on attachment: 240369

Committed r175294: <http://trac.webkit.org/changeset/175294>
Comment 17 WebKit Commit Bot 2014-10-28 17:58:14 PDT
All reviewed patches have been landed.  Closing bug.