Title: [91411] trunk/Tools
Revision
91411
Author
e...@webkit.org
Date
2011-07-20 15:06:03 -0700 (Wed, 20 Jul 2011)

Log Message

Move HttpLock to using a FileSystem object
https://bugs.webkit.org/show_bug.cgi?id=64885

Reviewed by Adam Barth.

I made a typo in my previous change, taking the value
of read_text_file(pid_file) and setting it to lock_pid_file
instead of current_pid.  Fixed now. :)

In order to test my new change I had to overhaul the unittests
for this class to create a separate set of tests which work off
of Mock objects instead of the real filesystem.

Since Executive doesn't yet wrap os.getpid() I added a FIXME
in several places where we're currently calling os.getpid().  I
felt adding Executive.getpid was outside of the scope of this change
but once it exists some of this code will be much simpler to mock.

* Scripts/webkitpy/layout_tests/port/http_lock.py:
* Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:
* Scripts/webkitpy/tool/mocktool.py:

Modified Paths

Diff

Modified: trunk/Tools/ChangeLog (91410 => 91411)


--- trunk/Tools/ChangeLog	2011-07-20 22:00:29 UTC (rev 91410)
+++ trunk/Tools/ChangeLog	2011-07-20 22:06:03 UTC (rev 91411)
@@ -1,3 +1,27 @@
+2011-07-20  Eric Seidel  <e...@webkit.org>
+
+        Move HttpLock to using a FileSystem object
+        https://bugs.webkit.org/show_bug.cgi?id=64885
+
+        Reviewed by Adam Barth.
+
+        I made a typo in my previous change, taking the value
+        of read_text_file(pid_file) and setting it to lock_pid_file
+        instead of current_pid.  Fixed now. :)
+
+        In order to test my new change I had to overhaul the unittests
+        for this class to create a separate set of tests which work off
+        of Mock objects instead of the real filesystem.
+
+        Since Executive doesn't yet wrap os.getpid() I added a FIXME
+        in several places where we're currently calling os.getpid().  I
+        felt adding Executive.getpid was outside of the scope of this change
+        but once it exists some of this code will be much simpler to mock.
+
+        * Scripts/webkitpy/layout_tests/port/http_lock.py:
+        * Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+
 2011-07-20  Adam Roben  <aro...@apple.com>
 
         Fix typo in TestFailures's Bugzilla constants

Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock.py (91410 => 91411)


--- trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock.py	2011-07-20 22:00:29 UTC (rev 91410)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock.py	2011-07-20 22:06:03 UTC (rev 91411)
@@ -49,6 +49,7 @@
         self._filesystem = filesystem or FileSystem()
         self._lock_path = lock_path
         if not self._lock_path:
+            # FIXME: FileSystem should have an accessor for tempdir()
             self._lock_path = tempfile.gettempdir()
         self._lock_file_prefix = lock_file_prefix
         self._lock_file_path_prefix = self._filesystem.join(self._lock_path, self._lock_file_prefix)
@@ -69,7 +70,7 @@
 
     def _lock_file_list(self):
         """Return the list of lock files sequentially."""
-        lock_list = glob.glob(self._lock_file_path_prefix + '*')
+        lock_list = self._filesystem.glob(self._lock_file_path_prefix + '*')
         lock_list.sort(key=self._extract_lock_number)
         return lock_list
 
@@ -80,7 +81,7 @@
             return 0
         return self._extract_lock_number(lock_list[-1]) + 1
 
-    def _curent_lock_pid(self):
+    def _current_lock_pid(self):
         """Return with the current lock pid. If the lock is not valid
         it deletes the lock file."""
         lock_list = self._lock_file_list()
@@ -88,7 +89,7 @@
             _log.debug("No lock file list")
             return
         try:
-            current_lock_file = self._filesystem.read_text_file(lock_list[0])
+            current_pid = self._filesystem.read_text_file(lock_list[0])
             if not (current_pid and self._executive.check_running_pid(int(current_pid))):
                 _log.debug("Removing stuck lock file: %s" % lock_list[0])
                 self._filesystem.remove(lock_list[0])
