[Freeipa-devel] [PATCH] Fix a bunch of unit tests.

[Rob Crittenden]

Pavel Zuna wrote:
> Rob Crittenden wrote:
>> Pavel Zuna wrote:
>>> Rob Crittenden wrote:
>>>> Pavel Zuna wrote:
>>>>> Only pwpolicy test is still broken - I'm looking into it.
>>>>>
>>>>> Pavel
>>>>
>>>> This brings up the return values question again. I thought we had 
>>>> decided that any attribute that had only one value would be returned 
>>>> as a scalar. In this case userCertificate is being returned as a 
>>>> list which is causing things to fail. Now arguably userCertificate 
>>>> is a multi-valued attribute but we will only store one certificate 
>>>> at a time there so I think we're ok.
>>> Yeah, I remember, but I'm not sure if we agreed on the logic.
>>>
>>> There are 2 ways of doing this:
>>>
>>> 1) Make ldap backend check the schema. If it's multi-value - leave it 
>>> as a list. If it's single-value - convert it to a scalar.
>>>
>>> 2) Make ldap backend check if the attribute contains 1 or more 
>>> values. If there's only one, convert it to a scalar.
>>>
>>> With 1) plugin authors can depend on the schema when manipulating 
>>> attributes, but they have to know the schema and handle multi/single 
>>> attributes differently.
>>
>> Yes but they already have to deal with it/be aware of it because 
>> updates may fail if you try to add another value to a single-valued 
>> attribute.
> Yeah, the command will fail, but at least the code won't blow up.
> 
>>>
>>> With 2) plugin authors have to always check, if the attribute is a 
>>> list or a scalar.
>>
>> Not necessarly. If the author has some awareness of the schema they 
>> can get by ok.
> That's true for single-value attributes. Multi-value attributes with 
> only one value will get converted to scalars. Also, when handling a set 
> of attributes (in a loop for example, which is the case most of the 
> time), we still have to check every single one:
> 
> for a in attrs:
>     if isinstance(a, (list, tuple)):
>         # do something
>     else:
>         # do something
> 
> Or (and I've seen this A LOT in old code):
> 
> for a in attrs:
>     if not isinstance(a, (list, tuple)):
>         a = [a]
>     # do something
> 
> Instead of:
> 
> for a in attrs:
>     # do something
> 
>>
>>> I think that having attributes always returned as list makes things 
>>> easier on plugin authors - no checks required, everything is handled 
>>> the same way. What's the advantage of returning attribute values as 2 
>>> different types?
>>
>> Because some values are single-value by nature and we're treating them 
>> like multi-values.
> Ok, fair enough, I'll make the change. I don't really mind that much, I 
> just prefer generic solutions, where everything handles the same. :)
> 
>>>>
>>>> Also, why the change to the principal name in the service tests?
>>> At first I didn't know where the problem in this test was. So, I 
>>> tried a few different things and this is a leftover. Doesn't hurt 
>>> anything, but I can always change it back.
>>
>> Yes, please do. You're effectively adding a subdomain onto the hostname.
> I just checked and I'm not adding any subdomains. This patch changes the 
> hostname from 'ipatest.$IPA_DOMAIN' to 'ipatest.test' and we no longer 
> make the assumption the host already exists (the test was failing on my 
> machine, because it didn't exist).

Yeah, you're right, I misread the patch. I though it was still sticking 
in the domain name. In any case hostname this change isn't needed.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20091118/77af4130/attachment.bin>