[SSSD] [PATCH] Add integration tests

Michal Židek mzidek at redhat.com
Wed Apr 29 14:58:18 UTC 2015


On 04/29/2015 12:23 PM, Nikolai Kondrashov wrote:
> 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.

We do not want to introduce "regex" like readability to
function parameters. I do not think that the assert functions
need to be powerful, they should be simple.

>
> 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.

This does not look bad to me. Please add example of usage to
the docstring of the "all" and "some" functions. So that
it is clear where and how the functions are to be used.
Also add reference to these functions to the diff function,
so that they are linked both ways (something along the
line "use this and that to create the list or tuple
and do not craft it manually"... of course better
wording is advised :) )

Also I would rename the "some" to "contains" and "all"
to "contains_only". The "some" word misleads the reader
a little in the way that "some of the listed
users must match, but not all", which is not the case.
The "all" can mislead the reader in the way "all these
members must match, but we do not care if there are some
others", which is also not how it works.

>
>> 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
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list