Bug 70492 – REGRESSION(97879): Pixel tests no longer work with NRWT on Mac

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

[Eric Seidel (no email)]

REGRESSION(97879): Pixel tests no longer work with NRWT on Mac

[Eric Seidel (no email)]

Created attachment 111755 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 111755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111755&action=review

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:600
> +                assert out_line[-1] == "\n", "Missing newline"
> +                content_length_before_header_check = block._content_length
>                  self._process_stdout_line(block, out_line)
>                  # FIXME: Unlike HTTP, DRT dumps the content right after printing a Content-Length header.
>                  # Don't wait until we're done with headers, just read the binary blob right now.
> -                if block._content_length is not None and not block.content:
> -                    block.content = self._server_process.read_stdout(deadline - time.time(), block._content_length)
> +                if content_length_before_header_check != block._content_length:
> +                    block.content = self._server_process.read_stdout(deadline, block._content_length)

This is the actual bugfix.  Note the assert which will prevent us from getting this type of bug in the future.

We were ending up reading the image content as normal text lines instead of a binary blob.  Thus when we sent the lines to ImageDiff it got confused.

[Eric Seidel (no email)]

Comment on attachment 111755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111755&action=review

> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:121
> -    def write(self, input):
> +    def write(self, bytes):

input is a global function in python, so we prefer not to shadow it with variable names.

[Adam Roben (:aroben)]

Comment on attachment 111755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111755&action=review

Doesn't seem like you have a test for the fix?

> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:228
> +        ms_to_wait = deadline - time.time()

time.time() returns a number in seconds, not milliseconds.

[Eric Seidel (no email)]

I can undo the deadline part of the fix.  The fix for pixel tests is covered by that assert.  I agree this could all use more testing. :)  I shouldn't have tried to fix this at 5am.

[Eric Seidel (no email)]

Comment on attachment 111755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111755&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:228
>> +        ms_to_wait = deadline - time.time()
> 
> time.time() returns a number in seconds, not milliseconds.

Yes, my (historical) use of ms_ in the variable name was incorrect.  Will fix.

[Eric Seidel (no email)]

Created attachment 111865 [details]
now with more test!

[Eric Seidel (no email)]

Committed r98034: <http://trac.webkit.org/changeset/98034>