[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Martin Basti
mbasti at redhat.com
Wed Aug 26 15:53:46 UTC 2015
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
More information about the Freeipa-devel
mailing list