Code review comment for lp:~diegosarmentero/ubuntuone-windows-installer/close-on-license

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

* I understand we can't patch sys to set frozen to it, but we should add a cleanup call to restore the absence of this attribute. So, this code:

        sys.frozen = True

should be followed by something like:

        self.addCleanup(del, sys.frozen)

* Same for this code, we need to restore the env like it was before the test run:

        if hasattr(sys, "frozen"):
            delattr(sys, "frozen")

should migrate to something like:

        frozen = getattr(sys, "frozen", None)
        if frozen is not None:
            delattr(sys, "frozen")
            self.addCleanup(setattr, sys, 'frozen', frozen)

* Unless I'm missing something, there is no need to define the FakeCaller, isn't? The test case base class inherits from a testcase that has the _set_called method defined, so you can replace:

        self.patch(win32api, "ShellExecute", fake_caller.call)
        self.assertFalse(FakeCaller.was_called)

with:

        self.patch(win32api, "ShellExecute", self._set_called)
        self.assertFalse(self._called)

Also, this code:

        self.patch(os.path, "exists", fake_caller.exists)

can morph into (is easier to read, no need to scroll up to see what does fake_caller is/does):

        self.patch(os.path, "exists", lambda path: True)

Thanks!

review: Needs Fixing

« Back to merge proposal