Bug 53838 – nrwt multiprocessing: move code from dump_render_tree_thread to single_test_runner

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

nrwt multiprocessing: move code from dump_render_tree_thread to single_test_runner

[Dirk Pranke]

Created attachment 81340 [details]
split out most of the code from drt into str

[Tony Chang]

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?

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 81589 [details]
update w/ tony's review feedback

[Tony Chang]

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.

[Dirk Pranke]

Committed r77992: <http://trac.webkit.org/changeset/77992>