Nice work, a few comments but only about cosmetics. +1! [1] +def check_script_users(config): +def setup_http_proxy(config): +def check_account_name_and_password(config): Please add a docstring to these new functions (and maybe to setup() as well while you're at it). [2] -def register(config, reactor=None): +def register(config, message_handler_f, error_handler_f, reactor=None): We don't usually encode type information in variable names, or at least not with abbreviations. Please consider renaming these parameters to something like "on_finish" and "on_error". Also what do you think of adding print_text and sys.exit as default values for these new parameters? So you get a more user-friendly interface (you don't need to customize what you don't need) and call sites or tests don't need to be modified. [3] +class ObservableRegistration(object): Please add a class docstring for this class and for all its methods. The class docstring should include __init__ parameters to, for example class ObservableRegistration(object): """Meant to do something useful. @param idle_f: Optionally, a function that will be called when... """ [4] + for fun in self._succeed_observers: + fun() + self.do_idle() Please use full names, s/fun/function/ [5] + self.notified = False + self.notified_message = None + self.notified_error = False + + def notify_me(message, error=False): + self.notified = True + self.notified_message = message + self.notified_error = error This is okay, but of course leaves some "attribute garbage" on the test case instance, that in principle could get in the way of other tests, if don't write them carefully. Please consider change it to a version without side effects, like: notified = [] notified_message = [] notified_error = [] def notify_me(message, error=False): notified.append(True) notified_message.append(message) notified_error.append(error) plus self.assertEqual([True], notified) self.assertEqual(notified_message, ["Blimey"]) self.assertEqual(notified_error, [False]) Also, this has the advantage of asserting also how many times notify_me was called, and not only which parameters it was passed. Alternatively, you could go for a Mocker version: def test_notify_observers(self): observable = ObservableRegistration() notify_me = self.mocker.mock() self.expect(notify_me("Blimey", error=False)) self.expect(notify_me("Go lummey!", error=True)) self.mocker.replay() observable.notify_observers("Blimey", error=False) observable.notify_observers("Gor lummey!", error=True) and you can rely on mocker to assert that notify_me was called exactly in that order, with those arguments and that number for times. I just thought I'd mention it, not sure how much I like this myself though :) [6] + def default_machine(self): Please consider renaming this to set_default_computer_title (and maybe other similar methods to). The rationals are that the "set_" prefix makes it more clear what the method does (for example it sets something as opposed to returning it), and computer_title makes it more clear that it's modifying self._computer_title. [7] + self.real_getfqdn = socket.getfqdn + socket.getfqdn = get_fqdn Please instead of monkey patching the module's getfqdn, add a "getfqdn" class attribute to the class defaulting to socket.getfqdn and set: self.controller.getfqdn = get_fqdn in the test setup. You'll also avoid the need of a tearDown.