Skip to content
Permalink
Browse files
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:

Canonical link: https://commits.webkit.org/48733@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@57444 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Adam Barth committed Apr 11, 2010
1 parent 04e0fee commit 67eaa9370b30713f7c21170be40c1aa25925b7f2
Showing 9 changed files with 44 additions and 58 deletions.
@@ -1,3 +1,29 @@
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:

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

Reviewed by Eric Seidel.
@@ -138,6 +138,19 @@ def cpu_count(self):
# machines.
return 2

def kill_process(self, pid):
if platform.system() == "Windows":
# According to http://docs.python.org/library/os.html
# os.kill isn't available on Windows. However, when I tried it
# using Cygwin, it worked fine. We should investigate whether
# we need this platform specific code here.
subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
stdin=open(os.devnull, 'r'),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
return
os.kill(pid, signal.SIGKILL)

# Error handlers do not need to be static methods once all callers are
# updated to use an Executive object.

@@ -572,13 +572,6 @@ def default_configuration(self):
# or any of its subclasses.
#

def _kill_process(self, pid):
"""Forcefully kill a process.
This routine should not be used or needed generically, but can be
used in helper files like http_server.py."""
raise NotImplementedError('Port.kill_process')

def _path_to_apache(self):
"""Returns the full path to the apache binary.
@@ -125,14 +125,6 @@ def _check_wdiff_install(self):
return result


def _kill_process(self, pid):
"""Forcefully kill the process.
Args:
pid: The id of the process to be killed.
"""
os.kill(pid, signal.SIGKILL)

def _kill_all_process(self, process_name):
null = open(os.devnull)
subprocess.call(['killall', '-TERM', '-u', os.getenv('USER'),
@@ -105,14 +105,6 @@ def _lighttpd_path(self, *comps):
return self.path_from_chromium_base('third_party', 'lighttpd',
'mac', *comps)

def _kill_process(self, pid):
"""Forcefully kill the process.
Args:
pid: The id of the process to be killed.
"""
os.kill(pid, signal.SIGKILL)

def _kill_all_process(self, process_name):
"""Kill any processes running under this name."""
# On Mac OS X 10.6, killall has a new constraint: -SIGNALNAME or
@@ -110,17 +110,6 @@ def _lighttpd_path(self, *comps):
return self.path_from_chromium_base('third_party', 'lighttpd', 'win',
*comps)

def _kill_process(self, pid):
"""Forcefully kill the process.
Args:
pid: The id of the process to be killed.
"""
subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
stdin=open(os.devnull, 'r'),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)

def _path_to_apache(self):
return self.path_from_chromium_base('third_party', 'cygwin', 'usr',
'sbin', 'httpd')
@@ -143,15 +143,6 @@ def _tests_for_disabled_features(self):
]
return disabled_feature_tests + webarchive_tests

# FIXME: This doesn't have anything to do with WebKit.
def _kill_process(self, pid):
"""Forcefully kill the process.
Args:
pid: The id of the process to be killed.
"""
os.kill(pid, signal.SIGKILL)

# FIXME: This doesn't have anything to do with WebKit.
def _kill_all_process(self, process_name):
# On Mac OS X 10.6, killall has a new constraint: -SIGNALNAME or
@@ -42,6 +42,8 @@
import factory
import http_server

from webkitpy.common.system.executive import Executive

_log = logging.getLogger("webkitpy.layout_tests.port.websocket_server")

_WS_LOG_PREFIX = 'pywebsocket.ws.log-'
@@ -200,6 +202,7 @@ def start(self):
_log.debug('Starting %s server on %d.' % (
self._server_name, self._port))
_log.debug('cmdline: %s' % ' '.join(start_cmd))
# FIXME: We should direct this call through Executive for testing.
self._process = subprocess.Popen(start_cmd,
stdin=open(os.devnull, 'r'),
stdout=self._wsout,
@@ -250,8 +253,8 @@ def stop(self, force=False):
'Failed to find %s server pid.' % self._server_name)

_log.debug('Shutting down %s server %d.' % (self._server_name, pid))
# FIXME: We shouldn't be calling a protected method of the port_obj!
self._port_obj._kill_process(pid)
# FIXME: We should use a non-static Executive for easier testing.
Executive().kill_process(pid)

if self._process:
self._process.wait()
@@ -83,19 +83,6 @@ def _tests_for_disabled_features(self):
]
return disabled_feature_tests + webarchive_tests

# FIXME: This doesn't have anything to do with WebKit.
# FIXME: Copy/pasted from chromium-win
def _kill_process(self, pid):
"""Forcefully kill the process.
Args:
pid: The id of the process to be killed.
"""
subprocess.call(('taskkill.exe', '/f', '/pid', str(pid)),
stdin=open(os.devnull, 'r'),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)

def _path_to_apache_config_file(self):
return os.path.join(self.layout_tests_dir(), 'http', 'conf',
'cygwin-httpd.conf')

0 comments on commit 67eaa93

Please sign in to comment.