@@ -115,6 +116,7 @@
 
         self._process_lock_file_name = (self._lock_file_path_prefix + str(self._next_lock_number()))
         _log.debug("Creating lock file: %s" % self._process_lock_file_name)
+        # FIXME: Executive.py should have an accessor for getpid()
         self._filesystem.write_text_file(self._process_lock_file_name, str(os.getpid()))
         self._guard_lock.release_lock()
         return True
@@ -126,7 +128,8 @@
             _log.debug("Warning, http locking failed!")
             return
 
-        while self._curent_lock_pid() != os.getpid():
+        # FIXME: This can hang forever!
+        while self._current_lock_pid() != os.getpid():
             time.sleep(1)
 
         _log.debug("HTTP lock acquired")

Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py (91410 => 91411)


--- trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py	2011-07-20 22:00:29 UTC (rev 91410)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py	2011-07-20 22:06:03 UTC (rev 91411)
@@ -24,28 +24,32 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-import glob
-import http_lock
-import os
+from http_lock import HttpLock
+import os  # Used for os.getpid()
 import unittest
 
+from webkitpy.common.system.filesystem_mock import MockFileSystem
+from webkitpy.tool.mocktool import MockExecutive
 
-class HttpLockTest(unittest.TestCase):
-    # FIXME: These tests all touch the real disk, but could be written to a MockFileSystem instead.
+
+# FIXME: These tests all touch the real disk, but could be written to a MockFileSystem instead.
+class HttpLockTestWithRealFileSystem(unittest.TestCase):
+    # FIXME: Unit tests do not use an __init__ method, but rather setUp and tearDown methods.
     def __init__(self, testFunc):
-        self.http_lock_obj = http_lock.HttpLock(None, "WebKitTestHttpd.lock.", "WebKitTest.lock")
-        self.lock_file_path_prefix = os.path.join(self.http_lock_obj._lock_path, self.http_lock_obj._lock_file_prefix)
+        self.http_lock = HttpLock(None, "WebKitTestHttpd.lock.", "WebKitTest.lock")
+        self.filesystem = self.http_lock._filesystem  # FIXME: We should be passing in a MockFileSystem instead.
+        self.lock_file_path_prefix = self.filesystem.join(self.http_lock._lock_path, self.http_lock._lock_file_prefix)
         self.lock_file_name = self.lock_file_path_prefix + "0"
-        self.guard_lock_file = self.http_lock_obj._guard_lock_file
+        self.guard_lock_file = self.http_lock._guard_lock_file
         self.clean_all_lockfile()
         unittest.TestCase.__init__(self, testFunc)
 
     def clean_all_lockfile(self):
-        if os.path.exists(self.guard_lock_file):
-            os.unlink(self.guard_lock_file)
-        lock_list = glob.glob(self.lock_file_path_prefix + '*')
+        if self.filesystem.exists(self.guard_lock_file):
+            self.filesystem.unlink(self.guard_lock_file)
+        lock_list = self.filesystem.glob(self.lock_file_path_prefix + '*')
         for file_name in lock_list:
-            os.unlink(file_name)
+            self.filesystem.unlink(file_name)
 
     def assertEqual(self, first, second):
         if first != second:
@@ -53,27 +57,40 @@
         unittest.TestCase.assertEqual(self, first, second)
 
     def _check_lock_file(self):
-        if os.path.exists(self.lock_file_name):
+        if self.filesystem.exists(self.lock_file_name):
             pid = os.getpid()
-            lock_file = open(self.lock_file_name, 'r')
-            lock_file_pid = lock_file.readline()
-            lock_file.close()
+            lock_file_pid = self.filesystem.read_text_file(self.lock_file_name)
             self.assertEqual(pid, int(lock_file_pid))
             return True
         return False
 
     def test_lock_lifecycle(self):
-        self.http_lock_obj._create_lock_file()
+        self.http_lock._create_lock_file()
 
         self.assertEqual(True, self._check_lock_file())
