[Freeipa-devel] [PATCH] First part of integration tests for Topology Plugin
Oleg Fayans
ofayans at redhat.com
Thu Aug 27 15:41:00 UTC 2015
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
>
--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
============================= test session starts ==============================
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: multihost, sourceorder
collected 3 items
test_integration/test_topology.py .FF
=================================== FAILURES ===================================
_________________ TestTopologyOptions.test_add_remove_segment __________________
self = <ipatests.test_integration.test_topology.TestTopologyOptions object at 0x7f6980ffb610>
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])
> assert err == "", err
E AssertionError: ipa: ERROR: Could not get Connectivity interactively
E
E assert 'ipa: ERROR: ...teractively\n' == ''
E - ipa: ERROR: Could not get Connectivity interactively
test_integration/test_topology.py:103: AssertionError
_____________ TestTopologyOptions.test_remove_the_only_connection ______________
self = <ipatests.test_integration.test_topology.TestTopologyOptions object at 0x7f6981009d10>
def test_remove_the_only_connection(self):
"""
Testcase: 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
"""
text = "Removal of Segment disconnects topology"
error1 = "The system should not have let you remove the segment"
error2 = "Wrong error message thrown during segment removal: \"%s\""
replicas = (self.replicas[0].hostname, self.replicas[1].hostname)
returncode, error = tasks.destroy_segment(self.master, "%s-to-%s" % replicas)
assert returncode != 0, error1
> assert error.count(text) == 1, error2 % error
E AssertionError: Wrong error message thrown during segment removal: "ipa: ERROR: f22replica1.pesen.net-to-f22replica2.pesen.net: segment not found
E "
E assert 0 == 1
E + where 0 = <built-in method count of str object at 0x7f6980fc3198>('Removal of Segment disconnects topology')
E + where <built-in method count of str object at 0x7f6980fc3198> = 'ipa: ERROR: f22replica1.pesen.net-to-f22replica2.pesen.net: segment not found\n'.count
test_integration/test_topology.py:142: AssertionError
==================== 2 failed, 1 passed in 1801.27 seconds =====================
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0002.9-Integration-tests-topology-plugin.patch
Type: text/x-patch
Size: 9103 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150827/3a8e00fd/attachment.bin>
More information about the Freeipa-devel
mailing list