Code review comment for lp:~rvb/maas-test/display-maas-version

Revision history for this message
Raphaël Badin (rvb) wrote :

 > Because the mock is returned from patch(), you can avoid importing
> MagickMock and just do:
>
>[...]

Well, actually, the mock object is *not* returned by patch(…). In MAAS it is, see src/maastesting/testcase.py:MAASTestCase.patch. But in maas-test, we don't have that utility

>
> > + maas_fixture.install_maas() + + self.assertEqual( +
> > ( + [mock.call()], + [ +
> > mock.call("Installing MAAS (version %s)..." % version), +
> > mock.call("Done installing MAAS."), + ]), +
> > (mock_get_maas_version.mock_calls, mock_logging.mock_calls))
>
> I think this is definitely a case where having more than one assertion
> is better than compounding them into a single call. This is extremely
> hard to read and digest. And ease of reading trumps everything else!

All right, you know I'm a sucker for compound assertions but in this instance, you're definitely right.

> [...]
> You could make this easier to read by assigning a variable to the
> result of mock.call() and using that in the assert_has_call.

Actually, I've refactored that code a bit and it's now a bit simpler.

Thanks for the review!

« Back to merge proposal