Bug 70435

Summary: NRWT ServerProcess can't read lines from stderr and stdio separately
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, dpranke, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 70492    
Bug Blocks: 56729, 63683, 63981    
Attachments:
Description Flags
Patch
none
Patch for landing eric: commit-queue+

Description Eric Seidel (no email) 2011-10-19 13:17:35 PDT
NRWT ServerProcess can't read lines from stderr and stdio separately
Comment 1 Eric Seidel (no email) 2011-10-19 13:24:40 PDT
Created attachment 111664 [details]
Patch
Comment 2 Adam Barth 2011-10-19 13:52:40 PDT
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.
Comment 3 Eric Seidel (no email) 2011-10-19 14:06:33 PDT
(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.
Comment 4 Eric Seidel (no email) 2011-10-19 14:18:11 PDT
Created attachment 111672 [details]
Patch for landing
Comment 5 Eric Seidel (no email) 2011-10-19 14:37:44 PDT
Committed r97879: <http://trac.webkit.org/changeset/97879>
Comment 6 Nikolas Zimmermann 2011-10-20 02:13:19 PDT
(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?
Comment 7 Eric Seidel (no email) 2011-10-20 02:23:56 PDT
Looking.
Comment 8 Eric Seidel (no email) 2011-10-20 04:47:12 PDT
Regression fix posted in bug 70492.