[SSSD] [PATCH] Add integration tests

Michal Židek mzidek at redhat.com
Fri Apr 24 16:16:00 UTC 2015


Hi Nick!

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.

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) 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 function to test just for existence of a user could look
like this:

assert_user_by_name_exists("user1")

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

>
>      # Assert one of the entries returned by getgrent(), with name
> "group1",
>      # has gid == 2001 and no members, and another entry, with name
>      # "one_user_group2", has gid == 2016 and one member named "user2"
>      ent.group_name_dict_assert(dict(group1          = dict(gid=2001,
> mem=[]),
>                                      one_user_group2 = dict(gid=2016,
> mem=["user2"])))
>
>      # Assert one of the entries returned by getpwent(), with uid == 1003,
>      # has name "user3", and another entry, with uid == 1002, has shell
>      # "/bin/bash"
>      ent.passwd_uid_dict_assert({1003: dict(name="user3"),
>                                  1002: dict(shell="/bin/bash")})
>
>      # Assert all the entries returned by getpwent() match at least one
> of the
>      # specified patterns and there are no other entries, entries
> returned by
>      # getpwnam() for names of each of those match, entries returned by
>      # getpwuid() for uids of each of those match.
>      ent.passwd_assert(
>          [
>              dict(name='user1', passwd='*', uid=1001, gid=2001,
> gecos='1001', dir='/home/user1', shell='/bin/bash'),
>              dict(name='user2', passwd='*', uid=1002, gid=2002,
> gecos='1002', dir='/home/user2', shell='/bin/bash'),
>              dict(name='user3', passwd='*', uid=1003, gid=2003,
> gecos='1003', dir='/home/user3', shell='/bin/bash')
>          ], list=True, name=True, uid=True)
>
>      # Assert all the entries returned by getpwent() match at least one
> of the
>      # specified patterns and there are no other entries.
>      ent.passwd_assert(
>          [
>              dict(shell='/bin/bash'),
>              dict(gid=2001, shell='/bin/bash'),
>              dict(dir='/home/user3', shell='/bin/bash')
>          ], list=True)
>



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

>
> The assert functions attempt to explain the differences in the assert
> message.
> The explanations might not be perfect, but I think they help quite a bit.
> I'll show some simpler cases as examples:
>
>      Code:
>      ent.passwd_assert_by_name("user1", dict(uid=1002, gid=2001))
>
>      Traceback (most recent call last):
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ldap_test.py",
> line 272, in test_sanity_rfc2307_bis
>          ent.passwd_assert_by_name("user1", dict(uid=1002, gid=2001))
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ent.py",
> line 135, in passwd_assert_by_name
>          assert not d, d
>      AssertionError: 'uid' mismatch: 1002 != 1001
>
>
>      Code:
>      ent.passwd_uid_dict_assert({1003: dict(name="user4"),
>                                  1002: dict(shell="/bin/bash")})
>
>      Traceback (most recent call last):
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ldap_test.py",
> line 282, in test_sanity_rfc2307_bis
>          1002: dict(shell="/bin/bash")})
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ent.py",
> line 190, in passwd_uid_dict_assert
>          assert not d, d
>      AssertionError: 1003 mismatch: 'name' mismatch: 'user4' != 'user3'
>
>
> And one complicated:
>
>      Code:
>      ent.passwd_assert(
>          [
>              dict(gid=2001, shell='/bin/sh'),
>              dict(dir='/home/user3', shell='/bin/bash')
>          ], list=True)
>
>      Traceback (most recent call last):
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ldap_test.py",
> line 276, in test_sanity_rfc2307_bis
>          ], list=True)
>        File
> "/home/nkondras/projects/fedorahosted.org/sssd/src/tests/intg/ent.py",
> line 243, in passwd_assert
>          assert not d, d
>      AssertionError: list mismatch:
>      unmatched pattern item(s):
>      [{'gid': 2001, 'shell': '/bin/sh'}]
>      unmatched entry item(s):
>      [{'dir': '/home/user1',
>        'gecos': '1001',
>        'gid': 2001,
>        'name': 'user1',
>        'passwd': '*',
>        'shell': '/bin/bash',
>        'uid': 1001},
>       {'dir': '/home/user2',
>        'gecos': '1002',
>        'gid': 2002,
>        'name': 'user2',
>        'passwd': '*',
>        'shell': '/bin/bash',
>        'uid': 1002}]
>

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.

>
> Another note: assertion backtraces will always include the module's
> assertion
> function itself, there doesn't seem to be an easy way to imitate assertion
> failure at the invocation point. However, I kept all "assert" statements
> short
> and simple, so they at least don't grab too much attention.

I am not sure if we can do something about this. But overall the
error reporting is not very optimal in CI, but that can be dealt
with later.



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

Michal



More information about the sssd-devel mailing list