Changeset 57444 in webkit


Ignore:
Timestamp:
Apr 10, 2010 11:52:12 PM (14 years ago)
Author:
abarth@webkit.org
Message:

2010-04-10 Adam Barth <​abarth@webkit.org>

Reviewed by Eric Seidel.

kill_process is copy/pasted in five places
​https://bugs.webkit.org/show_bug.cgi?id=37405

We shouldn't replicate the kill_process logic in every port. Instead,
we should move the process interaction to Executive.

Dirk mentioned that he wanted this abstraction to make it easier to
mock things out for testing. It turns out this function is only used
in one place where it can't be used as a mock point for testing because
the corresponding create process actually creates a real process. In
the long term, we should indirect both these calls through a non-static
Executive as a mock point. However, we should wait on that until we
actually want to write the test.

  • Scripts/webkitpy/layout_tests/port/base.py:
  • Scripts/webkitpy/layout_tests/port/chromium_linux.py:
  • Scripts/webkitpy/layout_tests/port/chromium_mac.py:
  • Scripts/webkitpy/layout_tests/port/chromium_win.py:
  • Scripts/webkitpy/layout_tests/port/mac.py:
  • Scripts/webkitpy/layout_tests/port/websocket_server.py:
  • Scripts/webkitpy/layout_tests/port/win.py:
Location:
trunk/WebKitTools
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebKitTools/ChangeLog

    r57443 r57444  
     12010-04-10  Adam Barth  <abarth@webkit.org>
     2
     3        Reviewed by Eric Seidel.
     4
     5        kill_process is copy/pasted in five places
     6        https://bugs.webkit.org/show_bug.cgi?id=37405
     7
     8        We shouldn't replicate the kill_process logic in every port.  Instead,
     9        we should move the process interaction to Executive.
     10
     11        Dirk mentioned that he wanted this abstraction to make it easier to
     12        mock things out for testing.  It turns out this function is only used
     13        in one place where it can't be used as a mock point for testing because
     14        the corresponding create process actually creates a real process.  In
     15        the long term, we should indirect both these calls through a non-static
     16        Executive as a mock point.  However, we should wait on that until we
     17        actually want to write the test.
     18
     19        * Scripts/webkitpy/layout_tests/port/base.py:
     20        * Scripts/webkitpy/layout_tests/port/chromium_linux.py:
     21        * Scripts/webkitpy/layout_tests/port/chromium_mac.py:
     22        * Scripts/webkitpy/layout_tests/port/chromium_win.py:
     23        * Scripts/webkitpy/layout_tests/port/mac.py:
     24        * Scripts/webkitpy/layout_tests/port/websocket_server.py:
     25        * Scripts/webkitpy/layout_tests/port/win.py:
     26
    1272010-04-10  Adam Barth  <abarth@webkit.org>
    228

  • trunk/WebKitTools/Scripts/webkitpy/common/system/executive.py

    r57427 r57444  
    139139        return 2
    140140
     141    def kill_process(self, pid):
     142        if platform.system() == "Windows":
     143            # According to http://docs.python.org/library/os.html
     144            # os.kill isn't available on Windows.  However, when I tried it
     145            # using Cygwin, it worked fine.  We should investigate whether
     146            # we need this platform specific code here.
     147            subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
     148                            stdin=open(os.devnull, 'r'),
     149                            stdout=subprocess.PIPE,
     150                            stderr=subprocess.PIPE)
     151            return
     152        os.kill(pid, signal.SIGKILL)
     153
    141154    # Error handlers do not need to be static methods once all callers are
    142155    # updated to use an Executive object.

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

    r57422 r57444  
    573573    #
    574574
    575     def _kill_process(self, pid):
    576         """Forcefully kill a process.
    577 
    578         This routine should not be used or needed generically, but can be
    579         used in helper files like http_server.py."""
    580         raise NotImplementedError('Port.kill_process')
    581 
    582575    def _path_to_apache(self):
    583576        """Returns the full path to the apache binary.

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_linux.py

    r57399 r57444  
    126126
    127127
    128     def _kill_process(self, pid):
    129         """Forcefully kill the process.
    130 
    131         Args:
    132         pid: The id of the process to be killed.
    133         """
    134         os.kill(pid, signal.SIGKILL)
    135 
    136128    def _kill_all_process(self, process_name):
    137129        null = open(os.devnull)

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py

    r57399 r57444  
    106106                                            'mac', *comps)
    107107
    108     def _kill_process(self, pid):
    109         """Forcefully kill the process.
    110 
    111         Args:
    112             pid: The id of the process to be killed.
    113         """
    114         os.kill(pid, signal.SIGKILL)
    115 
    116108    def _kill_all_process(self, process_name):
    117109        """Kill any processes running under this name."""

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py

    r57399 r57444  
    111111                                            *comps)
    112112
    113     def _kill_process(self, pid):
    114         """Forcefully kill the process.
    115 
    116         Args:
    117         pid: The id of the process to be killed.
    118         """
    119         subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
    120                         stdin=open(os.devnull, 'r'),
    121                         stdout=subprocess.PIPE,
    122                         stderr=subprocess.PIPE)
    123 
    124113    def _path_to_apache(self):
    125114        return self.path_from_chromium_base('third_party', 'cygwin', 'usr',

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/mac.py

    r57426 r57444  
    145145
    146146    # FIXME: This doesn't have anything to do with WebKit.
    147     def _kill_process(self, pid):
    148         """Forcefully kill the process.
    149 
    150         Args:
    151         pid: The id of the process to be killed.
    152         """
    153         os.kill(pid, signal.SIGKILL)
    154 
    155     # FIXME: This doesn't have anything to do with WebKit.
    156147    def _kill_all_process(self, process_name):
    157148        # On Mac OS X 10.6, killall has a new constraint: -SIGNALNAME or

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py

    r57423 r57444  
    4343import http_server
    4444
     45from webkitpy.common.system.executive import Executive
     46
    4547_log = logging.getLogger("webkitpy.layout_tests.port.websocket_server")
    4648
    … …  
    201203                   self._server_name, self._port))
    202204        _log.debug('cmdline: %s' % ' '.join(start_cmd))
     205        # FIXME: We should direct this call through Executive for testing.
    203206        self._process = subprocess.Popen(start_cmd,
    204207                                         stdin=open(os.devnull, 'r'),
    … …  
    251254
    252255        _log.debug('Shutting down %s server %d.' % (self._server_name, pid))
    253         # FIXME: We shouldn't be calling a protected method of the port_obj!
    254         self._port_obj._kill_process(pid)
     256        # FIXME: We should use a non-static Executive for easier testing.
     257        Executive().kill_process(pid)
    255258
    256259        if self._process:

  • trunk/WebKitTools/Scripts/webkitpy/layout_tests/port/win.py

    r57426 r57444  
    8484        return disabled_feature_tests + webarchive_tests
    8585
    86     # FIXME: This doesn't have anything to do with WebKit.
    87     # FIXME: Copy/pasted from chromium-win
    88     def _kill_process(self, pid):
    89         """Forcefully kill the process.
    90 
    91         Args:
    92         pid: The id of the process to be killed.
    93         """
    94         subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
    95                         stdin=open(os.devnull, 'r'),
    96                         stdout=subprocess.PIPE,
    97                         stderr=subprocess.PIPE)
    98 
    9986    def _path_to_apache_config_file(self):
    10087        return os.path.join(self.layout_tests_dir(), 'http', 'conf',
Note: See TracChangeset for help on using the changeset viewer.