[Freeipa-devel] [PATCH 0019][Tests] Fix for failing location tests

[Martin Basti]

On 23.06.2016 12:55, Lenka Doudova wrote:
>
>
>
> On 06/23/2016 10:30 AM, Martin Basti wrote:
>>
>>
>>
>> On 23.06.2016 06:55, Lenka Doudova wrote:
>>>
>>>
>>>
>>> On 06/22/2016 05:11 PM, Lenka Doudova wrote:
>>>>
>>>>
>>>>
>>>> On 06/22/2016 04:37 PM, Lenka Doudova wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 06/22/2016 08:33 AM, Martin Basti wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 22.06.2016 07:37, Lenka Doudova wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 06/21/2016 06:57 PM, Martin Basti wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 21.06.2016 15:39, Lenka Doudova wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> attaching patch for failing location tests 
>>>>>>>>> (ipatests/test_xmlrpc/test_location_plugin.py).
>>>>>>>>>
>>>>>>>>> Lenka
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> 1)
>>>>>>>> + expected_updates={u'ipalocation_location': 
>>>>>>>> [location.idnsname_obj],
>>>>>>>> + u'enabled_role_servrole': (
>>>>>>>> +                                  u'CA server', u'DNS server', 
>>>>>>>> u'NTP server')},
>>>>>>>>
>>>>>>>> This depends on services installed on server, so server without 
>>>>>>>> DNS will cause test failures. We probably should skip test id 
>>>>>>>> DNS isn't installed.
>>>>>>>> Without DNS installed you get much more different warnings
>>>>>>>>
>>>>>>>>
>>>>>>>> 2)
>>>>>>>> +    def update(self, updates, expected_updates=None, 
>>>>>>>> messages=None):
>>>>>>>> ....
>>>>>>>> +        self.messages = messages
>>>>>>>>
>>>>>>>> Why is this needed? I'm puzzled by this
>>>>>>>>
>>>>>>>> It is defined outside __init__ what is wrong and it is never used.
>>>>>>>>
>>>>>>> Hi, thanks for review.
>>>>>>> ad 1: will fix
>>>>>>> ad 2: the 'messages' key is indeed used, because this key is 
>>>>>>> returned every time a server is attached to/removed from a 
>>>>>>> location. If the 'messages' is not supplied to the result 
>>>>>>> comparison, the test fails (see 
>>>>>>> https://paste.fedoraproject.org/382936/65732411/ for result of 
>>>>>>> test without applied patch).
>>>>>>>
>>>>>>> Lenka
>>>>>>
>>>>>> Please point me to the line where ServerTracker.messages is used, 
>>>>>> I still don't see it. In your fpaste, removed self.messages in 
>>>>>> that context is TestLocationsServer.messages not 
>>>>>> ServerTracker.messages
>>>>>>
>>>>>> Martin^2
>>>>>
>>>>> Ah, right you are, my good sir. I fixed both issues, hopefully 
>>>>> will be fine now. Fixed patch attached.
>>>>> Lenka
>>>>>
>>>>>
>>>> Self NACK, will provide some more changes tomorrow.
>>>> Lenka
>>>>
>>>>
>>> And here it goes...
>>>
>>> Lenka
>>>
>>>
>> This is really weird
>>
>> fuzzy_servrole = Fuzzy(
>>     type=list,
>>      test=lambda other: False not in set(
>>         item in (u'CA server', u'DNS server', u'NTP server')
>>         for item in other)
>>     and u'DNS server' in other)
>>
>> 1) Why is there special case 'DNS server' in other, IMO general fuzzy 
>> object should not enforce DNS server role
>>
>> 2) Several servroles related to AD trust are missing there, so if you 
>> are doing general fuzzy object for server roles it should contains 
>> all roles
>>
>> 3)
>> I suggest something like
>> lambda other: all(item in {u'CA server', u'DNS server', u'NTP 
>> server', ...} for item in other)
>>
>> 4) during yesterday's interactive review, I suggested to use just 
>> lambda other: True, as serveroles are tested in different test suite, 
>> but If you fix issues above, it can stay as it is
>>
>> Martin^2
> Fixed according to suggestion from interactiove review. Attached.
> Lenka
ACK
Pushed to master: eec440b2d54a8247bb25c521c8603cb203171699

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160623/44090bc5/attachment.htm>