Bug 70492

Summary: REGRESSION(97879): Pixel tests no longer work with NRWT on Mac
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, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70435    
Attachments:
Description Flags
Patch
none
now with more test! abarth: review+, abarth: commit-queue+

Eric Seidel (no email)
Reported 2011-10-20 04:37:11 PDT
REGRESSION(97879): Pixel tests no longer work with NRWT on Mac
Attachments
Patch (18.01 KB, patch)
2011-10-20 04:40 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
now with more test! (20.12 KB, patch)
2011-10-20 16:09 PDT, Eric Seidel (no email) 		abarth: review+
abarth: commit-queue+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2011-10-20 04:40:55 PDT
Created attachment 111755 [details] Patch
Eric Seidel (no email)
Comment 2 2011-10-20 04:43:52 PDT
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 3 2011-10-20 04:44:30 PDT
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 4 2011-10-20 05:25:24 PDT
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)
Comment 5 2011-10-20 15:03:47 PDT
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 6 2011-10-20 15:28:35 PDT
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)
Comment 7 2011-10-20 16:09:02 PDT
Created attachment 111865 [details] now with more test!
Eric Seidel (no email)
Comment 8 2011-10-20 16:16:44 PDT
Committed r98034: <http://trac.webkit.org/changeset/98034>
Note You need to log in before you can comment on or make changes to this bug.