Bug 70435 – NRWT ServerProcess can't read lines from stderr and stdio separately

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

NRWT ServerProcess can't read lines from stderr and stdio separately

[Eric Seidel (no email)]

Created attachment 111664 [details]
Patch

[Adam Barth]

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.

[Eric Seidel (no email)]

(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.

[Eric Seidel (no email)]

Created attachment 111672 [details]
Patch for landing

[Eric Seidel (no email)]

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

[Nikolas Zimmermann]

(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?

[Eric Seidel (no email)]

Looking.

[Eric Seidel (no email)]

Regression fix posted in bug 70492.