Bug 70435

Created attachment 111664 [details] Patch

Comment on attachment 111664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111664&action=review > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:216 > + def _pop_error_bytes(self, bytes): bytes -> number_of_bytes ? > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:219 > + # FIXME: Python must have a simpler way to do this partition? > + output = self.error[0:bytes] > + self.error = self.error[bytes:] It seems like there should be a more pythonic way of doing this. > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:248 > + def _read(self, timeout, termination_checker): > deadline = time.time() + timeout We should be consistent about using dealines. Switching back and forth is expensive and error prone (e.g., getTimeOfDay changes during execution and your deadline drifts). > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:253 > + bytes_to_return = termination_checker() retrive_bytes ? Dunno. termination_checker sounds like it should just return a bool. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:505 > + value = line.split()[1] This seems somewhat fragile. > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:519 > + block.content += line This is slow, but a problem for another day.

(In reply to comment #2) > (From update of attachment 111664 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111664&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:216 > > + def _pop_error_bytes(self, bytes): > > bytes -> number_of_bytes ? Fixed. > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:219 > > + # FIXME: Python must have a simpler way to do this partition? > > + output = self.error[0:bytes] > > + self.error = self.error[bytes:] > > It seems like there should be a more pythonic way of doing this. I'm not sure there is, because there is some question as of it you split before or after the index. :) But I added a helper function. > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:248 > > + def _read(self, timeout, termination_checker): > > deadline = time.time() + timeout > > We should be consistent about using dealines. Switching back and forth is expensive and error prone (e.g., getTimeOfDay changes during execution and your deadline drifts). I completely agree! Out of scope for this patch though. > > Tools/Scripts/webkitpy/layout_tests/port/server_process.py:253 > > + bytes_to_return = termination_checker() > > retrive_bytes ? Dunno. termination_checker sounds like it should just return a bool. Renamed it fetch_bytes_from_buffers_callback > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:505 > > + value = line.split()[1] > > This seems somewhat fragile. Agreed. It's just the old code, moved. Probably best for another day. > > Tools/Scripts/webkitpy/layout_tests/port/webkit.py:519 > > + block.content += line > > This is slow, but a problem for another day. Yup.

Created attachment 111672 [details] Patch for landing

Committed r97879: <http://trac.webkit.org/changeset/97879>

(In reply to comment #5) > Committed r97879: <http://trac.webkit.org/changeset/97879> Some of the last NRWT commits broke pixel testing: Testing (4%): 88 ran as expecteImageDiff timed out ImageDiff timed out worker/0 raised AttributeError(''NoneType' object has no attribute 'startswith''): layout_tests/controllers/worker.py:91 (in run) self._worker_connection.run_message_loop() layout_tests/controllers/message_broker.py:191 (in run_message_loop) self._broker.run_message_loop(self._run_topic, self._client, delay_secs) layout_tests/controllers/message_broker.py:127 (in run_message_loop) self._run_loop(topic_name, client, block=True, delay_secs=delay_secs) layout_tests/controllers/message_broker.py:141 (in _run_loop) self._dispatch_message(msg, client) layout_tests/controllers/message_broker.py:150 (in _dispatch_message) message_handler(message.src, *optargs) layout_tests/controllers/worker.py:111 (in handle_test_list) self._run_test(test_input) layout_tests/controllers/worker.py:126 (in _run_test) result = self.run_test_with_timeout(test_input, test_timeout_sec) layout_tests/controllers/worker.py:165 (in run_test_with_timeout) return self._run_test_in_this_thread(test_input) layout_tests/controllers/worker.py:250 (in _run_test_in_this_thread) return self.run_single_test(self._driver, test_input) layout_tests/controllers/worker.py:254 (in run_single_test) test_input, driver, self._name) layout_tests/controllers/single_test_runner.py:45 (in run_single_test) return runner.run() layout_tests/controllers/single_test_runner.py:124 (in run) return self._run_compare_test() layout_tests/controllers/single_test_runner.py:129 (in _run_compare_test) test_result = self._compare_output(driver_output, expected_driver_output) layout_tests/controllers/single_test_runner.py:236 (in _compare_output) failures.extend(self._compare_image(driver_output, expected_driver_output)) layout_tests/controllers/single_test_runner.py:279 (in _compare_image) diff_result = self._port.diff_image(driver_output.image, expected_driver_output.image) layout_tests/port/webkit.py:161 (in diff_image) return self._read_image_diff(process) layout_tests/port/webkit.py:204 (in _read_image_diff) if output.startswith('diff'): Can you confirm?

Looking.

Regression fix posted in bug 70492.