[Freeipa-devel] [PATCH 0014-0016][Tests] Authentication indicators

[Milan Kubík]

On 07/14/2016 11:43 AM, Lenka Doudova wrote:
>
>
>
> On 07/14/2016 11:25 AM, Lenka Doudova wrote:
>>
>>
>>
>> On 07/14/2016 09:20 AM, Lenka Doudova wrote:
>>>
>>>
>>>
>>> On 07/13/2016 04:48 PM, Milan Kubík wrote:
>>>> On 07/11/2016 01:34 PM, Lenka Doudova wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 07/08/2016 02:24 PM, Milan Kubík wrote:
>>>>>> On 07/01/2016 05:13 PM, Lenka Doudova wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 07/01/2016 02:42 PM, Milan Kubík wrote:
>>>>>>>> On 06/16/2016 03:23 PM, Lenka Doudova wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> attached are tests for authentication indicators. Please note:
>>>>>>>>>
>>>>>>>>> 1. newly created service tracker is not exactly complete, list 
>>>>>>>>> of unimplemented methods is in doc. These methods can be 
>>>>>>>>> filled in when existing declarative tests are refactored.
>>>>>>>>>
>>>>>>>>> 2. patch 0015 depends on 0014, so it should not be pushed 
>>>>>>>>> without it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Lenka
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> patch 0014:
>>>>>>>>
>>>>>>>> In the update method, what happens when the updated attributes 
>>>>>>>> contain addattr? It is not clear to me. Is it necessary?
>>>>>>>>
>>>>>>> Example:
>>>>>>>     ipa service-mod SRV --addattr="authind=radius"
>>>>>>>
>>>>>>> Result:
>>>>>>>     The way the tracker works, this adds 
>>>>>>> /u'addattr="authind=radius"'/ to the list of expected results 
>>>>>>> (result of /self.attrs.update(updates)/. Of course nothing like 
>>>>>>> that appears anywhere, so in case there's the /--addattr/ 
>>>>>>> option, it's necessary to ensure it won't get to the 
>>>>>>> /self.attrs/ atribute.
>>>>>>>
>>>>>>>> patch 0015:
>>>>>>>>
>>>>>>>> host1 and service2 do not tell anything about the purpose of 
>>>>>>>> the fixture. Please assign more descriptive names to them.
>>>>>>>> Why do the fixtures have 'function' scope? Does the service 
>>>>>>>> entry exist during the second and third test case?
>>>>>>>>
>>>>>>> Renamed.
>>>>>>>>
>>>>>>>> patch 0016:
>>>>>>>>
>>>>>>>> Per offline discussion, admin user has no special privileges 
>>>>>>>> here, LGTM.
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> Milan Kubik
>>>>>>>
>>>>>>> Thanks for review, fixed patches (14.2 and 15.2) attached.
>>>>>>> Lenka
>>>>>>
>>>>>> NACK,
>>>>>>
>>>>>> the update method of ServiceTracker creates the entry if it 
>>>>>> doesn't exist. Why? I know the base class has this problem also 
>>>>>> [1], though.
>>>>>> Given this will be addressed, the fixtures in the xmlrpc test 
>>>>>> will fail since the fixture scope is wrong - function instead of 
>>>>>> class.
>>>>>>
>>>>>> [1]: https://fedorahosted.org/freeipa/ticket/6045
>>>>>>
>>>>>> -- 
>>>>>> Milan Kubik
>>>>> Hi,
>>>>>
>>>>> fixed patch attached. I chose to leave the scope as it was (in 
>>>>> case one test breaks and entry is not even created, the other 
>>>>> tests can still be successful), but I removed the creation command 
>>>>> from ServiceTracker update method and called it directly in the 
>>>>> tests, so there shouldn't be hidden actions.
>>>>>
>>>>> Lenka
>>>>
>>>> Thanks for the updated patches. There are still some issues I 
>>>> haven't noticed before:
>>>>
>>>> service tracker: track_create method doesn't set self.exists flag 
>>>> which is needed
>>>>
>>>> In the xmlrpc test method `test_adding_second_indicator` uses 
>>>> 'addattr' to set the indicator, why?
>>>>
>>>> -- 
>>>> Milan Kubik
>>>
>>> Hi,
>>> fixes for comments in attached patches.
>>> After offline discussion, I removed the usage of "addattr" and 
>>> replaced it with test to add two indicators in one command.
>>>
>>> Lenka
>>>
>>>
>> One more problem was pointed out to me offline, attaching changed 
>> patch 0014.
>>
>> Lenka
>>
>>
>>
> Resending the complete patch set.
> L.
>
>

Thanks, ACK.

-- 
Milan Kubik

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