maas-test

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

import-images-api
Merge into trunk

Revision history for this message

Jeroen T. Vermeulen (jtv) wrote on 2014-04-09:

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)?

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?

By the way, the initial assignment for nb_images looks dead to me.

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”?

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?

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?

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))

Thanks for fixing this — and let's not forget, diagnosing it!

Notes as usual:

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?

By the way, the initial assignment for nb_images looks dead to me.

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”?

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?

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?

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))

review: Approve

« Back to merge proposal