Bug 137667 – Make runtest.py call test-webkitpy with the --json flag when in non-interactive mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jake Nielsen]

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.

[Jake Nielsen]

Created attachment 239764 [details]
Makes runtest.py call test-webkitpy with the --json flag when in non-interactive mode

[Jake Nielsen]

bump

[Daniel Bates]

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.

[Jake Nielsen]

(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.

[Jake Nielsen]

Created attachment 239899 [details]
Reorders lines in runtests.py as per comment

[Jake Nielsen]

Comment on attachment 239899 [details]
Reorders lines in runtests.py as per comment

Sigh, I need to stop doing this. Wrong patch.

[Jake Nielsen]

Created attachment 239902 [details]
Intended patch

[Daniel Bates]

(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?

[Daniel Bates]

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.

[Jake Nielsen]

Created attachment 240357 [details]
Addresses comments

[Daniel Bates]

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.

[Jake Nielsen]

Created attachment 240369 [details]
Uses Executive.ignore_error

[Daniel Bates]

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.

[Daniel Bates]

(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.

[Daniel Bates]

(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.

[WebKit Commit Bot]

Comment on attachment 240369 [details]
Uses Executive.ignore_error

Clearing flags on attachment: 240369

Committed r175294: <http://trac.webkit.org/changeset/175294>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.