Bug 36504 – new-run-webkit-tests leaves -crash.txt files in the layout tests directory instead of the build output directory

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

[James Robinson]

Add a flag to run_webkit_tests to control saving crashing stacks

[James Robinson]

Created attachment 51449 [details]
Patch

[Dirk Pranke]

Comment on attachment 51449 [details]
Patch


> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> index de38dff..418b0c3 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_shell_thread.py
> @@ -53,7 +53,7 @@ _log = logging.getLogger("webkitpy.layout_tests.layout_package."
>  
>  def process_output(port, test_info, test_types, test_args, target, output_dir,
>                     crash, timeout, test_run_time, actual_checksum,
> -                   output, error):
> +                   output, error, options):
>      """Receives the output from a test_shell process, subjects it to a number
>      of tests, and returns a list of failure types the test produced.
>

You should add a comment about the |options| variable into the docstring.
  
> @@ -82,13 +82,14 @@ def process_output(port, test_info, test_types, test_args, target, output_dir,
>          failures.append(test_failures.FailureTimeout())
>  
>      if crash:
> -        _log.debug("Stacktrace for %s:\n%s" % (test_info.filename, error))
> -        # Strip off "file://" since RelativeTestFilename expects
> -        # filesystem paths.
> -        filename = os.path.join(output_dir, test_info.filename)
> -        filename = os.path.splitext(filename)[0] + "-stack.txt"
> -        port.maybe_make_directory(os.path.split(filename)[0])
> -        open(filename, "wb").write(error)
> +         _log.debug("Stacktrace for %s:\n%s" % (test_info.filename, error))
> +         # Strip off "file://" since RelativeTestFilename expects
> +         # filesystem paths.
> +         if not options.nosave_crash_stack:
> +             filename = os.path.join(output_dir, test_info.filename)
> +             filename = os.path.splitext(filename)[0] + "-stack.txt"
> +             port.maybe_make_directory(os.path.split(filename)[0])
> +             open(filename, "wb").write(error)


move the "# Strip off "file://" comment inside the if block

>      elif error:
>          _log.debug("Previous test output extra lines after dump:\n%s" %
>                     error)
> @@ -129,7 +130,7 @@ class SingleTestThread(threading.Thread):
>      """Thread wrapper for running a single test file."""
>  
>      def __init__(self, port, image_path, shell_args, test_info,
> -        test_types, test_args, target, output_dir):
> +        test_types, test_args, options):
>          """
>          Args:
>            port: object implementing port-specific hooks
> @@ -146,7 +147,9 @@ class SingleTestThread(threading.Thread):

update the docstring

>  
>      def get_test_stats(self):
> @@ -364,8 +367,7 @@ class TestShellThread(threading.Thread):
>                                    test_info,
>                                    self._test_types,
>                                    self._test_args,
> -                                  self._options.target,
> -                                  self._options.results_directory)
> +                                  self._options)
>  

update the docstring

>          worker.start()
>  
> @@ -422,7 +424,7 @@ class TestShellThread(threading.Thread):
>                                 self._test_args, self._options.target,
>                                 self._options.results_directory, crash,
>                                 timeout, end - start, actual_checksum,
> -                               output, error)
> +                               output, error, self._options)

update the docstring

otherwise LGTM.

[James Robinson]

Created attachment 51453 [details]
Patch

[Dirk Pranke]

Comment on attachment 51453 [details]
Patch

> +    option_parser.add_option("", "--nosave-crash-stack", action="store_true",
> +                             default=False,
> +                             help="Don't save crashing stack traces to disk")

Okay, from poking around in the optparse documentation, it looks like perhaps the "clean" way to handle boolean flags would be
to have:

option_parser.add_option("", "--save-crash-stack", action="store_true", default="True",
                         help="save crashing stack traces to disk")
option_parser.add_option("", "--nosave-crash-stack", value="save_crash_stack",
                         action="store_false", help="don't save crashing stack traces to disk")

and then test against if options.save_crash_stack (to avoid the double negative). Note the use of the 'value="xxx" named argument in the second line to tell the parser that the two flags affect the same field.

-- Dirk

[Dirk Pranke]

(In reply to comment #4)
> option_parser.add_option("", "--nosave-crash-stack", value="save_crash_stack",
>                          action="store_false", help="don't save crashing stack

er, that should be 'dest="save_crash_stack"', not 'value="save_crash_stack"'.

[Ojan Vafai]

Why do we need this?

[James Robinson]

The motivation is that I'll frequently test patches that cause large numbers of layout tests to fail, leaving tons of -crash.txt files in my working directory that I then have to go clean up later.

I think the fundamental issue is that our scripts stick things that are logically output (like -crash.txt files) into the working directory instead of some other location.  Adding a flag is an easier workaround than changing where the scripts look for results, however.

[Eric Seidel (no email)]

I also don't really see why we'd want this.  crash files should go in with the layout test results in teh build directory anyway. :)

[Ojan Vafai]

(In reply to comment #8)
> I also don't really see why we'd want this.  crash files should go in with the
> layout test results in teh build directory anyway. :)

The crash files do currently go in the build directory. This is making it so that they don't get written anywhere if you pass --nosave-crash-stack.

James, I'm not sure I follow. The crash files go in the build output directory, no? Isn't it sufficient to svn/git ignore the build output directory?

[James Robinson]

Whenever I run the tests and some crash, the -stack.txt file ends up in the same directory as the test.  I.e. if I make fast/clip/003.html crash, I end up with LayoutTests/fast/clip/003-stack.txt.  I'm not sure if that is what you mean by "build output directory" but it's not what I think of as an output directory.

Should the -stack.txt files be going somewhere else when I run the tests?  If so that would take care of my problem as well.

[Ojan Vafai]

(In reply to comment #10)
> Whenever I run the tests and some crash, the -stack.txt file ends up in the
> same directory as the test.  I.e. if I make fast/clip/003.html crash, I end up
> with LayoutTests/fast/clip/003-stack.txt.  I'm not sure if that is what you
> mean by "build output directory" but it's not what I think of as an output
> directory.
> 
> Should the -stack.txt files be going somewhere else when I run the tests?  If
> so that would take care of my problem as well.

Yes. That's a bug. It should go to the same place the other layout test results go to, which is a folder in the build output directory. I'm pretty sure this used to work, so it must be a regression in the past couple weeks/months.

[Dirk Pranke]

stealing this from james ...

[Dirk Pranke]

Created attachment 52694 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 52694 [details]
Patch

OK.

[Dirk Pranke]

Comment on attachment 52694 [details]
Patch

Clearing flags on attachment: 52694

Committed r57187: <http://trac.webkit.org/changeset/57187>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.