Bug 73381 - base port needs to implement diff_image
Summary: base port needs to implement diff_image
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-29 17:37 PST by Ryosuke Niwa
Modified: 2011-11-30 00:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2011-11-29 18:45 PST, Hayato Ito
hayato: commit-queue?
Details | Formatted Diff | Diff
fix name (1.97 KB, patch)
2011-11-29 20:31 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
fix changelog (1.98 KB, patch)
2011-11-29 20:35 PST, Hayato Ito
rniwa: review+
hayato: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-11-29 17:37:11 PST
Right now, only chromium port implements diff_image. This would mean that python throws an exception like the one below whenever ref-test fails:

Line:61 Path does not exist. platform/mac/fonts/color-bitmap.html
worker/0 raised TypeError('argument 1 must be string or buffer, not None'):
  layout_tests/controllers/worker.py:91 (in run)
    self._worker_connection.run_message_loop()
  layout_tests/controllers/message_broker.py:191 (in run_message_loop)
    self._broker.run_message_loop(self._run_topic, self._client, delay_secs)
  layout_tests/controllers/message_broker.py:127 (in run_message_loop)
    self._run_loop(topic_name, client, block=True, delay_secs=delay_secs)
  layout_tests/controllers/message_broker.py:141 (in _run_loop)
    self._dispatch_message(msg, client)
  layout_tests/controllers/message_broker.py:150 (in _dispatch_message)
    message_handler(message.src, *optargs)
  layout_tests/controllers/worker.py:111 (in handle_test_list)
    self._run_test(test_input)
  layout_tests/controllers/worker.py:126 (in _run_test)
    result = self.run_test_with_timeout(test_input, test_timeout_sec)
  layout_tests/controllers/worker.py:165 (in run_test_with_timeout)
    return self._run_test_in_this_thread(test_input)
  layout_tests/controllers/worker.py:250 (in _run_test_in_this_thread)
    return self.run_single_test(self._driver, test_input)
  layout_tests/controllers/worker.py:254 (in run_single_test)
    test_input, driver, self._name)
  layout_tests/controllers/single_test_runner.py:46 (in run_single_test)
    return runner.run()
  layout_tests/controllers/single_test_runner.py:124 (in run)
    return self._run_reftest()
  layout_tests/controllers/single_test_runner.py:292 (in _run_reftest)
    test_result_writer.write_test_result(self._port, self._test_name, driver_output1, driver_output2, test_result.failures)
  layout_tests/controllers/test_result_writer.py:74 (in write_test_result)
    writer.write_image_diff_files(image_diff)
  layout_tests/controllers/test_result_writer.py:207 (in write_image_diff_files)
    fs.write_binary_file(diff_filename, image_diff)
  common/system/filesystem.py:205 (in write_binary_file)
    f.write(contents)
Comment 1 Hayato Ito 2011-11-29 18:12:33 PST
A quick fix might be simply skipping writing the result of image_diff on mac port. Skipping writing result doesn't affect the result of tests.

Later we should implement image_diff on mac port. Any thought?
Comment 2 Hayato Ito 2011-11-29 18:45:25 PST
Created attachment 117096 [details]
Patch
Comment 3 Ryosuke Niwa 2011-11-29 19:14:53 PST
As far as I know, chromium is the only port that implements diff_image.
Comment 4 Ryosuke Niwa 2011-11-29 19:17:11 PST
Comment on attachment 117096 [details]
Patch

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

Is it really hard to implement diff_image for other ports? I'd rather not leave it broken for other ports.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74
> +                # Mac port returns 'None' since it doesn't implement diff_image.

This should say non-chromium port.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:75
> +                if image_diff is not None:

isn't != less verbose?
Comment 5 Hayato Ito 2011-11-29 20:31:26 PST
Created attachment 117106 [details]
fix name
Comment 6 Ryosuke Niwa 2011-11-29 20:32:32 PST
Comment on attachment 117106 [details]
fix name

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

> Tools/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.
Comment 7 Hayato Ito 2011-11-29 20:33:08 PST
Comment on attachment 117096 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:74
>> +                # Mac port returns 'None' since it doesn't implement diff_image.
> 
> This should say non-chromium port.

Done.

> Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:76
> +                    writer.write_image_diff_files(image_diff)

"is not" is better in this case because None is singleton. I guess that's slightly faster and well accepted usage.
Comment 8 Hayato Ito 2011-11-29 20:33:46 PST
Ops. I'll fix it and update the patch.

(In reply to comment #6)
> (From update of attachment 117106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117106&action=review
> 
> > Tools/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.
Comment 9 Hayato Ito 2011-11-29 20:35:58 PST
Created attachment 117109 [details]
fix changelog
Comment 10 Hayato Ito 2011-11-29 23:03:47 PST
I noticed that mac port actually implements diff_image.

WebKitPort is a super class of Apple port, which is super class of MacPort.
WebKitPort implements diff_image.

The problem is WebKitPorts's diff_image seems to be wrongly implemented.
Let me investigate further and file another bug.
Comment 11 Hayato Ito 2011-11-30 00:17:46 PST
Committed r101455: <http://trac.webkit.org/changeset/101455>
Comment 12 Hayato Ito 2011-11-30 00:28:24 PST
I filed another bug in:
https://bugs.webkit.org/show_bug.cgi?id=73406