Bug 53838 - nrwt multiprocessing: move code from dump_render_tree_thread to single_test_runner
Summary: nrwt multiprocessing: move code from dump_render_tree_thread to single_test_r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 53839
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-04 20:04 PST by Dirk Pranke
Modified: 2011-02-08 16:40 PST (History)
5 users (show)

See Also:


Attachments
split out most of the code from drt into str (21.43 KB, patch)
2011-02-04 20:06 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ tony's review feedback (22.87 KB, patch)
2011-02-07 22:54 PST, Dirk Pranke
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-04 20:04:24 PST
nrwt multiprocessing: move code from dump_render_tree_thread to single_test_runner
Comment 1 Dirk Pranke 2011-02-04 20:06:05 PST
Created attachment 81340 [details]
split out most of the code from drt into str
Comment 2 Tony Chang 2011-02-07 17:22:18 PST
Comment on attachment 81340 [details]
split out most of the code from drt into str

View in context: https://bugs.webkit.org/attachment.cgi?id=81340&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:31
> +"""This module implements a shared-memory, thread-based version of the worker."""

Can you be more specific than "the worker"?

> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:202
> -        tests_run_file = self._port._filesystem.open_text_file_for_writing(tests_run_filename, append=False)
> +                                                         "tests_run.txt")
> +        tests_run_file = self._port._filesystem.open_text_file_for_writing(tests_run_filename, append=True)

Why did this change to append?
Comment 3 Dirk Pranke 2011-02-07 18:26:45 PST
Comment on attachment 81340 [details]
split out most of the code from drt into str

View in context: https://bugs.webkit.org/attachment.cgi?id=81340&action=review

>> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:31
>> +"""This module implements a shared-memory, thread-based version of the worker."""
> 
> Can you be more specific than "the worker"?

Sure. I don't have a great descriptive name for "thing that actually talks to DRT", but I'll put in something, even if it's "thing that actually talks to DRT".

>> Tools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:202
>> +        tests_run_file = self._port._filesystem.open_text_file_for_writing(tests_run_filename, append=True)
> 
> Why did this change to append?

Good catch. Previously, each thread was opening the same file and hence stepping on each other. Now they should at least be writing to the same file. However, the whole idea of multiple threads writing to the same file unsettles me, especially with the switch to separate processes coming up, so I'll change this to append the worker number to the filename.

I'm not sure that anyone actually looks at these files normally, so it may be best to just remove it, but I'll post that as a separate change. Generally speaking, I think NRWT logs more stuff in more places than it should and I'd like to remove some things and rationalize others. But that's a thought for a different thread.
Comment 4 Dirk Pranke 2011-02-07 22:54:31 PST
Created attachment 81589 [details]
update w/ tony's review feedback
Comment 5 Tony Chang 2011-02-08 09:42:32 PST
Comment on attachment 81589 [details]
update w/ tony's review feedback

(In reply to comment #3)
> I'm not sure that anyone actually looks at these files normally, so it may be best to just remove it, but I'll post that as a separate change. Generally speaking, I think NRWT logs more stuff in more places than it should and I'd like to remove some things and rationalize others. But that's a thought for a different thread.

Is the file used for anything?  If not, I agree that we should remove it in a follow up patch.  Seems like it could be reconstructed from the output as the script is run.
Comment 6 Dirk Pranke 2011-02-08 16:40:57 PST
Committed r77992: <http://trac.webkit.org/changeset/77992>