[Webkit-unassigned] [Bug 60957] Pass flags from run_webkit_tests to ImageDiff, read results from stdout and store in unexpected_results.json

Thu May 26 16:21:15 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=60957

Dirk Pranke <dpranke at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #93762|review?                     |review-
               Flag|                            |

--- Comment #4 from Dirk Pranke <dpranke at chromium.org>  2011-05-26 16:21:15 PST ---
(From update of attachment 93762)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.