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

[Lenka Doudova]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160714/ce15da87/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0014.4-Tests-Tracker-class-for-services.patch
Type: text/x-patch
Size: 7208 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160714/ce15da87/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-ldoudova-0015.4-Tests-Authentication-indicators-xmlrpc-tests.patch
Type: text/x-patch
Size: 2967 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160714/ce15da87/attachment-0001.bin>