[SSSD] [PATCH] Add integration tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Mon Apr 27 15:56:41 UTC 2015


Hi Michal,

Thank you for the review. Please see my comments below.

On 04/24/2015 07:16 PM, Michal Židek wrote:
> I did not test the module, but see the comments bellow (I will
> test it when you sent the whole patch). Also it will be better
> if you always send the entire patch, not just one file. It will
> be easier for me to review that way, especially when the
> patch has example tests, that show the entire thing in
> action :) . Thanks.

In this case the entire patch wasn't ready yet, I didn't want to divert any
attention to other parts, there weren't many examples (just two functions
used) in the patch, the module works fine alone, and I wanted to get early
feedback on this particular module (which, I think, worked).

Anyway, I can do as you ask and send full patches only from now on, no
problem.

> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>>
>> Alright, I did an assertions module by mutating the old "ent" module.
>> Please find it attached.
>>
>> It has both separate, small asserts for verifying individual entries,
>> dictionary-based asserts for verifying a few entries from getpwent, and big
>> asserts for verifying the whole database as I do in initial tests.
>>
>> In short, I tried to address all your requests. Although, not exactly in
>> the
>> way you suggested, but making some shortcuts, trying to keep the code
>> smaller.
>>
>> 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.

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

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.

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

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

Nick



More information about the sssd-devel mailing list