[Freeipa-devel] [PATCH] Fix a bunch of unit tests.
Pavel Zuna
pzuna at redhat.com
Wed Nov 18 12:53:13 UTC 2009
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).
> rob
Pavel
More information about the Freeipa-devel
mailing list