[webkit-reviews] review denied: [Bug 63844] tests_run0.txt gets clobbered when re-running failing tests : [Attachment 115804] proposed fix with unit test

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 21 12:57:56 PST 2011


Dirk Pranke <dpranke at chromium.org> has denied Kristóf Kosztyó
<kkristof at inf.u-szeged.hu>'s request for review:
Bug 63844: tests_run0.txt gets clobbered when re-running failing tests
https://bugs.webkit.org/show_bug.cgi?id=63844

Attachment 115804: proposed fix with unit test
https://bugs.webkit.org/attachment.cgi?id=115804&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115804&action=review


The manager_worker_broker class shouldn't have to know anything about retrying
logic, and modifying a module-level global is bad. I think we can just pass the
output directory to use through when we start each worker, and that might end
up a lot cleaner.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:913
> +	       manager_worker_broker.retry = True

I think instead of doing this we can just modify start_worker() to take the
output directory to use along with the worker number, and then change the
output directory based on whether self._retrying is True.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py:266
> +    def test_retrying(self):

In addition to this test, it might be good to make sure that the 'retrying'
flag propagates correctly across the process boundary in the multiprocess case
(this test will only test the inline case).

Unfortunately, you can't test that with the --platform=test class, since that
uses a mock filesystem (which isn't shared across process), which means we'd
need to test this with a real integration test. Maybe just add a a FIXME for
now?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:68
> +retry = False

This isn't necessary.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:109
> +def results_directory(port):

This function doesn't belong here; the broker class shouldn't have to know
anything about the results_directory.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:44
> +    root_output_dir = manager_worker_broker.results_directory(port)

I suggest we pass in the output directory to use instead.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:71
> +	   tests_run_filename =
self._filesystem.join(manager_worker_broker.results_directory(self._port),
"tests_run%d.txt" % self._worker_number)

We should pass in the output directory like we do the worker number.


More information about the webkit-reviews mailing list