[SSSD] [PATCH] Add integration tests

Michal Židek mzidek at redhat.com
Tue Apr 28 14:49:56 UTC 2015


On 04/27/2015 05:56 PM, Nikolai Kondrashov wrote:
>> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>>> Here are some examples of using the module:
>>>
>>>      # Assert getpwnam("user1") returns an entry with uid == 1001 and
>>>      # gid == 2001
>>>      ent.passwd_assert_by_name("user1", dict(uid=1001, gid=2001))
>>
>> It will be better if all assert functions begin with the
>> prefix assert_ (or alternatively you can put them to separate
>> module called ci_assert or something similar, but maybe prefixes
>> are enough)
>
> I wanted to have database name in front, but I think I can put the verb
> forward instead. It might make the code easier to read, indeed.
>
>> and they should include the condition that needs to
>> be met as part of their identifier. So this function should
>> be something like:
>>
>> assert_user_by_name_has_attributes("user1", dict(uid=1001, gid=2001))
>>
>> or with uid:
>>
>> assert_user_by_uid_has_attributes(1001, dict(name="user1", gid=2001))
>
> The problem is, "has_attributes" would only be correct for users. For
> groups
> we would have to use something else after we add partial matching of group
> members. I think we can use "matches" instead, as in
> "assert_user_by_uid_matches", as in "matches the pattern". We would have to
> use "matches" with most of the functions, though. So, if all of them just
> check for pattern matching, maybe we shouldn't repeat that and omit
> "matches"
> altogether?
>
> Still, switching to using specific functions can fix that (see below).
>
>> The function to test just for existence of a user could look
>> like this:
>>
>> assert_user_by_name_exists("user1")
>
> 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_ .

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

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

>
>> Instead of "unmatched pattern items", it would be better to use
>> something that is readable even for people who are not familiar
>> with the implementation. Imagine a contributor wondering: "What
>> the heck is pattern item?". Maybe something like this would be
>> better:
>>
>> - "Expected entries with these attributes were not found:" followed by
>> list of patterns that did not match any of the returned entries
>>
>> - "Unexpected entries found:" followed by entries that were
>> not matched by any given pattern (only if argument match_all
>> described above is true)
>>
>> Feel free to be verbose here. It will only be printed in case
>> of error, so the easier to spot, the better.
>
> I'll try to think of something better, but I suspect not much can be done
> here. The problem is the "diff" function is very generic and is used to
> match
> everything. This is done in an attempt to write less code (so there is
> less to
> understand and to maintain).
>
> At the moment, it is used to match entry (passwd/group) lists, as well as
> group member lists. So, "with these attributes" wouldn't apply to group
> members. Moreover, I would like to use this function to match other entry
> types eventually. I.e. netgroups and services and that may make it even
> more
> difficult to produce non-abstract error messages, which would fit all the
> uses.
>
> This is the general problem of doing abstract (short) code - the
> specifics are
> shifted up the stack. Personally, I have no problem with that. I prefer
> shorter and more flexible code that is easier to apply to new problems,
> and I
> usually adjust to the patterns required to use it easily. I understand
> that it
> makes it harder to read for the first few times, but I think it is worth
> it in
> the long term. In our case, I think we can afford it, as we're smart and
> have
> low developer turnover, and even outside developers need to be smart as
> well,
> to understand other specifics. Still, I might be miscalculating.
>
> If you think this might be a problem after all, I can do away with the
> single
> matching function and make specific functions instead. I.e. have separate
> functions to match various conditions of separate entry types, which would
> produce more specific error messages.
>
> However, there is one drawback to this: there will be more code,
> copy-pasting,
> and larger, less consistent interface to learn and maintain. I think
> that it
> is easier to learn what a pattern is and how it matches, once, then
> learn what
> each function matches against, than learn a bunch of specific functions and
> their specific interfaces. However, I understand that use of such interface
> might be cryptic at first.
>
> So, would you agree to keep the interface generic and somewhat harder to
> learn, but easier to keep in mind and more flexible?
>
> Or would you prefer I make the interface more specific, with descriptive
> function names, each tailored to specific task and producing specific error
> messages, so easier to understand the first few times, but larger and
> harder
> to remember, and needing more energy to read and to type, once learned?
>

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.

>> Also one more important thing. You mix up functions that can
>> be considered public API of the modules together with helper
>> private functions (that would be static in C). We should
>> clearly distinguish between public and private functions
>> (add underscore as prefix to the private functions and maybe
>> group the public ones on top of the module?)
>
> Alright, I'll hide some of the functions that are of little use to the
> outside
> world, and you'll tell me if you'd like anything else hidden.
>

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

Michal




More information about the sssd-devel mailing list