Merge lp:~mandel/ubuntuone-client/improve_watcher_tests into lp:ubuntuone-client

[Manuel de la Peña]

Improve the tests that are used in the watcher code so that it does not longer uses mocker.

Improve the tests that are used in the watcher code so that it does not longer uses mocker.

[lp:~mandel/ubuntuone-client/improve_watcher_tests]

1088. By Manuel de la Peña on 2011-07-28

Remove unused wd.

1089. By Manuel de la Peña on 2011-07-28

Merged with trunk.

1090. By Manuel de la Peña on 2011-07-28

Went back to use get_wd.

[Guillermo Gonzalez (verterok)]

looks good.

review: Approve (linux)

[lp:~mandel/ubuntuone-client/improve_watcher_tests]

1091. By Alejandro J. Cura on 2011-07-28

Verify the events were processed in the right thread

1092. By Manuel de la Peña on 2011-07-28

Added missing docs.

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'tests/platform/windows/test_filesystem_notifications.py'
--- tests/platform/windows/test_filesystem_notifications.py	2011-07-28 13:56:40 +0000
+++ tests/platform/windows/test_filesystem_notifications.py	2011-07-28 19:33:25 +0000
@@ -1,5 +1,6 @@
 #
-# Author: Manuel de la Pena <[e-mail]>
+# Authors: Manuel de la Pena <[e-mail]>
+#          Alejandro J. Cura <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -18,13 +19,14 @@
 
 import os
 import tempfile
+import thread
 import time
 
-from mocker import MockerTestCase, ANY
-
 from contrib.testing.testcase import BaseTwistedTestCase
+from ubuntuone.platform.windows import os_helper
 from ubuntuone.platform.windows.pyinotify import ProcessEvent
 from ubuntuone.platform.windows.filesystem_notifications import (
+    Watch,
     WatchManager,
     Notifier,
     FILE_NOTIFY_CHANGE_FILE_NAME,
@@ -40,28 +42,43 @@
 class TestCaseHandler(ProcessEvent):
     """ProcessEvent used for test cases."""
 
-    def my_init(self, **kwargs):
+    thread_id = None
+
+    def my_init(self, main_thread_id=None, **kwargs):
         """Init the event notifier."""
         self.processed_events = []
+        self.main_thread_id = main_thread_id
 
     def process_IN_CREATE(self, event):
+        """Process events and added to the list."""
         self.processed_events.append(event)
+        self._verify_thread_id()
 
     def process_IN_DELETE(self, event):
+        """Process events and added to the list."""
         self.processed_events.append(event)
+        self._verify_thread_id()
 
     def process_default(self, event):
         """Process events and added to the list."""
         self.processed_events.append(event)
+        self._verify_thread_id()
+
+    def _verify_thread_id(self):
+        """Verify that the event was processed in the correct thread."""
+        if self.main_thread_id:
+            assert self.main_thread_id == thread.get_ident()
 
 
 class TestWatch(BaseTwistedTestCase):
     """Test the watch so that it returns the same events as pyinotify."""
 
+    timeout = 10
+
     def setUp(self):
         """Set infor for the tests."""
         super(TestWatch, self).setUp()
-        self.basedir = os.path.abspath(self.mktemp('test_root'))
+        self.basedir = self.mktemp('test_root')
         self.mask = FILE_NOTIFY_CHANGE_FILE_NAME | \
             FILE_NOTIFY_CHANGE_DIR_NAME | \
             FILE_NOTIFY_CHANGE_ATTRIBUTES | \
@@ -70,18 +87,20 @@
             FILE_NOTIFY_CHANGE_SECURITY | \
             FILE_NOTIFY_CHANGE_LAST_ACCESS
 
+        self.handler = TestCaseHandler(main_thread_id=thread.get_ident())
+
     def _perform_operations(self, path, mask, auto_add, actions, number_events):
         """Perform the file operations and returns the recorded events."""
         manager = WatchManager()
-        manager.add_watch(path, mask, auto_add=auto_add)
-        handler = TestCaseHandler()
-        notifier = Notifier(manager, handler)
+        manager.add_watch(os_helper.get_windows_valid_path(path), mask,
+                          auto_add=auto_add)
+        notifier = Notifier(manager, self.handler)
         # execution the actions
         actions()
         # process the recorded events
-        while not len(handler.processed_events) >= number_events:
+        while not len(self.handler.processed_events) >= number_events:
             notifier.process_events()
-        events = handler.processed_events
+        events = self.handler.processed_events
         notifier.stop()
         return events
 
@@ -89,15 +108,15 @@
                                   time_out):
         """Perform the file operations and returns the recorded events."""
         manager = WatchManager()
