Bug 37487 – Make new-run-webkit-test PrettyPatch failure reporting more awesome

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make new-run-webkit-test PrettyPatch failure reporting more awesome

[Eric Seidel (no email)]

Created attachment 53222 [details]
Patch

[Adam Barth]

Comment on attachment 53222 [details]
Patch

ok.  missing a space

[Eric Seidel (no email)]

Committed r57500: <http://trac.webkit.org/changeset/57500>

[Dirk Pranke]

Comment on attachment 53222 [details]
Patch

>          command = ["ruby", "-I", pretty_patch_path, prettify_path, diff_path]
>          try:
>              return self._executive.run_command(command)
>          except OSError, e:
> -            # If they system is missing ruby just log the error, and stop trying.
> +            # If the system is missing ruby log the error and stop trying.
>              _pretty_patch_available = False
> -            return "Failed to run PrettyPatch: %s" % e
> -        except Executive.ScriptError, e:
> -            # If they system is missing ruby just log the error, and stop trying.
> +            _log.error("Failed to run PrettyPatch (%s): %s" % (command, e))
> +            return self._pretty_patch_error_html
> +        except ScriptError, e:
> +            # If ruby failed to run for some reason, log the command output and stop trying.
>              _pretty_patch_available = False
> -            return "Failed to run PrettyPatch: %s" % e
> +            _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
> +            return self._pretty_patch_error_html
>

Why does this routine have to catch either a ScriptError or an OSError? It seems like maybe the run_command() semantics aren't clear?

[Eric Seidel (no email)]

They mean different things.  OSError means you're missing the file you tried to run.  ScriptError means it returned non-zero.