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

[Martin Basti]

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?


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.

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

4)

+        replica.run_command(['poweroff'])
+        time.sleep(3)

There should not be poweroff, probably sleep could be removed too.


  *   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)


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

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")

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160728/97a78065/attachment.htm>