Changeset 123893 in webkit


Ignore:
Timestamp:
Jul 27, 2012 11:21:08 AM (12 years ago)
Author:
dpranke@chromium.org
Message:

nrwt: move image diffing code to a separate module
​https://bugs.webkit.org/show_bug.cgi?id=92447

Reviewed by Ryosuke Niwa.

This patch moves the code to talk to ImageDiff into its own
module, and adds more tests for it. In addition, the patch
modifies diff_image() so that we don't automatically stop
ImageDiff after a single invocation, and thus subsequent
diffs may be slightly faster. (Note that the chromium ports
don't use any of this code; that is not changed by this patch).

The main motivation for this change is to move more "generic"
code out of the port/* classes, and in particular to move more
code out of webkit.py so that we can eventually eliminate it by
merging it into base.py.

This patch also splits MockServerProcess out from driver_unittest.py
so that it can be re-used.

  • Scripts/webkitpy/layout_tests/port/base.py:

(Port.init):
(Port.diff_image):
(Port.clean_up_test_run):

  • Scripts/webkitpy/layout_tests/port/driver.py:

(Driver.init):
(Driver._start):

  • Scripts/webkitpy/layout_tests/port/driver_unittest.py:

(DriverTest.test_stop_cleans_up_properly):
(DriverTest.test_two_starts_cleans_up_properly):
(DriverTest.test_start_actually_starts):

  • Scripts/webkitpy/layout_tests/port/efl.py:

(EflPort.clean_up_test_run):

  • Scripts/webkitpy/layout_tests/port/gtk.py:

(GtkPort.clean_up_test_run):

  • Scripts/webkitpy/layout_tests/port/image_diff.py: Added.

(ImageDiffer):
(ImageDiffer.init):
(ImageDiffer.diff_image):
(ImageDiffer._start):
(ImageDiffer._read):
(ImageDiffer.stop):

  • Scripts/webkitpy/layout_tests/port/image_diff_unittest.py: Added.

(for):
(FakePort):
(FakePort.init):
(FakePort._path_to_image_diff):
(FakePort.setup_environ_for_server):
(TestImageDiffer):
(TestImageDiffer.test_diff_image):

  • Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:

(MockDRTPortTest.test_diff_image):

  • Scripts/webkitpy/layout_tests/port/port_testcase.py:

(PortTestCase.test_diff_imagemissing_both):
(PortTestCase.test_diff_image):
(PortTestCase.test_diff_image.make_proc):

  • Scripts/webkitpy/layout_tests/port/server_process.py:

(ServerProcess._start):

  • Scripts/webkitpy/layout_tests/port/server_process_mock.py: Added.

(MockServerProcess):
(MockServerProcess.init):
(MockServerProcess.write):
(MockServerProcess.has_crashed):
(MockServerProcess.read_stdout_line):
(MockServerProcess.read_stdout):
(MockServerProcess.pop_all_buffered_stderr):
(MockServerProcess.read_either_stdout_or_stderr_line):
(MockServerProcess.start):
(MockServerProcess.stop):
(MockServerProcess.kill):

  • Scripts/webkitpy/layout_tests/port/webkit.py:

(WebKitPort._build_driver_flags):
(WebKitPort._symbols_string):

Location:
trunk/Tools
Files:
3 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r123892 r123893  
     12012-07-27  Dirk Pranke  <dpranke@chromium.org>
     2
     3        nrwt: move image diffing code to a separate module
     4        https://bugs.webkit.org/show_bug.cgi?id=92447
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        This patch moves the code to talk to ImageDiff into its own
     9        module, and adds more tests for it. In addition, the patch
     10        modifies diff_image() so that we don't automatically stop
     11        ImageDiff after a single invocation, and thus subsequent
     12        diffs may be slightly faster. (Note that the chromium ports
     13        don't use any of this code; that is not changed by this patch).
     14
     15        The main motivation for this change is to move more "generic"
     16        code out of the port/* classes, and in particular to move more
     17        code out of webkit.py so that we can eventually eliminate it by
     18        merging it into base.py.
     19
     20        This patch also splits MockServerProcess out from driver_unittest.py
     21        so that it can be re-used.
     22
     23        * Scripts/webkitpy/layout_tests/port/base.py:
     24        (Port.__init__):
     25        (Port.diff_image):
     26        (Port.clean_up_test_run):
     27        * Scripts/webkitpy/layout_tests/port/driver.py:
     28        (Driver.__init__):
     29        (Driver._start):
     30        * Scripts/webkitpy/layout_tests/port/driver_unittest.py:
     31        (DriverTest.test_stop_cleans_up_properly):
     32        (DriverTest.test_two_starts_cleans_up_properly):
     33        (DriverTest.test_start_actually_starts):
     34        * Scripts/webkitpy/layout_tests/port/efl.py:
     35        (EflPort.clean_up_test_run):
     36        * Scripts/webkitpy/layout_tests/port/gtk.py:
     37        (GtkPort.clean_up_test_run):
     38        * Scripts/webkitpy/layout_tests/port/image_diff.py: Added.
     39        (ImageDiffer):
     40        (ImageDiffer.__init__):
     41        (ImageDiffer.diff_image):
     42        (ImageDiffer._start):
     43        (ImageDiffer._read):
     44        (ImageDiffer.stop):
     45        * Scripts/webkitpy/layout_tests/port/image_diff_unittest.py: Added.
     46        (for):
     47        (FakePort):
     48        (FakePort.__init__):
     49        (FakePort._path_to_image_diff):
     50        (FakePort.setup_environ_for_server):
     51        (TestImageDiffer):
     52        (TestImageDiffer.test_diff_image):
     53        * Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:
     54        (MockDRTPortTest.test_diff_image):
     55        * Scripts/webkitpy/layout_tests/port/port_testcase.py:
     56        (PortTestCase.test_diff_image__missing_both):
     57        (PortTestCase.test_diff_image):
     58        (PortTestCase.test_diff_image.make_proc):
     59        * Scripts/webkitpy/layout_tests/port/server_process.py:
     60        (ServerProcess._start):
     61        * Scripts/webkitpy/layout_tests/port/server_process_mock.py: Added.
     62        (MockServerProcess):
     63        (MockServerProcess.__init__):
     64        (MockServerProcess.write):
     65        (MockServerProcess.has_crashed):
     66        (MockServerProcess.read_stdout_line):
     67        (MockServerProcess.read_stdout):
     68        (MockServerProcess.pop_all_buffered_stderr):
     69        (MockServerProcess.read_either_stdout_or_stderr_line):
     70        (MockServerProcess.start):
     71        (MockServerProcess.stop):
     72        (MockServerProcess.kill):
     73        * Scripts/webkitpy/layout_tests/port/webkit.py:
     74        (WebKitPort._build_driver_flags):
     75        (WebKitPort._symbols_string):
     76
    1772012-07-27  Tom Hudson  <hudson@google.com>
    278

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py

    r123871 r123893  
    5757from webkitpy.layout_tests.port import driver
    5858from webkitpy.layout_tests.port import http_lock
     59from webkitpy.layout_tests.port import image_diff
     60from webkitpy.layout_tests.port import server_process
    5961from webkitpy.layout_tests.servers import apache_http_server
    6062from webkitpy.layout_tests.servers import http_server
    … …  
    111113        self._http_server = None
    112114        self._websocket_server = None
     115        self._image_differ = None
     116        self._server_process_constructor = server_process.ServerProcess  # overridable for testing
    113117        self._http_lock = None  # FIXME: Why does this live on the port object?
    114118
    … …  
    310314        |tolerance| should be a percentage value (0.0 - 100.0).
    311315        If it is omitted, the port default tolerance value is used.
     316
     317        If an error occurs (like ImageDiff isn't found, or crashes, we log an error and return True (for a diff).
    312318        """
    313         raise NotImplementedError('Port.diff_image')
     319        if not actual_contents and not expected_contents:
     320            return (None, 0)
     321        if not actual_contents or not expected_contents:
     322            return (True, 0)
     323        if not self._image_differ:
     324            self._image_differ = image_diff.ImageDiffer(self)
     325        self.set_option_default('tolerance', 0.1)
     326        tolerance = tolerance or self.get_option('tolerance')
     327        return self._image_differ.diff_image(expected_contents, actual_contents, tolerance)
    314328
    315329    def diff_text(self, expected_text, actual_text, expected_filename, actual_filename):
    … …  
    781795    def clean_up_test_run(self):
    782796        """Perform port-specific work at the end of a test run."""
    783         pass
     797        if self._image_differ:
     798            self._image_differ.stop()
     799            self._image_differ = None
    784800
    785801    # FIXME: os.environ access should be moved to onto a common/system class to be more easily mockable.

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/driver.py

    r123871 r123893  
    3737from webkitpy.common.system import path
    3838
    39 from webkitpy.layout_tests.port import server_process
    40 
    4139
    4240_log = logging.getLogger(__name__)
    … …  
    116114        self._no_timeout = no_timeout
    117115
    118         # overridable for testing.
    119         self._server_process_constructor = server_process.ServerProcess
    120 
    121116        self._driver_tempdir = None
    122117        # WebKitTestRunner can report back subprocess crashes by printing
    … …  
    272267        self._crashed_process_name = None
    273268        self._crashed_pid = None
    274         self._server_process = self._server_process_constructor(self._port, server_name, self.cmd_line(pixel_tests, per_test_args), environment)
     269        self._server_process = self._port._server_process_constructor(self._port, server_name, self.cmd_line(pixel_tests, per_test_args), environment)
    275270        self._server_process.start()
    276271

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/driver_unittest.py

    r123250 r123893  
    3232
    3333from webkitpy.layout_tests.port import Port, Driver, DriverOutput
     34from webkitpy.layout_tests.port.server_process_mock import MockServerProcess
    3435
    3536# FIXME: remove the dependency on TestWebKitPort
    … …  
    233234    def test_stop_cleans_up_properly(self):
    234235        port = TestWebKitPort()
    235         driver = Driver(port, 0, pixel_tests=True)
    236         driver._server_process_constructor = MockServerProcess
     236        port._server_process_constructor = MockServerProcess
     237        driver = Driver(port, 0, pixel_tests=True)
    237238        driver.start(True, [])
    238239        last_tmpdir = port._filesystem.last_tmpdir
    … …  
    243244    def test_two_starts_cleans_up_properly(self):
    244245        port = TestWebKitPort()
    245         driver = Driver(port, 0, pixel_tests=True)
    246         driver._server_process_constructor = MockServerProcess
     246        port._server_process_constructor = MockServerProcess
     247        driver = Driver(port, 0, pixel_tests=True)
    247248        driver.start(True, [])
    248249        last_tmpdir = port._filesystem.last_tmpdir
    … …  
    252253    def test_start_actually_starts(self):
    253254        port = TestWebKitPort()
    254         driver = Driver(port, 0, pixel_tests=True)
    255         driver._server_process_constructor = MockServerProcess
     255        port._server_process_constructor = MockServerProcess
     256        driver = Driver(port, 0, pixel_tests=True)
    256257        driver.start(True, [])
    257258        self.assertTrue(driver._server_process.started)
    258259
    259260
    260 class MockServerProcess(object):
    261     def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None):
    262         self.timed_out = False
    263         self.lines = lines or []
    264         self.crashed = False
    265         self.started = False
    266 
    267     def has_crashed(self):
    268         return self.crashed
    269 
    270     def read_stdout_line(self, deadline):
    271         return self.lines.pop(0) + "\n"
    272 
    273     def read_stdout(self, deadline, size):
    274         first_line = self.lines[0]
    275         if size > len(first_line):
    276             self.lines.pop(0)
    277             remaining_size = size - len(first_line) - 1
    278             if not remaining_size:
    279                 return first_line + "\n"
    280             return first_line + "\n" + self.read_stdout(deadline, remaining_size)
    281         result = self.lines[0][:size]
    282         self.lines[0] = self.lines[0][size:]
    283         return result
    284 
    285     def read_either_stdout_or_stderr_line(self, deadline):
    286         # FIXME: We should have tests which intermix stderr and stdout lines.
    287         return self.read_stdout_line(deadline), None
    288 
    289     def start(self):
    290         self.started = True
    291 
    292     def stop(self, kill_directly=False):
    293         return
    294 
    295     def kill(self):
    296         return
    297 
    298 
    299261if __name__ == '__main__':
    300262    unittest.main()

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/efl.py

    r123731 r123893  
    7373
    7474    def clean_up_test_run(self):
     75        super(EflPort, self).clean_up_test_run()
    7576        self._restore_pulseaudio_module()
    7677

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/gtk.py

    r123058 r123893  
    5252
    5353    def clean_up_test_run(self):
     54        super(GtkPort, self).clean_up_test_run()
    5455        self._restore_pulseaudio_module()
    5556

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py

    r121812 r123893  
    5858        pass
    5959
     60    def test_diff_image(self):
     61        pass
     62
    6063    def test_uses_apache(self):
    6164        pass

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py

    r121701 r123893  
    4242from webkitpy.layout_tests.port import factory
    4343from webkitpy.layout_tests.port.config_mock import MockConfig
     44from webkitpy.layout_tests.port.server_process_mock import MockServerProcess
    4445from webkitpy.tool.mocktool import MockOptions
    4546
    … …  
    210211        self.assertFalse(port.diff_image(None, '')[0])
    211212        self.assertFalse(port.diff_image('', None)[0])
     213
    212214        self.assertFalse(port.diff_image('', '')[0])
    213215
    … …  
    221223        self.assertTrue(port.diff_image('foo', None)[0])
    222224        self.assertTrue(port.diff_image('foo', '')[0])
     225
     226    def test_diff_image(self):
     227        port = self.make_port()
     228        self.proc = None
     229
     230        def make_proc(port, nm, cmd, env):
     231            self.proc = MockServerProcess(port, nm, cmd, env, lines=['diff: 100% failed\n'])
     232            return self.proc
     233
     234        port._server_process_constructor = make_proc
     235        port.setup_test_run()
     236        self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0))
     237        self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
     238        port.clean_up_test_run()
     239        self.assertTrue(self.proc.stopped)
     240        self.assertEquals(port._image_differ, None)
    223241
    224242    def test_check_build(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process.py

    r122913 r123893  
    3333import logging
    3434import signal
    35 import subprocess
    3635import sys
    3736import time
    … …  
    10099        # close_fds is a workaround for http://bugs.python.org/issue2320
    101100        close_fds = not self._host.platform.is_win()
    102         self._proc = subprocess.Popen(self._cmd, stdin=subprocess.PIPE,
    103                                       stdout=subprocess.PIPE,
    104                                       stderr=subprocess.PIPE,
    105                                       close_fds=close_fds,
    106                                       env=self._env,
    107                                       universal_newlines=self._universal_newlines)
     101        self._proc = self._host.executive.popen(self._cmd, stdin=self._host.executive.PIPE,
     102            stdout=self._host.executive.PIPE,
     103            stderr=self._host.executive.PIPE,
     104            close_fds=close_fds,
     105            env=self._env,
     106            universal_newlines=self._universal_newlines)
    108107        self._pid = self._proc.pid
    109108        fd = self._proc.stdout.fileno()

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py

    r123058 r123893  
    3535import logging
    3636import operator
    37 import re
    38 import time
    39 
    40 from webkitpy.common.system.executive import Executive, ScriptError
    41 from webkitpy.layout_tests.port import server_process, Port
     37
     38from webkitpy.common.system.executive import ScriptError
     39from webkitpy.layout_tests.port import Port
    4240
    4341
    … …  
    9795        return []
    9896
    99     def diff_image(self, expected_contents, actual_contents, tolerance=None):
    100         # Handle the case where the test didn't actually generate an image.
    101         # FIXME: need unit tests for this.
    102         if not actual_contents and not expected_contents:
    103             return (None, 0)
    104         if not actual_contents or not expected_contents:
    105             # FIXME: It's not clear what we should return in this case.
    106             # Maybe we should throw an exception?
    107             return (True, 0)
    108 
    109         process = self._start_image_diff_process(expected_contents, actual_contents, tolerance=tolerance)
    110         return self._read_image_diff(process)
    111 
    112     def _image_diff_command(self, tolerance=None):
    113         # FIXME: There needs to be a more sane way of handling default
    114         # values for options so that you can distinguish between a default
    115         # value of None and a default value that wasn't set.
    116         if tolerance is None:
    117             if self.get_option('tolerance') is not None:
    118                 tolerance = self.get_option('tolerance')
    119             else:
    120                 tolerance = 0.1
    121 
    122         command = [self._path_to_image_diff(), '--tolerance', str(tolerance)]
    123         return command
    124 
    125     def _start_image_diff_process(self, expected_contents, actual_contents, tolerance=None):
    126         command = self._image_diff_command(tolerance)
    127         environment = self.setup_environ_for_server('ImageDiff')
    128         process = server_process.ServerProcess(self, 'ImageDiff', command, environment)
    129 
    130         process.write('Content-Length: %d\n%sContent-Length: %d\n%s' % (
    131             len(actual_contents), actual_contents,
    132             len(expected_contents), expected_contents))
    133         return process
    134 
    135     def _read_image_diff(self, sp):
    136         deadline = time.time() + 2.0
    137         output = None
    138         output_image = ""
    139 
    140         while True:
    141             output = sp.read_stdout_line(deadline)
    142             if sp.timed_out or sp.has_crashed() or not output:
    143                 break
    144 
    145             if output.startswith('diff'):  # This is the last line ImageDiff prints.
    146                 break
    147 
    148             if output.startswith('Content-Length'):
    149                 m = re.match('Content-Length: (\d+)', output)
    150                 content_length = int(m.group(1))
    151                 output_image = sp.read_stdout(deadline, content_length)
    152                 output = sp.read_stdout_line(deadline)
    153                 break
    154 
    155         stderr = sp.pop_all_buffered_stderr()
    156         if stderr:
    157             _log.warn("ImageDiff produced stderr output:\n" + stderr)
    158         if sp.timed_out:
    159             _log.error("ImageDiff timed out")
    160         if sp.has_crashed():
    161             _log.error("ImageDiff crashed")
    162         # FIXME: There is no need to shut down the ImageDiff server after every diff.
    163         sp.stop()
    164 
    165         diff_percent = 0
    166         if output and output.startswith('diff'):
    167             m = re.match('diff: (.+)% (passed|failed)', output)
    168             if m.group(2) == 'passed':
    169                 return [None, 0]
    170             diff_percent = float(m.group(1))
    171 
    172         return (output_image, diff_percent)
    173 
    17497    def _tests_for_other_platforms(self):
    17598        # By default we will skip any directory under LayoutTests/platform
    … …  
    202125        for path_to_module in self._modules_to_search_for_symbols():
    203126            try:
    204                 symbols += self._executive.run_command([self.nm_command(), path_to_module], error_handler=Executive.ignore_error)
     127                symbols += self._executive.run_command([self.nm_command(), path_to_module], error_handler=self._executive.ignore_error)
    205128            except OSError, e:
    206129                _log.warn("Failed to run nm: %s.  Can't determine supported features correctly." % e)
Note: See TracChangeset for help on using the changeset viewer.