[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Oleg Fayans
ofayans at redhat.com
Thu Aug 27 17:21:03 UTC 2015
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.
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0002.10-Integration-tests-topology-plugin.patch
Type: text/x-patch
Size: 16901 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/90bee45e/attachment.bin>
More information about the Freeipa-devel
mailing list