RESOLVED FIXED 36504
new-run-webkit-tests leaves -crash.txt files in the layout tests directory instead of the build output directory
https://bugs.webkit.org/show_bug.cgi?id=36504
Summary new-run-webkit-tests leaves -crash.txt files in the layout tests directory in...
James Robinson
Reported 2010-03-23 13:31:43 PDT
Add a flag to run_webkit_tests to control saving crashing stacks
Attachments
Patch (5.33 KB, patch)
2010-03-23 13:32 PDT, James Robinson
no flags
Patch (6.57 KB, patch)
2010-03-23 14:22 PDT, James Robinson
no flags
Patch (1.64 KB, patch)
2010-04-06 19:12 PDT, Dirk Pranke
no flags
James Robinson
Comment 1 2010-03-23 13:32:54 PDT
Dirk Pranke
Comment 2 2010-03-23 13:59:42 PDT
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
Comment 3 2010-03-23 14:22:05 PDT
Dirk Pranke
Comment 4 2010-03-25 16:28:15 PDT
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
Comment 5 2010-03-25 16:31:49 PDT
(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
Comment 6 2010-03-25 18:15:19 PDT
Why do we need this?
James Robinson
Comment 7 2010-03-25 18:32:57 PDT
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)
Comment 8 2010-03-26 14:24:50 PDT
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
Comment 9 2010-03-26 14:44:59 PDT
(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
Comment 10 2010-03-31 12:36:33 PDT
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
Comment 11 2010-03-31 14:43:03 PDT
(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
Comment 12 2010-04-06 18:33:40 PDT
stealing this from james ...
Dirk Pranke
Comment 13 2010-04-06 19:12:37 PDT
Eric Seidel (no email)
Comment 14 2010-04-06 19:14:39 PDT
Comment on attachment 52694 [details] Patch OK.
Dirk Pranke
Comment 15 2010-04-06 19:33:56 PDT
Comment on attachment 52694 [details] Patch Clearing flags on attachment: 52694 Committed r57187: <http://trac.webkit.org/changeset/57187>
Dirk Pranke
Comment 16 2010-04-06 19:34:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.