-        manager.add_watch(path, mask, auto_add=auto_add)
-        handler = TestCaseHandler()
-        notifier = Notifier(manager, handler)
+        manager.add_watch(os_helper.get_windows_valid_path(path), mask,
+                          auto_add=auto_add)
+        notifier = Notifier(manager, self.handler)
         # execution the actions
         actions()
         # process the recorded events
         time.sleep(time_out)
         notifier.process_events()
-        events = handler.processed_events
+        events = self.handler.processed_events
         notifier.stop()
         return events
 
@@ -371,9 +390,10 @@
         """Test that the exclude filter works as expectd."""
         manager = WatchManager()
         # add a watch that will always exclude all actions
-        manager.add_watch(self.basedir, self.mask, auto_add=True,
-            exclude_filter=lambda x: True )
-        handler = TestCaseHandler()
+        manager.add_watch(os_helper.get_windows_valid_path(self.basedir),
+                          self.mask, auto_add=True,
+                          exclude_filter=lambda x: True )
+        handler = TestCaseHandler(main_thread_id=thread.get_ident())
         notifier = Notifier(manager, handler)
         # execution the actions
         file_name = os.path.join(self.basedir, 'test_file_create')
@@ -398,30 +418,40 @@
             open_dir, 2)
         self.assertEqual(0, len(events))
 
-TestWatch.skip = 'To be fixed by mandel and alecu.'
-
-
-class TestWatchManager(BaseTwistedTestCase, MockerTestCase):
+
+class FakeEvent():
+    """A fake event."""
+
+    def __init__(self, path):
+        """Create an event with the given path."""
+        self.pathname = path
+
+
+class TestWatchManager(BaseTwistedTestCase):
     """Test the watch manager."""
 
     def setUp(self):
         """Set each of the tests."""
         super(TestWatchManager, self).setUp()
-        self.watch_factory = self.mocker.mock()
-        self.watch = self.mocker.mock()
-        self.manager = WatchManager(watch_factory=self.watch_factory)
+        self.path = u'\\\\?\\C:\\path'  # a valid windows path
+        self.watch = Watch(1, self.path, None, True)
+        self.manager = WatchManager()
+        self.manager._wdm = {1: self.watch}
 
     def test_stop(self):
         """Test that the different watches are stoppe."""
-        self.manager._wdm = {1: self.watch}
-        self.watch.path
-        self.watch.stop_watching()
-        self.mocker.replay()
+        self.was_called = False
+
+        def stop_watching():
+            """Fake stop watch."""
+            self.was_called = True
+
+        self.watch.stop_watching = stop_watching
         self.manager.stop()
+        self.assertTrue(self.was_called, 'The watch stop was not called.')
 
     def test_get_present_watch(self):
         """Test that we can get a Watch using is wd."""
-        self.manager._wdm = {1: self.watch}
         self.assertEqual(self.watch, self.manager.get_watch(1))
 
     def test_get_missing_watch(self):
@@ -430,112 +460,100 @@
 
     def test_delete_present_watch(self):
         """Test that we can remove a present watch."""
-        self.manager._wdm = {1: self.watch}
-        self.watch.stop_watching()
-        self.mocker.replay()
-        self.mocker.replay()
+        self.was_called = False
+
+        def stop_watching():
+            """Fake stop watch."""
+            self.was_called = True
+
+        self.watch.stop_watching = stop_watching
         self.manager.del_watch(1)
+        self.assertRaises(KeyError, self.manager.get_watch, (1,))
 
     def test_add_single_watch(self):
         """Test the addition of a new single watch."""
-        wd = 0
-        path = 'path'
+        self.was_called = False
+
+        def start_watching(*args):
+            """Fake stop watch."""
+            self.was_called = True
+
+        Watch.start_watching = start_watching
+        self.manager._wdm = {}
+
         mask = 'bit_mask'
         auto_add = True
-        events_queue = self.manager._events_queue
         exclude_filter = lambda x: False
         proc_fun = lambda x: None
