[Freeipa-devel] [Test][Patch-0047] Added a test for Ticket N 5964

Wed Aug 3 12:45:35 UTC 2016

Hi Martin,

Thanks for the review! Both patches were updated.

On 07/28/2016 04:11 PM, Martin Basti wrote:
>
>
> On 08.07.2016 15:41, Oleg Fayans wrote:
>> Hi Martin,
>>
>> Thanks for the review!
>>
>> On 07/08/2016 02:18 PM, Martin Basti wrote:
>>>
>>>
>>> On 27.06.2016 13:53, Oleg Fayans wrote:
>>>> Hi guys,
>>>>
>>>> Is there a chance the patches NN 0047.1 and 0048.1 get reviewed before
>>>> 4.4 release? They cover a good part of the Managed Topology 4.4
>>>> feature.
>>>>
>>>> On 06/17/2016 11:18 AM, Oleg Fayans wrote:
>>>>> One more test was added to the patch-0048
>>>>>
>>>>> On 06/17/2016 09:43 AM, Oleg Fayans wrote:
>>>>>> Fixed a bug in the previous patch, automated 2 more testcases from
>>>>>> http://www.freeipa.org/page/V4/Manage_replication_topology_4_4/Test_Plan
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 06/16/2016 04:46 PM, Oleg Fayans wrote:
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>> IIUC, this will turn off the machine completely, how is cleanup done
>>> then.  AFAIK our tests cannot turn on machine again and run cleanup, so
>>> you will not be able to run more tests on the same topology without
>>> manual cleanup and manual start.
>>>
>>> +        replica = self.replicas[0]
>>> +        replica.run_command(['poweroff'])
>>>
>>> IMO would be better to just call 'ipactl stop' instead of 'poweroff'
>>
>> Agreed! Fixed.
>>
>>>
>>> Martin^2
>>>
>>
>>
>>
> *Automated ipa-replica-manage del tests*
>
> 1)
> +        replica.run_command(['ipactl', 'stop'])
> +        time.sleep(3)
>
> Why do you need sleep here?

Removed, it was left from the old "poweroff" approach

>
>
> 2)
> +        ruvid_re = re.compile(".*%s:389: (\d+).*" % replica.hostname)
> +        replica_ruvs = ruvid_re.findall(result.stdout_text)
> +        master.run_command(['ipa-replica-manage', 'clean-ruv', 'f',
> +                            '-p', master.config.dirman_password,
> +                            replica_ruvs[0]])
>
> Because you are using re.findall(), without any match you will receive
> IndexError here replica_ruvs[0]. IMO it deserves assert before

Implemented the assert which checks that the output contains enough 
replica RUVs

>
> 3)
> assert(replica.hostname in result1.stdout_text)
>
> I think that this is error prone. What if there is just error 'could not
> connect to replica <replica hostname>', or something similar. instead of
> listing/cleaning/whatever operation was executed. I think that it should
> be more specific regexp than just finding a replica name substring  (Yes
> In IPA we dont always print error so stderr)
>
> I'm not sure, but probably there might be cases when non critical error
> happen and exist status is still 0

Agree. Implemented a regex-based search

>
> 4)
>
> +        replica.run_command(['poweroff'])
> +        time.sleep(3)
>
> There should not be poweroff, probably sleep could be removed too.

Gone

>
>
>   *   Automated clean-ruv subcommand test*
>
> 1) PEP8, 2 new lines expected
> ./ipatests/test_integration/test_topology.py:163:1: E302 expected 2
> blank lines, found 0
> ./ipatests/test_integration/test_topology.py:182:80: E501 line too long
> (85 > 79 characters)

Fixed

>
>
> 2)
> I dont like doing assert just with count of occurences of substring in
> STDOUT, would be possible to improve this somehow?

Maybe, but frankly, I don't see how. In this case we are making sure 
that both simple and CA-specific RUVs of a replica are displayed. The 
format of the output is strict:
Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
Certificate Server Replica Update Vectors:
replica1_hostname:389: RUV_id
replica2_hostname:389: RUV_id
If we do not see 2 occurrences of the replica hostname than definitely 
something went wrong

>
> 3)
> I'm not sure if clean-ruv is instant operations or there is some magic
> happening in background (we have abort-clean-ruv). Maybe some sleep
> should be there, but this needs investigation.
>
> +        assert(replica.hostname in result2.stdout_text), (
> +            "The wrong RUV was deleted")
> +        result3 = master.run_command(['ipa-replica-manage', 'list-ruv',
> +                                      '-p', master.config.dirman_password])
> +        assert(result3.stdout_text.count(replica.hostname) == 1), (
> +            "CA RUV of the replica is still displayed")
>

Based on my discussion with Stanislav Laznicka, I understood that by 
default clean-ruv does not return the shell until the operation is 
finished. You can force dropping into the shell by pressing CTRL+C, in 
which case the background job will still be running, but this is not the 
default behavior

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0047.2-Automated-clean-ruv-subcommand-test.patch
Type: text/x-patch
Size: 3675 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160803/ec095881/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ofayans-0048.3-Automated-ipa-replica-manage-del-tests.patch
Type: text/x-patch
Size: 4659 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160803/ec095881/attachment-0001.bin>