Dave and Jesse,
Thanks again for the code review. Dave, Comments inline.
I have updated the webrev and rerun the tests.
Webrev: https://cr.opensolaris.org/action/browse/caiman/hshaw/7147174_1
Harold
On 03/16/12 12:13, David Marker wrote:
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)
Good idea. Done
2988 can be removed if you make nr_matches a property.
Done.
I believe 3020-3023 is covered later when no match is found at line
3046. So you don't need it here.
I waffled on this one a bit. The reason that it was there was to short
circuit the loop if no candidate disks were found. Since the list will
never be that long and because if does simplify the code I have removed
the check.
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.
That would be better. Done
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
Done.
3106 another extra check. you already raised a Selection error at 3048
Done.
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.
All done.
cheers
-Dave
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