-        # expect the factory to be called with the passed values
-        valid_path = u'\\\\?\\C:\\path'  # a valid windows path
-        self.watch_factory(wd, valid_path, mask, auto_add,
-            events_queue=events_queue, exclude_filter=exclude_filter,
-            proc_fun=proc_fun)
-        self.mocker.result(self.watch)
-        self.watch.start_watching()
-        self.mocker.replay()
-        self.manager.add_watch(path, mask, proc_fun=proc_fun, auto_add=auto_add,
+        self.manager.add_watch(self.path, mask, proc_fun=proc_fun, auto_add=auto_add,
             exclude_filter=exclude_filter)
         self.assertEqual(1, len(self.manager._wdm))
+        self.assertTrue(self.was_called, 'The watch start was not called.')
+        self.assertEqual(self.path, self.manager._wdm[0]._path)
+        self.assertEqual(mask, self.manager._wdm[0]._mask)
+        self.assertEqual(auto_add, self.manager._wdm[0]._auto_add)
+        self.assertEqual(proc_fun, self.manager._wdm[0]._proc_fun)
+        self.assertEqual(exclude_filter, self.manager._wdm[0].exclude_filter)
 
     def test_update_present_watch(self):
         """Test the update of a present watch."""
-        self.manager._wdm = {1: self.watch}
+        self.stop_was_called = False
+        self.start_was_called = False
+
+        def stop_watching():
+            """Fake stop watch."""
+            self.stop_was_called = True
+
+        def start_watching():
+            """Fake stop watch."""
+            self.start_was_called = True
+
+        proc_fun = lambda x: None
         mask = 'mask'
-        proc_fun = lambda x: None
-        self.watch.path
-        self.watch.stop_watching()
-        self.watch.update(mask, proc_fun=proc_fun, auto_add=False)
-        self.watch.start_watching()
-        self.mocker.replay()
+        self.watch.stop_watching = stop_watching
+        self.watch.start_watching = start_watching
         self.manager.update_watch(1, mask, proc_fun)
-
-    def test_get_wath_present_wd(self):
-        """Test that the correct path is returned."""
-        self.manager._wdm = {1: self.watch}
-        path = 'path'
-        self.watch.path
-        self.mocker.result(path)
-        self.mocker.replay()
-        self.assertEqual(path, self.manager.get_path(1))
-
-    def test_get_wath_missing_wd(self):
-        """Test that the correct path is returned."""
+        self.assertEqual(self.watch._mask, mask)
+        self.assertEqual(self.watch._proc_fun, proc_fun)
+        self.assertTrue(self.stop_was_called, 'The watch stop was not called.')
+        self.assertTrue(self.start_was_called,
+                             'The watch start was not called.')
+
+    def test_get_watch_present_wd(self):
+        """Test that the correct path is returned."""
+        self.assertEqual(self.path, self.manager.get_path(1))
+
+    def test_get_watch_missing_wd(self):
+        """Test that the correct path is returned."""
+        self.manager._wdm = {}
         self.assertEqual(None, self.manager.get_path(1))
 
     def test_get_wd_exact_path(self):
         """Test the wd is returned when there is a watch for the path."""
-        self.manager._wdm = {1: self.watch}
-        path = 'path'
-        self.watch.auto_add
-        self.mocker.result(True)
-        self.watch.path
-        self.mocker.result(path)
-        self.mocker.replay()
-        self.assertEqual(1, self.manager.get_wd(path))
+        self.assertEqual(1, self.manager.get_wd(self.path))
 
     def test_get_wd_child_path(self):
         """Test the wd is returned when we have a child path."""
-        self.manager._wdm = {1: self.watch}
-        path = 'path'
-        self.watch.path
-        self.mocker.result(path)
-        self.watch.auto_add
-        self.mocker.result(True)
-        self.mocker.replay()
-        self.assertEqual(1, self.manager.get_wd(os.path.join(path, 'child')))
+        child = os.path.join(self.path, 'child')
+        self.assertEqual(1, self.manager.get_wd(child))
 
     def test_rm_present_wd(self):
         """Test the removal of a present watch."""
-        self.manager._wdm = {1: self.watch}
-        self.watch.stop_watching()
-        self.mocker.replay()
         self.manager.rm_watch(1)
         self.assertEqual(None, self.manager._wdm.get(1))
 
     def test_rm_root_path(self):
         """Test the removal of a root path."""
-        self.manager._wdm = {1: self.watch}
-        path = 'path'
-        self.watch.path
-        self.mocker.result(path)
-        self.watch.auto_add
-        self.mocker.result(True)
-        self.watch.exclude_filter = ANY
-        self.mocker.replay()
-        self.manager.rm_path(path)
+        self.manager.rm_path(self.path)
         self.assertEqual(self.watch, self.manager._wdm.get(1))
+        event = FakeEvent(self.path)
+        self.assertTrue(self.watch.exclude_filter(event))
 
     def test_rm_child_path(self):
         """Test the removal of a child path."""
-        self.manager._wdm = {1: self.watch}
-        path = 'path'
-        self.watch.path
-        self.mocker.result(path)
-        self.watch.auto_add
-        self.mocker.result(True)
-        self.watch.exclude_filter = ANY
-        self.mocker.replay()
-        self.manager.rm_path(os.path.join(path, 'child'))
+        child = os.path.join(self.path, u'child')
+        event = FakeEvent(child)
+        self.manager.rm_path(child)
         self.assertEqual(self.watch, self.manager._wdm[1])
-
-TestWatchManager.skip = 'To be fixed by mandel and alecu.'
+        # assert that the path is ignored
+        self.assertTrue(self.watch.exclude_filter(event))

=== modified file 'ubuntuone/platform/windows/filesystem_notifications.py'
--- ubuntuone/platform/windows/filesystem_notifications.py	2011-07-28 15:35:45 +0000
+++ ubuntuone/platform/windows/filesystem_notifications.py	2011-07-28 19:33:25 +0000
@@ -119,8 +119,7 @@
     FILE_NOTIFY_CHANGE_LAST_ACCESS
 
 
-# The implementation of the code that is provided as the pyinotify
-# substitute
+# The implementation of the code that is provided as the pyinotify substitute
 class Watch(object):
     """Implement the same functions as pyinotify.Watch."""
 
@@ -157,6 +156,11 @@
             events_queue = Queue()
         self.events_queue = events_queue
 
+    def _is_excluded(self, event):
+        """Return if an event is ignored."""
+        return (self.exclude_filter and self.exclude_filter(event)) or\
+            event.mask == IN_OPEN | IN_ISDIR
+
     # XXX: confirm is using this decorator is correct!!! (nessita)
     @is_valid_windows_path(path_indexes=[1])
     def _path_is_dir(self, path):
@@ -177,12 +181,6 @@
             self._subdirs.append(path)
         return is_dir
 
-    def _is_excluded(self, event):
-        """Return if an event is ignored."""
-        return (self.exclude_filter and self.exclude_filter(event)) or\
-            event.mask == IN_OPEN | IN_ISDIR
-
-
     def _process_events(self):
         """Process the events form the queue."""
         # we transform the events to be the same as the one in pyinotify
@@ -195,27 +193,21 @@
                 continue
             # map the windows events to the pyinotify ones, tis is dirty but
             # makes the multiplatform better, linux was first :P
-            is_dir = self._path_is_dir(file_name)
-            if os.path.exists(file_name):
-                is_dir = os.path.isdir(file_name)
-            else:
-                # we removed the path, we look in the internal list
-                if file_name in self._subdirs:
-                    is_dir = True
-                    self._subdirs.remove(file_name)
+            syncdaemon_path = get_syncdaemon_valid_path(
+                                        os.path.join(self._path, file_name))
+            is_dir = self._path_is_dir(os.path.join(self._path, file_name))
             if is_dir:
                 self._subdirs.append(file_name)
             mask = WINDOWS_ACTIONS[action]
             head, tail = os.path.split(file_name)
             if is_dir:
                 mask |= IN_ISDIR
-            syncdaemon_path = get_syncdaemon_valid_path(self._path)
             event_raw_data = {
                 'wd': self._descriptor,
                 'dir': is_dir,
                 'mask': mask,
                 'name': tail,
-                'path': head.replace(syncdaemon_path, '.')}
+                'path': '.'}
             # by the way in which the win api fires the events we know for
             # sure that no move events will be added in the wrong order, this
             # is kind of hacky, I dont like it too much
