Code review comment for lp:~rvb/maas-test/import-images-api

> Thanks for fixing this — and let's not forget, diagnosing it!
>
> Notes as usual:
>
> Maybe in the docstring for wait_until_boot_images_scanned, say “until the
> master cluster has reported boot images” (so as not to give the impression
> that we just wait for them to appear in /var/lib/maas)?

Right, done.

> Any particular reason why wait_until_boot_images_scanned returns the number of
> images? It seems a bit arbitrary (why not the actual list, for instance?) and
> introduces testing for something that's not actually used. Why not just
> return nothing, and document the fact that either a timeout or a failure to
> request the listing will raise an exception?

Okay, done.

> Also in wait_until_boot_images_scanned, it turns out that RuntimeError isn't
> really for this kind of error, but rather for errors originating in the Python
> runtime. And, it's deprecated anyway. Better derive an ad-hoc exception
> class from Exception.
>
> The exception's error message says “cluster” instead of “the cluster,” which
> makes it look a little weird to me. But maybe it would be more helpful to say
> e.g. “Boot image download timed out: cluster reported no images”?

Done.

> In test_maasfixture.py, I wonder if it would make sense to make the patch for
> wait_until_boot_images_scanned a standard part of make_maas_fixture, with an
> option to skip it?

I'd rather not do that, I know all the setup steps look a bit redundant but I'd rather keep the setup explicit than mess things up with helpers that I fear will soon become spaghetti code.

> What's going on with the separate make_kvm_fixture steps? AFAIK you don't
> need to do that — just call make_maas_fixture and it will create a KVMFixture
> for you. In test_list_boot_images_lists_images there's even a commented-out
> line that did it that way. Is there a difference? Or was this a transient
> change during development that is no longer needed?

True, fixed.

> Setup gets a little lengthy in test_list_boot_images_lists_images. Might it
> make sense to extract this bit into a helper?
>
> uuid = self.getUniqueString()
> self.patch(
> maas_fixture, 'get_master_ng_uuid',
> mock.MagicMock(return_value=uuid))

Again, I don't think it's worth it.

Thanks for the review!