-        self.assertEqual(1, self.http_lock_obj._next_lock_number())
+        self.assertEqual(1, self.http_lock._next_lock_number())
 
-        self.http_lock_obj.cleanup_http_lock()
+        self.http_lock.cleanup_http_lock()
 
         self.assertEqual(False, self._check_lock_file())
-        self.assertEqual(0, self.http_lock_obj._next_lock_number())
+        self.assertEqual(0, self.http_lock._next_lock_number())
 
-    def test_extract_lock_number(self,):
+
+class HttpLockTest(unittest.TestCase):
+    def setUp(self):
+        self.filesystem = MockFileSystem()
+        self.http_lock = HttpLock(None, "WebKitTestHttpd.lock.", "WebKitTest.lock", filesystem=self.filesystem, executive=MockExecutive())
+        # FIXME: Shouldn't we be able to get these values from the http_lock object directly?
+        self.lock_file_path_prefix = self.filesystem.join(self.http_lock._lock_path, self.http_lock._lock_file_prefix)
+        self.lock_file_name = self.lock_file_path_prefix + "0"
+
+    def test_current_lock_pid(self):
+        # FIXME: Once Executive wraps getpid, we can mock this and not use a real pid.
+        current_pid = os.getpid()
+        self.http_lock._filesystem.write_text_file(self.lock_file_name, str(current_pid))
+        self.assertEquals(self.http_lock._current_lock_pid(), current_pid)
+
+    def test_extract_lock_number(self):
         lock_file_list = (
             self.lock_file_path_prefix + "00",
             self.lock_file_path_prefix + "9",
@@ -84,15 +101,15 @@
         expected_number_list = (0, 9, 1, 21)
 
         for lock_file, expected in zip(lock_file_list, expected_number_list):
-            self.assertEqual(self.http_lock_obj._extract_lock_number(lock_file), expected)
+            self.assertEqual(self.http_lock._extract_lock_number(lock_file), expected)
 
     def test_lock_file_list(self):
-        lock_file_list = [
-            self.lock_file_path_prefix + "6",
-            self.lock_file_path_prefix + "1",
-            self.lock_file_path_prefix + "4",
-            self.lock_file_path_prefix + "3",
-        ]
+        self.http_lock._filesystem = MockFileSystem({
+            self.lock_file_path_prefix + "6": "",
+            self.lock_file_path_prefix + "1": "",
+            self.lock_file_path_prefix + "4": "",
+            self.lock_file_path_prefix + "3": "",
+        })
 
         expected_file_list = [
             self.lock_file_path_prefix + "1",
@@ -101,10 +118,4 @@
             self.lock_file_path_prefix + "6",
         ]
 
-        for file_name in lock_file_list:
-            open(file_name, 'w')
-
-        self.assertEqual(self.http_lock_obj._lock_file_list(), expected_file_list)
-
-        for file_name in lock_file_list:
-            os.unlink(file_name)
+        self.assertEqual(self.http_lock._lock_file_list(), expected_file_list)

Modified: trunk/Tools/Scripts/webkitpy/tool/mocktool.py (91410 => 91411)


--- trunk/Tools/Scripts/webkitpy/tool/mocktool.py	2011-07-20 22:00:29 UTC (rev 91410)
+++ trunk/Tools/Scripts/webkitpy/tool/mocktool.py	2011-07-20 22:06:03 UTC (rev 91411)
@@ -715,7 +715,12 @@
     def __init__(self, should_log=False, should_throw=False):
         self._should_log = should_log
         self._should_throw = should_throw
+        # FIXME: Once executive wraps os.getpid() we can just use a static pid for "this" process.
+        self._running_pids = [os.getpid()]
 
+    def check_running_pid(self, pid):
+        return pid in self._running_pids
+
     def run_and_throw_if_fail(self, args, quiet=False, cwd=None):
         if self._should_log:
             log("MOCK run_and_throw_if_fail: %s, cwd=%s" % (args, cwd))
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to