[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin

[Oleg Fayans]

Hi Martin,

As discussed, here is a new version with pep8-related fixes


On 08/14/2015 10:44 AM, Oleg Fayans wrote:
> Hi Martin,
>
> Already noticed that. Implemented the named groups as Tomas advised.
> Added the third test for
> http://www.freeipa.org/page/V4/Manage_replication_topology/Test_plan#Test_case:_Removal_of_a_topology_segment_is_allowed_only_if_there_is_at_least_one_more_segment_connecting_the_given_replica
>
>
>
>
> On 08/13/2015 05:06 PM, Martin Basti wrote:
>>
>>
>> On 08/11/2015 03:36 PM, Oleg Fayans wrote:
>>> Hi Martin,
>>>
>>> On 08/11/2015 02:02 PM, Martin Basti wrote:
>>>> NACK, comments inline.
>>>>
>>>> On 11/08/15 13:25, Oleg Fayans wrote:
>>>>> Hi Martin,
>>>>>
>>>>> Thanks for the review!
>>>>>
>>>>> On 08/10/2015 07:08 PM, Martin Basti wrote:
>>>>>> Thank you for patch, I have a few nitpicks:
>>>>>>
>>>>>> 1)
>>>>>> On 10/08/15 13:05, Oleg Fayans wrote:
>>>>>>> +def create_segment(master, leftnode, rightnode):
>>>>>>> +    """create_segment(master, leftnode, rightnode)
>>>>>> Why do you add the name of method in docstring?
>>>>> My bad, fixed.
>>>>
>>>> still there
>>>>
>>>> +        tokenize_topologies(command_output)
>>>> +        takes an output of `ipa topologysegment-find` and returns an
>>>> array of
>>>>
>>> Fixed, sorry.
>>>>
>>>>>>
>>>>>>
>>>>>> 2)
>>>>>>
>>>>>> +def create_segment(master, leftnode, rightnode):
>>>>>> +    """create_segment(master, leftnode, rightnode)
>>>>>> +    creates a topology segment. The first argument is a node to
>>>>>> run the
>>>>>> command on
>>>>>> +    The first 3 arguments should be objects of class Host
>>>>>> +    Returns a hash object containing segment's name, leftnode,
>>>>>> rightnode information
>>>>>> +    """
>>>>>>
>>>>>> I would prefer to add assert there instead of just document that a
>>>>>> Host
>>>>>> object is needed
>>>>>> assert(isinstance(master, Host))
>>>>>> ...
>>>>>
>>>>> Fixed. Created a decorator that checks the type of arguments
>>>>
>>>> This does not scale well.
>>>> If we will want to add new argument that is not host object, you need
>>>> change it again.
>>>
>>> Agreed. Modified the decorator so that you can specify a slice of
>>> arguments to be checked and a class to compare to. This does scale :)
>>>>
>>>> This might be used as function with specified variables that have to be
>>>> host objects
>>>>>
>>>>>>
>>>>>> 3)
>>>>>> +def destroy_segment(master, segment_name):
>>>>>> +    """
>>>>>> +    destroy_segment(master, segment_name)
>>>>>> +    Destroys topology segment. First argument should be object of
>>>>>> class
>>>>>> Host
>>>>>>
>>>>>> Instead of description of params as first, second etc., you may use
>>>>>> following:
>>>>>>
>>>>>> +def destroy_segment(master, segment_name):
>>>>>> +    """
>>>>>> +    Destroys topology segment.
>>>>>> +    :param master: reference to master object of class Host
>>>>>> +    :param segment: name fo segment
>>>>>> and eventually this in other methods
>>>>>> +    :returns: Lorem ipsum sit dolor mit amet
>>>>>> +    :raises NotFound: if segment is not found
>>>>>>
>>>>> Fixed
>>>>>>
>>>>>> 4)
>>>>>>
>>>>>> cls.replicas[:len(cls.replicas) - 1],
>>>>>>
>>>>>> I suggest cls.replicas[:-1]
>>>>>>
>>>>>> In [2]: a = [1, 2, 3, 4, 5]
>>>>>>
>>>>>> In [3]: a[:-1]
>>>>>> Out[3]: [1, 2, 3, 4]
>>>>>>
>>>>> Fixed
>>>>>>
>>>>>> 5)
>>>>>> Why re.findall() and then you just use the first result?
>>>>>> 'leftnode': self.leftnode_re.findall(i)[0]
>>>>>>
>>>>>> Isn't just re.search() enough?
>>>>>> leftnode_re.search(value).group(1)
>>>>>
>>>>> in fact
>>>>> leftnode_re.findall(string)[0]
>>>>> and
>>>>> leftnode_re.search(string).group(1),
>>>>> Are equally bad from the readability point of view. The first one is
>>>>> even shorter a bit, so why change? :)
>>>>
>>>> It depends on point of view,  because when I reviewed it yesterday my
>>>> brain raises exception that you are trying to add multiple values to
>>>> single value attribute, because you used findall, I expected that you
>>>> need multiple values, and then I saw that index [0] at the end, and I
>>>> was pretty confused what are you trying to achieve.
>>>>
>>>> And because findall is not effective in case when you need to find just
>>>> one occurrence.
>>>
>>> I got it. Fixed.
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> Python 3 nitpick:
>>>>>> I'm not sure if time when we should enforce python 2/3 compability
>>>>>> already comes, but just for record:
>>>>>> instead of open(file, 'r'), please use io.open(file, 'r') (import io
>>>>>> before)
>>>>>>
>>>>> Done.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>> 1)
>>>>
>>>> +#
>>>>
>>>> empty comment here (several times)
>>>
>>> Removed
>>>>
>>>
>>
>> NACK
>>
>> you changed it wrong
>>
>> group() returns everything, you need use group(1) to return content in
>> braces.
>
>
>

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0002.6-Integration-tests-topology-plugin.patch
Type: text/x-patch
Size: 16724 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150819/05c07725/attachment.bin>