@@ -229,7 +221,7 @@
             event = Event(event_raw_data)
             # FIXME: event deduces the pathname wrong and we need to manually
             # set it
-            event.pathname = file_name
+            event.pathname = syncdaemon_path
             # add the event only if we do not have an exclude filter or
             # the exclude filter returns False, that is, the event will not
             # be excluded
@@ -288,14 +280,9 @@
             # add the diff events to the q so that the can be processed no
             # matter the speed.
             for action, file_name in results:
-                file_name = os.path.join(self._path, file_name)
-                # the COM api returns unicode files which although later in
-                # the future is something that we want, atm syncdaemon expects
-                # paths to be a byte sequence encoded with utf8
-                full_filename = get_syncdaemon_valid_path(file_name)
-                self._raw_events_queue.put((full_filename, action))
-                self.log.debug('Added %s to raw events queue.',
-                    (full_filename, action))
+                self._raw_events_queue.put((file_name, action))
+                self.log.debug('Added %r to raw events queue.',
+                    (file_name, action))
 
     def start_watching(self):
         """Tell the watch to start processing events."""
@@ -414,6 +401,7 @@
         self._wd_count += 1
         self.log.debug('Watch count increased to %s', self._wd_count)
 
+    # XXX: Add path validation!!! (nessita)
     def add_watch(self, path, mask, proc_fun=None, auto_add=False,
         quiet=True, exclude_filter=None):
         if hasattr(path, '__iter__'):