On Mar 9, 2012, at 4:13 PM, Harold Shaw wrote:
> Happy Friday,
>
> Can I get a code review for the following bug:
>
> CR: http://monaco.sfbay/detail.jsf?cr=7147174
>
> There is an additional change that isn't noted in the webrev. I moved line
> 194 in target_selection.py left by one tab. The function,
> __pretty_print_disk, if called with a list of disks would only create a
> string with the first disk in the list. The return statement needed to be
> moved out of the loop.
>
> Webrev: https://cr.opensolaris.org/action/browse/caiman/hshaw/7147174/
target_selection.py:
Over all what you are doing looks right. But I would alter a few things.
class DiskMatches:
should inherit from object
I would make nr_matches a property that just returns
len(self.matches)
2988 can be removed if you make nr_matches a property.
I believe 3020-3023 is covered later when no match is found at line
3046. So you don't need it here.
3038 why not change it to append matched_disk.ctd ?
This would also simplifty lines 3034-3035 as you don't need the
list comprehension.
Just `if matched_disk.ctd not in matched_disk_list:` would be
enough.
3046: instead of checking here, you can take advantage of python's
for/else:
for candidate_disk in candidate_disk_list:
…
else:
# you are guaranteed you didn't break out above so
don't need to check
# if candidate_disk.mapped_disk is None
3106 another extra check. you already raised a Selection error at 3048
test_target_selection_sparc.py:
test_target_selection_props_1:
OK so you are testing for the property FIXED.
In order to make expected_xml a little more resilient (and
since you don't care about size) I would replace the dev_size with a regex.
That is line 1528:
dev_size="\d+secs"/>
Similarly on 1531 I would change it to:
......<size val="\d+" start_sector="\d+"/>
If we were testing sizes I would leave them in, but this way if
somebody changes c99t2d0 in DISCOVERED_TARGETS_XML you tests will still work.
Also, mostly its ShadowLists job to test sizing things.
test_target_selection_props_2_1:
same, you are testing the dev_vendor not a size
test_target_selection_props_2_2,
test_target_selection_props_2_with_pools,
test_target_selection_props_3_2,
test_target_selection_props_3_3:
you do specify a size for one of the disks, so you can leave
its size in. but the other should also move to regex.
test_target_selection_x86.py:
in general same comments as the sparc version. rip out as many sector
sizes as you can.
cheers
-Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
