Summary: | new-run-webkit-tests leaves -crash.txt files in the layout tests directory instead of the build output directory | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | dglazkov, dpranke, eric, ojan | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
James Robinson
2010-03-23 13:31:43 PDT
Created attachment 51449 [details]
Patch
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. Created attachment 51453 [details]
Patch
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 (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"'. Why do we need this? 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. 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. :) (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? 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. (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. stealing this from james ... Created attachment 52694 [details]
Patch
Comment on attachment 52694 [details]
Patch
OK.
Comment on attachment 52694 [details] Patch Clearing flags on attachment: 52694 Committed r57187: <http://trac.webkit.org/changeset/57187> All reviewed patches have been landed. Closing bug. |