Code review comment for lp:~diegosarmentero/ubuntuone-client/fix-user-home-tests

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

* Though FakeMainTestCase used to use self.tmpdir as homke dir, it is more correct to use a dedicated dir for that, so we can isolate home data into a dir (we may need that to debug a test).

Can you please make self.home_dir = self.mktemp('ubuntuonehacker')?

* Also, in tests/syncdaemon/test_action_queue.py you're removing the patch os.environ['HOME'] = self.home, which is great, but the self.home variable should point to the same home that the base test case is using to patch the xdg_home var. So, I would guess that in BasicTestCase we need to change:

        self.home = self.mktemp('home')

for

        self.home = self.home_dir

(please confirm this makes sense before changing it, I did not reviewed the whole test module).

* There are several tests that are re-defining self.home_dir and re-patching xdg_home... can you please remove that? (we need to confirm they all inherit from BaseTwistedTestCase either directly or by some inheritance chain). A quick grep gives me:

nessita@dali:~/canonical/client/review_fix-user-home-tests$ grep -rn --color "patch.*xdg_home" *
contrib/testing/testcase.py:383: self.patch(platform, "xdg_home", self.home_dir)
tests/syncdaemon/test_localrescan.py:134: self.patch(platform, 'xdg_home', self.home_dir)
tests/syncdaemon/test_localrescan.py:374: self.patch(platform, 'xdg_home', self.home_dir)
tests/syncdaemon/test_localrescan.py:2021: self.patch(platform, 'xdg_home', self.home_dir)
tests/syncdaemon/test_localrescan.py:2260: self.patch(platform, 'xdg_home', self.home_dir)
tests/syncdaemon/test_config.py:534: self.patch(platform, 'xdg_home', homedir)
tests/syncdaemon/test_main.py:267: self.patch(main_mod, "xdg_home", self.home_dir)
tests/syncdaemon/test_eq_inotify.py:664: self.patch(platform, 'xdg_home', self.home_dir)
tests/platform/test_os_helper.py:95: self.patch(platform, "xdg_home", self.my_home)
nessita@dali:~/canonical/client/review_fix-user-home-tests$

nessita@dali:~/canonical/client/review_fix-user-home-tests$ grep -rn --color "self\.home_dir =" *
contrib/testing/testcase.py:382: self.home_dir = self.tmpdir
tests/syncdaemon/test_vm.py:3739: self.home_dir = self.mktemp('ubuntuonehacker')
tests/syncdaemon/test_localrescan.py:126: self.home_dir = self.mktemp('ubuntuonehacker')
tests/syncdaemon/test_eventqueue.py:46: self.home_dir = self.mktemp('home_dir')
tests/syncdaemon/test_eq_inotify.py:649: self.home_dir = self.mktemp('ubuntuonehacker')
tests/platform/linux/test_vm.py:464: self.home_dir = os.path.join(self.tmpdir, 'home', 'ubuntuonehacker')
tests/platform/linux/test_filesystem_notifications.py:102: self.home_dir = self.mktemp('home_dir')
tests/platform/test_filesystem_notifications.py:102: self.home_dir = self.mktemp('home_dir')
nessita@dali:~/canonical/client/review_fix-user-home-tests$

review: Needs Fixing

« Back to merge proposal