Code review comment for lp:~mandel/ubuntuone-control-panel/auto-update-looping-call

> This style of coding tests is very ugly and error prone:
>
> 534 + def _clean_fake_window_class(self):
> 535 + """Clean the fake window class."""
> 536 + FakeMainWindow.called = []
> 537 + FakeMainWindow.close_callback = None
> 538 +
> 539 + def _clean_fake_icon_class(self):
> 540 + """Clean the fake icon class."""
> 541 + FakeTrayIcon.window = None
> 542 + FakeTrayIcon.auto_update = None
> 543 +
> 544 + def _clean_fake_looping_call_class(self):
> 545 + """Clean the fake looping call class."""
> 546 + FakeLoopingCall.function = None
> 547 + FakeLoopingCall.args = None
> 548 + FakeLoopingCall.kwargs = None
> 549 + FakeLoopingCall.called = []
>
> Instead of patching with a class that gets its attributes overwritten, please
> patch with an instance that gets thrown away, so there's no need to reset the
> class attributes like in the code above.
>
> For instance:
> "self.patch(gui, 'LoopingCall', FakeLoopingCall)" ->
> "self.patch(gui, 'LoopingCall', FakeLoopingCallClass())"
>
> This is present a few times in this branch, so make sure all occurrences end
> up fixed.
>

Fixed them on ubuntuone/controlpanel/gui/qt/tests/test_gui.py since that are the only tests where the class level and did indeed look ugly.

> ---
>
> Also, to test code that uses LoopingCalls (and reactor.callLaters, too)
> remember that you can use twisted.internet.task.Clock, by patching the
> LoopingCall.clock attribute. Then you will be able to fake the reactor moving
> forward in time by calling clock.advance.
>
> This gives you a lot of flexibility to test this kind of code, and the tests
> end up a lot faster and reliable than if they were using a small value for the
> LoopingCall or callLater time.

In my tests I'm asserting that the Looping call was indeed instantiated and started and I trust that the twisted implementation will be correct. I don't think I really need to move the clock to assert that the function was indeed called since the branch this branch depends on already tests that function.

I prefer not to force looping calls to be called since I find those types of tests to be integration tests. I'll leave the tests as they are so we can have a conversation later.