[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Martin Basti
mbasti at redhat.com
Thu Aug 27 16:27:38 UTC 2015
On 08/27/2015 05:41 PM, Oleg Fayans wrote:
> Hi,
>
> I am sorry I have missed that.
> Fixed. The test fails now due to this bug:
>
> https://fedorahosted.org/freeipa/ticket/5222
>
> The test output is attached together with the updated patch
>
> On 08/26/2015 05:53 PM, Martin Basti wrote:
>>
>>
>> On 08/26/2015 05:42 PM, Martin Basti wrote:
>>>
>>>
>>> On 08/26/2015 02:53 PM, Oleg Fayans wrote:
>>>> Hi,
>>>>
>>>> No more short links :)
>>>>
>>>> On 08/26/2015 11:50 AM, Tomas Babej wrote:
>>>>>
>>>>>
>>>>> On 08/26/2015 11:44 AM, Oleg Fayans wrote:
>>>>>> Hi Martin,
>>>>>>
>>>>>> On 08/20/2015 11:18 AM, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 08/20/2015 10:26 AM, Martin Basti wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 08/19/2015 04:17 PM, Martin Basti wrote:
>>>>>>>>> I got this:
>>>>>>>>>
>>>>>>>>> https://paste.fedoraproject.org/256746/43999380/
>>>>>>>> FYI replica install failure. (I will retest it, but I'm pretty
>>>>>>>> sure
>>>>>>>> that it was clean VM, test for some reason install client first)
>>>>>>>>
>>>>>>>> File
>>>>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> line 295, in decorated
>>>>>>>> func(installer)
>>>>>>>> File
>>>>>>>> "/usr/lib/python2.7/site-packages/ipaserver/install/server/replicainstall.py",
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> line 319, in install_check
>>>>>>>> sys.exit("IPA client is already configured on this system.\n"
>>>>>>>>
>>>>>>>> 2015-08-19T14:14:15Z DEBUG The ipa-replica-install command failed,
>>>>>>>> exception: SystemExit: IPA client is already configured on this
>>>>>>>> system.
>>>>>>>> Please uninstall it first before configuring the replica, using
>>>>>>>> 'ipa-client-install --uninstall'.
>>>>>>>> 2015-08-19T14:14:15Z ERROR IPA client is already configured on
>>>>>>>> this
>>>>>>>> system.
>>>>>>>> Please uninstall it first before configuring the replica, using
>>>>>>>> 'ipa-client-install --uninstall'.
>>>>>>> Confirm I got same error.
>>>>>> Fixed. It was a regex error.
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 08/19/2015 09:00 AM, Oleg Fayans wrote:
>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Please, do not use third-party URL shorteners from within our source
>>>>> code.
>>>>>
>>>>> Tomas
>>>>>
>>>>
>>>
>>> I received 2 errors
>>>
>>> ___________________________________________________________________________________
>>>
>>> TestTopologyOptions.test_add_remove_segment
>>> ___________________________________________________________________________________
>>>
>>>
>>>
>>> self = <ipatests.test_integration.test_topology.TestTopologyOptions
>>> object at 0x7f6ab95b3110>
>>>
>>> def test_add_remove_segment(self):
>>> """
>>> Make sure a topology segment can be manually created and
>>> deleted
>>> with the influence on the real topology
>>> Testcase
>>> http://www.freeipa.org/page/V4/Manage_replication_topology/
>>> Test_plan#Test_case:_Basic_CRUD_test
>>> """
>>> tasks.kinit_admin(self.master)
>>> # Install the second replica
>>> tasks.install_replica(self.master, self.replicas[1],
>>> setup_ca=False,
>>> setup_dns=False)
>>> # turn a star into a ring
>>> segment, err = tasks.create_segment(self.master,
>>> self.replicas[0],
>>> > self.replicas[1])
>>> E TypeError: 'NoneType' object is not iterable
>>>
>>> test_integration/test_topology.py:102: TypeError
>>>
>>> Maybe tasks.create_segment returns None?
>>> In [3]: a, b = None
>>> ---------------------------------------------------------------------------
>>>
>>>
>>> TypeError Traceback (most recent call
>>> last)
>>> <ipython-input-3-c020227e755d> in <module>()
>>> ----> 1 a, b = None
>>>
>>> TypeError: 'NoneType' object is not iterable
>>>
>>>
>>> --------------
>>>
>>> self = <pytest_multihost.transport.SSHCommand object at
>>> 0x7f399b9ea710>, raiseonerr = True
>>>
>>> def wait(self, raiseonerr=True):
>>> """Wait for the remote process to exit
>>>
>>> Raises an excption if the exit code is not 0, unless
>>> raiseonerr is
>>> true.
>>> """
>>> if self._done:
>>> return self.returncode
>>>
>>> self._end_process()
>>>
>>> self._done = True
>>>
>>> if raiseonerr and self.returncode:
>>> self.log.error('Exit code: %s', self.returncode)
>>> > raise subprocess.CalledProcessError(self.returncode,
>>> self.argv)
>>> E CalledProcessError: Command '['ipa',
>>> 'topologysegment-del', 'realm',
>>> 'vm-152.abc.idm.lab.eng.brq.redhat.com-to-vm-160.abc.idm.lab.eng.brq.redhat.com']'
>>>
>>> returned non-zero exit status 2
>>>
>> Honza found where the problem is.
>>
>> def check_arguments_are(slice, instanceof):
>> ...
>> def wrapper(func):
>> def wrapped(*args):
>> for i in args[slice[0]:slice[1]]:
>> assert isinstance(i, instanceof), "Wrong type: %s: %s"
>> % (i, type(i))
>> func(*args)
>> return wrapped
>> return wrapper
>>
>> there should be
>> return func(*args)
>>
>> otherwise None will be always returned
>>
>
It looks nice, but I miss the integration test in the patch.
More information about the Freeipa-devel
mailing list