[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Martin Basti
mbasti at redhat.com
Fri Aug 28 14:13:19 UTC 2015
On 08/27/2015 07:21 PM, Oleg Fayans wrote:
> Hi Martin,
>
> My bad, forgot to do git add.
>
>
> On 08/27/2015 06:27 PM, Martin Basti wrote:
>>
>>
>> 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.
>
ACK
Pushed to master: c7408f67f6cff7bd3ac14b7661472898f8e7dd73
More information about the Freeipa-devel
mailing list