Code review comment for lp://qastaging/~mabac/linaro-image-tools/image-support-for-android

On Fri, 2011-06-10 at 08:31 +0000, Mattias Backman wrote:
> > Hi Mattias,
> >
> > They look good to me; I've just a couple suggestions.
> >
> > I'd use zip(seq1, seq2) instead of map(None, ...) as it feels more natural.
>
> I did consider zip, but it will only iterate over the number of items
> in the shortest list if the lists are not of equal length. So if the
> list of expected offsets and sizes would miss the values for the last
> partition the test would pass using zip. I guess that could happen by
> forgetting to add data for a new partition or just accidentally
> removing the last pair.

Oh, good catch. It'd be nice to have a comment stating why we've used
map() so that people don't inadvertently change it to use zip().

>
> With the admittedly uglier map(None, ...) the test will fail like
> this:
> AssertionError: Match failed. Matchee: "None"
> Matcher: Equals((1212170240L, 935313408L))
> Difference: (1212170240L, 935313408L) != None
>
> Perhaps not very likely to be an issue but at least in theory the test
> would be a little more robust, I think.

Yeah, if there's any reason to use map() instead of zip() then by all
means do it. :)

>
> > Also, it'd be nice to have a class variable with the partition sizes as a
> > function of the sector numbers used in _create_android_tmpfile(), e.g:
> >
> > sizes_and_offsets = [
> > (63 * 512, 270272 * 512),
> > (270336 * 512, ...),
> > ...]
> >
> > That way if we ever need to change them in _create_android_tmpfile() we can
> > easily do a search-and-replace to change them here as well. And by having this
> > as a class variable we could use it in test_get_android_loopback_devices() as
> > well, to construct the list of expected commands:
> >
> > expected = [
> > '%s losetup -f --show %s --offset %s --sizelimit %s' % (sudo_args,
> > tmpfile, size, offset)
> > for (size, offset) in sizes_and_offsets]
>
> You're right that it made the tests a lot less cluttered and probably easier to maintain. I might do something similar for the non android functions too.

Cool, that's highly appreciated!