[webkit-reviews] review denied: [Bug 63494] REGRESSION(r84294): new-run-webkit-tests results.html generate links to diffs.html or diff.png that don't exist : [Attachment 99239] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 63494: REGRESSION(r84294): new-run-webkit-tests results.html generate links
to diffs.html or diff.png that don't exist
https://bugs.webkit.org/show_bug.cgi?id=63494

Attachment 99239: Patch
https://bugs.webkit.org/attachment.cgi?id=99239&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=99239&action=review

r- for nits.

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:268
> +	   res, buildbot_output, regular_output, user =
logging_run(['--print-last-failures'], filesystem=fs)
> +	   self.assertEqual(regular_output.get(),
[u'failures/expected/checksum.html\n\n'])

I'm confused what would prompt this test case to complain about checksum.html? 
This is slightly concerning.  Would be nice to hear dirks thoughts on this.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:275
> -	   if not expected_driver_outputs.image:
> +	   if not expected_driver_output.image:
>	       failures.append(test_failures.FailureMissingImage())

Why if the expected output is missing an image do we add FailureMissingImage
here?

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:281
> +	   elif driver_output.image_hash != expected_driver_output.image_hash:
> +	       driver_output.image_diff =
self._port.diff_image(driver_output.image, expected_driver_output.image)
> +	       if driver_output.image_diff:
> +		   failures.append(test_failures.FailureImageHashMismatch())

Its a little subtle that this image_diff creation is here.  We may need to
rename this method or add furhter commetns.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:72
> +	       # FIXME: This work should be done earlier in the pipeline.

Can you be more specific as to when?  You're saying that the image diffing
should be done before writing out files?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:265
> +	   # FIXME: This seems like a text file, not a binary file.

All .diff/.patch files are binary, but if this is the diffs.html file (which
cycles through the images) then yes, it seems like text rather than binary.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:937
> +	   self.image_diff = None  # image_diff gets filled in after
construction.

It's odd that we even need to store this on driver_output.  It doesn't come
from the driver.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:179
> -		   raise e
> +		   raise

Yay!  I wonder how many other places we get this wrong.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:182
> +	       if exit_code == 1:
> +		   result = self._filesystem.read_binary_file(diff_filename)

Although this seems awkward for chromium, this makes much more sense for WebKit
who sends the binary over the wire instead of through the filesystem.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:143
> +	       # FIXME: It's not clear what we should return in this case.
> +	       # Maybe we should throw an exception?
>	       return True

I would raise ValueError (or any other error which seems reasonable for bad
arguments).

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:145
> +	   process = self._create_image_diff_process(expected_contents,
actual_contents)

Maybe thsi should say "start"?	"create" blends wiht image_diff
"create_image_diff" as though you're asking for the process which creates the
image diffs.

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:160
> +				(len(actual_contents), actual_contents,
len(expected_contents), expected_contents))

Did you mean to indent this again?

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:189
> +	   if output.startswith('diff'):
> +	       m = re.match('diff: (.+)% (passed|failed)', output)
> +	       if m.group(2) == 'passed':
> +		   return None
> +	   return output

I wonder if ORWT saves the diffs even when they "pass"?