[webkit-reviews] review denied: [Bug 60957] Pass flags from run_webkit_tests to ImageDiff, read results from stdout and store in unexpected_results.json : [Attachment 93762] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Tom Hudson
<tomhudson at google.com>'s request for review:
Bug 60957: Pass flags from run_webkit_tests to ImageDiff, read results from
stdout and store in unexpected_results.json
https://bugs.webkit.org/show_bug.cgi?id=60957

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=93762&action=review

So this patch isn't what I'd like to see as the long-term way to do this, but
it's close for an incremental step. Ideally we should do something like modify
layout_package/single_test_runner._compare_image and
test_result_writer.diff_image so that we can compute your metrics in a single
step, and then pass a dict of the metrics through a TestFailure object.
However, changing the code to do this is a bit invasive to do all at once and
may be more than you're comfortable with (as we've discussed previously). I'll
upload a patch that demonstrates the protocol-level changes I have in mind and
we can eventually merge that patch with this line of development.

However, this batch is basically fine and doesn't need to be completely redone
to make those changes. I'm R-'ing it for a bunch of specific things we can
tweak somewhat to simplify things, in particular so that we don't need to
generate two "finished_test" messages per test.

Let me know if you have any questions on my comments. I look forward to getting
this feature working and will be glad to help now that I'm not either on
vacation or in workshops :)

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:263
> +	       default="",

I will usually try to omit the default (which is equivalent to default=None) if
I want something to be empty by default.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:161
> +	   port_obj.add_port_test_data(result_type, test, test_dict)

Can you add a FIXME here to extract this data from the failure_types list
directly instead of having to jump through the handle_port_finished_test /
add_port_test_data hoop instead?

If it's not clear what I have in mind, I can upload a modified version of this
patch that does what I'm talking about.

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:143
>	   self._worker_connection.post_message('finished_test', result,
elapsed_time)

Can we change this so that instead of sending a port_finished_test message we
call into a port routine that returns a dict and send the dict along in
finished_test? I don't really want to double the number of messages sent in a
test run, and I have no concerns over changing the message format.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:686
> +    def add_port_test_data(self, result_type, test, out_test_dict):

If the intent is for these to be default implementations that don't do
anything, can we please add a comment to that effect and a 'pass' statement? On
the other hand, if you want these to be virtual methods that must be
implemented in subclasses, please add a raise NotImplementedError (as we do in
other routines).

Also, please add tests for these routines in port/port_testcase.py.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:162
> +	   return diff_filename[prefix_len:-suffix_len] + '.html'

Ugh. There should be a method on the base class to do something like this, and
we shouldn't introduce a dependency on test_result_writer into this file. Can
you add a FIXME to fix this in a later patch?

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:196
> +	       if self.get_option('write_image_diff_metrics'):

Is there a reason we shouldn't just always pass the flag and write the metrics?


> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:346
> +    def _test_name_from_filename(self, diff_filename):

There is a port.relative_test_filename() in base.py that does this already.