Code review comment for lp://staging/~tealeg/landscape-client/disable-client-from-ui

Revision history for this message
Mike Milner (milner) wrote :

Looks good! +1

I just have a few issues with names/docs in the tests:

[1]
303 + def make_disabling(self, suceed):
304 +
305 + def _do_disabling(this):
306 + return suceed
307 +
308 + return _do_disabling
309 +

This function (and make_registration just above it) need documentation. The variable names are also very confusing. If these are used as fake callbacks, please use "fake" in their name somewhere so it's clear. Similar issues with tests in landscape/ui/model/registration/tests/test_proxy.py

[2]
303 + def make_disabling(self, suceed):

Small typo, should be succeed.

[3]
324 + self.assertEqual(True, self.mechanism.disable())

Should use assertTrue(self.mechanism.disable()) and assertFalse() in the test below.

review: Approve

« Back to merge proposal