[SSSD] [PATCH] Add integration tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Wed Apr 29 10:23:52 UTC 2015


On 04/28/2015 05:49 PM, Michal Židek wrote:
> On 04/27/2015 05:56 PM, Nikolai Kondrashov wrote:
>>> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>> There is no actual libc interface to check for user existence, so I think
>> having an assertion for that will be misleading. If you don't care what
>> attributes a user has, and only want to check that it exists, just pass an
>> empty pattern:
>>
>>      assert_passwd_by_name("user1", {})
>>
>> BTW, I wouldn't like to change "passwd" to "user". I think it will be more
>> straightforward to simply use NSS database names for all entry types.
>
> assert_passwd_by_name is a good name and it is probably better than
> the _has_attributes that I suggested. My strongest request was that
> all assert functions start with the prefix assert_ .

Alright.


>>> Do not worry too much if the identifiers get longer. Sometimes
>>> it is better to sacrifice the keyboard lifetime for the sake of
>>> better readability :) .
>>
>> More verbose interface might be easier to read the first few times, but
>> becomes a burden both to read (more text to scan) and to write (more
>> text to
>> type and make mistakes in), after you learn it.
>
> I tend to disagree here. It is IMO much greater burden if I need
> to check internals of functions just to get an idea
> of what the function is doing.

I don't like that either. I guess my function names and descriptions were just
not clear enough, or maybe we don't match on the acceptable clarity.


>>>> I attempted to describe the logic behind the generic matching function
>>>> "diff", but please ask if anything is not clear. There is one particular
>>>> flaw to it, though: there is no way to match a list partially, e.g. to
>>>> match for presence of just a few group members, instead of all. I
>>>> think we
>>>> can employ tuples for these. I.e. say that a tuple pattern matches a
>>>> list
>>>> if all tuple items match at least one list item, but not necessarily the
>>>> other way around.
>>>
>>> It will be better to remove this limitation, but not with the
>>> tuples.
>>> If think it is not good to have change in behaviour based on whether
>>> the argument is tuple or list. A better approach would be to have
>>> additional boolean argument in diff(), named for example match_all with
>>> default set to False (or True). If the parameter is True, then
>>> all returned entries must match some pattern, if not, it is enough
>>> if each given pattern matches some entries in the returned list of
>>> entries. Would that work for you?
>>
>> I would very much like to avoid this solution. It would be all-or-nothing
>> (i.e. no mixing different conditions in one call, no matter how high-level)
>> and would increase the code size by requiring passing that variable through
>> the stack of many functions.
>
> Well, the parameter match_all would have default value (I think False
> would be a good default) so the user will not have to pass it if
> he is OK with the default behaviour. And the stack of the functions
> is not very high (2) so I do not see this as a big problem.

The stacks are not deep but there are quite a few of them. These functions
would need to have that argument (using current, not projected names), along
with some limitations:

diff                    - matching all or some entries
passwd_list_assert      - matching all or some entries
passwd_diff             - matching all or some entries
passwd_assert           - matching all or some entries
group_assert_by_name    - matching all or some group members
group_assert_by_uid     - matching all or some group members
group_list_assert       - matching all or some entries and members,
                           but cannot match all entries with some members, or
                           some entries with all members
group_name_dict_assert  - matching all or some group members
group_gid_dict_assert   - matching all or some group members
passwd_diff             - matching all or some entries and members,
                           but cannot match all entries with some members, or
                           some entries with all members
passwd_assert           - matching all or some entries and members,
                           but cannot match all entries with some members, or
                           some entries with all members

We can, of course, remove or change some of these functions, but the general
problem will stay and it will reduce flexibility, forcing us to add more
functions eventually.


>> We already have different matching logic for dictionaries and lists. Adding
>> tuples doesn't make it much more complex, and keeps the patterns
>> succinct and
>> expressive.
>
> The difference between lists and dictionaries is visually big
> enough (if the dictionary is not empty) to make sure the user will
> not mistake the two and get the expected behaviour. In case
> of tuples and lists, the visual difference is not so big.
>
> I think it will be better if we get rid of this behaviour where
> different parameter types do these subtle diffrencies in
> behavior.

What if we think about these patterns not just as fixed format arguments, but
rather similar to, say, regexes? Sure, regexes are cryptic, and subtle
differences in them introduce significant behavior changes, like omitting or
inserting an extra backslash, using '*', instead of '+', etc. Yet, they're
also powerful.

I understand your motivation to keep the interface accessible and safe, and
I'm willing to implement it your way, if this message doesn't change your
mind.

May I suggest another solution, which might make it more palatable? What if we
replace lists/tuples with functions or class constructors, which would
annotate what's happening?

So, instead of writing this rather unusual, but illustrative example:

     pattern = (
         {name='group1', gid=1001, mem=['userA', 'userB', 'userC']},
         {name='group2', gid=1002, mem=('userX', 'userY')},
     )
     ent.group_assert(pattern)

You would write something like this:

     pattern = ent.some(
         {name='group1', gid=1001, mem=ent.all('userA', 'userB', 'userC')},
         {name='group2', gid=1002, mem=ent.some('userX', 'userY')}
     )
     ent.group_assert(pattern)

Theoretically, we can also make a distinction of matching some/all of
dictionary members, but I don't see that as useful ATM.


> We can add "type" parameter to the diff function and init
> the messages based on this parameter. This way we will not
> have to copy-paste the code and we could use more specific
> messages.

Hmm, I guess that might go some way. I'll try it, thanks.


> Also do not forget to rebase the patches on top of the
> current master. There were some changes in the build
> files recently.

Sure, of course.

Nick



More information about the sssd-devel mailing list