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

Revision history for this message
Geoff Teale (tealeg) 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

Done.

>
> [2]
> 303 + def make_disabling(self, suceed):
>
> Small typo, should be succeed.

Done.

>
> [3]
> 324 + self.assertEqual(True, self.mechanism.disable())
>
> Should use assertTrue(self.mechanism.disable()) and assertFalse() in the test
> below.

Done.

« Back to merge proposal