WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(6.57 KB, patch)
2010-03-23 14:22 PDT
,
James Robinson
no flags
Details
Formatted Diff
Diff
Patch
(1.64 KB, patch)
2010-04-06 19:12 PDT
,
Dirk Pranke
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
James Robinson
Comment 1
2010-03-23 13:32:54 PDT
Created
attachment 51449
[details]
Patch
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
Created
attachment 51453
[details]
Patch
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
Created
attachment 52694
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug