Merge lp:~nataliabidart/ubuntuone-client/remove-watches-even-on-rename into lp:ubuntuone-client

[Natalia Bidart]

Watches are remove after UDF subscription no matter if paths exist on disk or not.

[Natalia Bidart (nataliabidart)]

Watches are remove after UDF subscription no matter if paths exist on disk or not.

Fixed test about UDF unsubscription after ancestor path was messed up.

[dobey (dobey)]

review: Approve

[Facundo Batista (facundo)]

===============================================================================
[ERROR]: tests.syncdaemon.test_eq_inotify.AncestorsUDFTestCase.test_file_events_are_ignored_on_udf_ancestor

Traceback (most recent call last):
Failure: twisted.python.failure.DefaultException:
===============================================================================
[ERROR]: tests.syncdaemon.test_eq_inotify.AncestorsUDFTestCase.test_file_events_are_ignored_on_udf_ancestor

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/internet/base.py", line 757, in runUntilCurrent
    call.func(*call.args, **call.kw)
  File "/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/tests/syncdaemon/test_eq_inotify.py", line 1166, in check
    self.assertEquals([], self.listener.events)
  File "/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/tests/syncdaemon/test_eq_inotify.py", line 1140, in failUnlessEqual
    raise exception
twisted.trial.unittest.FailTest: not equal:
a = []
b = [('VM_UDF_CREATED', (<ubuntuone.syncdaemon.volume_manager.UDF object at 0xa1f8f6c>,), {})]

===============================================================================
[ERROR]: tests.syncdaemon.test_eq_inotify.AncestorsUDFTestCase.test_file_events_are_not_ignored_on_common_prefix_name

Traceback (most recent call last):
Failure: twisted.python.failure.DefaultException:
===============================================================================
[ERROR]: tests.syncdaemon.test_eq_inotify.AncestorsUDFTestCase.test_file_events_are_not_ignored_on_common_prefix_name

Traceback (most recent call last):
  File "/usr/lib/python2.6/dist-packages/twisted/internet/base.py", line 757, in runUntilCurrent
    call.func(*call.args, **call.kw)
  File "/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/tests/syncdaemon/test_eq_inotify.py", line 1245, in check
    self.assertEquals(expected, self.listener.events)
  File "/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/tests/syncdaemon/test_eq_inotify.py", line 1140, in failUnlessEqual
    raise exception
twisted.trial.unittest.FailTest: not equal:
a = [('FS_FILE_CREATE', ('/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/_trial_temp/tmp/tests.syncdaemon.test_eq_inotify/AncestorsUDFTestCase/test_file_events_are_not_ignored/tmpdir/ubuntuonehacker/Documents/Reading/Book/testit',), {}), ('FS_FILE_OPEN', ('/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/_trial_temp/tmp/tests.syncdaemon.test_eq_inotify/AncestorsUDFTestCase/test_file_events_are_not_ignored/tmpdir/ubuntuonehacker/Documents/Reading/Book/testit',), {}), ('FS_FILE_CLOSE_WRITE', ('/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/_trial_temp/tmp/tests.syncdaemon.test_eq_inotify/AncestorsUDFTestCase/test_file_events_are_not_ignored/tmpdir/ubuntuonehacker/Documents/Reading/Book/testit',), {}), ('FS_FILE_OPEN', ('/home/facundo/canonical/u1-client/review_remove-watches-even-on-rename/_trial_temp/tmp/tests.syncdaemon.test_eq_inotify/AncestorsUDFTestCase/test_file_events_are_not_ignored/tmpdir/ubuntuonehacker/Documents/Reading/...

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

> FAILED (errors=8, successes=43)

Tests are fixed now. Thanks!

[Facundo Batista (facundo)]

It's ok!

review: Approve

[dobey (dobey)]

Attempt to merge lp:~nataliabidart/ubuntuone-client/remove-watches-even-on-rename into lp:ubuntuone-client failed due to merge conflicts:

text conflict in tests/syncdaemon/test_eq_inotify.py

[Natalia Bidart (nataliabidart)]

> Attempt to merge lp:~nataliabidart/ubuntuone-client/remove-watches-even-on-
> rename into lp:ubuntuone-client failed due to merge conflicts:
>
> text conflict in tests/syncdaemon/test_eq_inotify.py

Conflicts resolved.

[Guillermo Gonzalez (verterok)]

3 approve stamp!

review: Approve

=== modified file 'tests/syncdaemon/test_eq_inotify.py'
--- tests/syncdaemon/test_eq_inotify.py	2010-01-26 15:57:39 +0000
+++ tests/syncdaemon/test_eq_inotify.py	2010-01-26 19:17:11 +0000
@@ -20,6 +20,7 @@
 
 import functools
 import os
+import shutil
 import unittest
 
 from twisted.internet import defer, reactor
@@ -1088,21 +1089,27 @@
         return self._deferred
 
 
-class AncestorsUDFTestCase(BaseTwisted):
+class AncestorsUDFTestCase(testcase.BaseTwistedTestCase):
     """Events over UDF's ancestor are properly handled."""
 
+    timeout = 2
+
     def setUp(self):
         """Init."""
-        result = BaseTwisted.setUp(self)
+        testcase.BaseTwistedTestCase.setUp(self)
+        self._deferred = defer.Deferred()
         self.home_dir = self.mktemp('ubuntuonehacker')
-
-
+        self.root_dir = self.mktemp('root_dir')
         self.data_dir = self.mktemp('data_dir')
         self.shares_dir = self.mktemp('shares_dir')
         self.partials_dir = self.mktemp('partials_dir')
         self.main = testcase.FakeMain(self.root_dir, self.shares_dir,
                              self.data_dir, self.partials_dir)
-        ##self.eq.fs.vm = self.main.vm
+        self.eq = self.main.event_q
+        assert self.eq is self.eq.fs.vm.m.event_q
+
+        self.listener = testcase.Listener()
+        self.eq.subscribe(self.listener)
 
         self.env_var = 'HOME'
         self.old_value = os.environ.get(self.env_var, None)
@@ -1116,31 +1123,24 @@
                                       suggested_path, path, True)
         self.ancestors = self.udf.ancestors # this requires a fake HOME
         os.makedirs(path)
-        self.eq.fs.vm.add_udf(self.udf)
-        assert self.udf.volume_id in self.eq.fs.vm.udfs
+        d = self.eq.fs.vm.add_udf(self.udf)
 
         # every ancestor has a watch already, added by LocalRescan. Copy that.
         self.eq.inotify_add_watch(self.udf.path)
         for path in self.ancestors:
             self.eq.inotify_add_watch(path)
 
-        self.listener = testcase.Listener()
-        self.eq.subscribe(self.listener)
-
-        return result
+        # reset events up to now
+        d.addCallback(lambda _: setattr(self.listener, 'events', []))
+        return d
 
     def tearDown(self):
         """Cleanup."""
         self.eq.unsubscribe(self.listener)
-
-        ##self.eq.fs.vm._remove_watches(self.home_dir)
-        for path in self.ancestors + [self.udf.path]:
-            if os.path.exists(path):
-                self.eq.inotify_rm_watch(path)
-
         self.main.shutdown()
 
         self.rmtree(self.home_dir)
+        self.rmtree(self.root_dir)
         self.rmtree(self.data_dir)
         self.rmtree(self.shares_dir)
         self.rmtree(self.partials_dir)
@@ -1150,7 +1150,27 @@
         else:
             os.environ[self.env_var] = self.old_value
 
-        BaseTwisted.tearDown(self)
+        testcase.BaseTwistedTestCase.tearDown(self)
+
+    def failUnlessEqual(self, first, second, msg=''):
+        """Fail the test if C{first} and C{second} are not equal.
+
+        @param msg: A string describing the failure that's included in the
+            exception.
+
+        """
+        if not first == second:
+            if msg is None:
+                msg = ''
+            if len(msg) > 0:
+                msg += '\n'
+            exception = self.failureException(
+                '%snot equal:\na = %s\nb = %s\n'
+                % (msg, repr(first), repr(second)))
+            self._deferred.errback(msg)
+            raise exception
+        return first
+    assertEqual = assertEquals = failUnlessEquals = failUnlessEqual
 
     def test_file_events_are_ignored_on_udf_ancestor(self):
         """Events on UDF ancestors are ignored."""
@@ -1175,7 +1195,7 @@
         def check():
             """Check."""
             self.assertEquals([], self.listener.events)
-            self.finished_ok()
+            self._deferred.callback(True)
 
         reactor.callLater(.1, check)
         return self._deferred
@@ -1212,7 +1232,7 @@
         def check():
             """Check."""
             self.assertEquals(expected, self.listener.events)
-            self.finished_ok()
+            self._deferred.callback(True)
 
         reactor.callLater(.1, check)
         return self._deferred
@@ -1230,20 +1250,19 @@
         assert not self.eq._processor._is_udf_ancestor(path)
 
         os.makedirs(path)
-        self.eq.fs.vm.add_udf(other_udf)
-
         # every ancestor has a watch already, added by LocalRescan. Copy that.
         self.eq.inotify_add_watch(other_udf.path)
         for path in other_ancestors:
             self.eq.inotify_add_watch(path)
 
         fname = os.path.join(other_udf.path, 'testit')
-        # generate FS_FILE_CREATE, FS_FILE_OPEN, FS_FILE_CLOSE_WRITE
-        open(fname, 'w').close()
+        def modify(_):
+            # generate FS_FILE_CREATE, FS_FILE_OPEN, FS_FILE_CLOSE_WRITE
+            open(fname, 'w').close()
 
-        # generate FS_FILE_CLOSE_NOWRITE
-        with open(fname) as f:
-            f.read()
+            # generate FS_FILE_CLOSE_NOWRITE
+            with open(fname) as f:
+                f.read()
 
         expected = [('FS_FILE_CREATE', (fname,), {}),
                     ('FS_FILE_OPEN', (fname,), {}),
@@ -1251,10 +1270,15 @@
                     ('FS_FILE_OPEN', (fname,), {}),
                     ('FS_FILE_CLOSE_NOWRITE', (fname,), {})]
 
+        d = self.eq.fs.vm.add_udf(other_udf)
+        # reset events up to now
+        d.addCallback(lambda _: setattr(self.listener, 'events', []))
+        d.addCallback(modify)
+
         def check():
             """Check."""
             self.assertEquals(expected, self.listener.events)
-            self.finished_ok()
+            self._deferred.callback(True)
 
         reactor.callLater(.1, check)
         return self._deferred
@@ -1263,30 +1287,30 @@
         """UDF is unsubscribed on ancestor move."""
         original = self.eq.fs.vm.unsubscribe_udf
         expected = []
-        # pylint: disable-msg=W0108
-        self.eq.fs.vm.unsubscribe_udf = lambda uid: expected.append(uid)
-
-        def restore():
-            """Restore original unsubscribe_udf."""
-            self.eq.fs.vm.unsubscribe_udf = original
-        self.addCleanup(restore)
-
         path = self.ancestors[-1] # any ancestor
         # generate IN_MOVED_FROM and IN_MOVED_TO
         newpath = path + u'.old'
         os.rename(path, newpath)
-        # generate FS_DIR_DELETE over the former UDFs
-        # those have to be ignored!
-        # HUGE FIXME: this generates event on the new, moved path
-        ##self.rmtree(newpath)
+        assert os.path.exists(newpath)
 
-        def check():
+        def check(uid):
             """Check."""
-            self.assertEquals([], self.listener.events)
-            self.assertEquals(expected, [self.udf.volume_id], 'udf unsubscribed')
-            self.finished_ok()
-
-        reactor.callLater(.1, check)
+            original(uid)
+            self.assertEquals(uid, self.udf.id, 'udf unsubscribed')
+            self.assertEquals(False, self.eq.inotify_has_watch(self.udf.path),
+                             'watch removed')
+            self.assertEquals(False, self.eq.fs.vm.udfs[self.udf.id].subscribed)
+            self._deferred.callback(True)
+
+        # pylint: disable-msg=W0108
+        self.eq.fs.vm.unsubscribe_udf = check
+
+        def restore():
+            """Restore original unsubscribe_udf."""
+            self.eq.fs.vm.unsubscribe_udf = original
+            shutil.rmtree(newpath)
+        self.addCleanup(restore)
+
         return self._deferred
 
     def test_remove_udf_per_se(self):
@@ -1309,7 +1333,7 @@
             """Check."""
             self.assertEquals([], self.listener.events)
             self.assertEquals(expected, [uid], 'udf deleted')
-            self.finished_ok()
+            self._deferred.callback(True)
 
         reactor.callLater(.1, check)
         return self._deferred

=== modified file 'tests/syncdaemon/test_vm.py'
--- tests/syncdaemon/test_vm.py	2010-01-25 23:15:23 +0000
+++ tests/syncdaemon/test_vm.py	2010-01-26 19:17:10 +0000
@@ -448,6 +448,21 @@
         for path in paths:
             self.assertFalse(self.main.event_q.inotify_has_watch(path), path)
 
+    def test_remove_watches_after_dir_rename(self):
+        """Test for VolumeManager._remove_watches after dir rename."""
+        path = os.path.join(self.root_dir, 'testit')
+        os.mkdir(path)
+        self.main.fs.create(path, "", is_dir=True)
+        self.main.fs.set_node_id(path, 'dir_node_id')
+        self.main.event_q.inotify_add_watch(path)
+
+        os.rename(path, path+'.old')
+        # remove the watches
+        self.vm._remove_watches(self.root_dir)
+
+        self.assertFalse(self.main.event_q.inotify_has_watch(path),
+                         'watch should noy be present')
+
     def test_delete_fsm_object(self):
         """Test for VolumeManager._delete_fsm_object"""
         path = os.path.join(self.root_dir, 'dir')

=== modified file 'ubuntuone/syncdaemon/volume_manager.py'
--- ubuntuone/syncdaemon/volume_manager.py	2010-01-26 19:11:29 +0000
+++ ubuntuone/syncdaemon/volume_manager.py	2010-01-26 19:17:11 +0000
@@ -573,7 +573,7 @@
     def _remove_watches(self, path):
         """Remove the inotify watches from path and it subdirs."""
         for a_path, is_dir in self.m.fs.get_paths_starting_with(path):
-            if is_dir and os.path.exists(a_path):
+            if is_dir:
                 self._remove_watch(a_path)
 
     def create_share(self, path, username, name, access_level):