[SSSD] [PATCH] Add integration tests

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


On 04/24/2015 06:16 PM, Michal Židek wrote:
> 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.

I see you already did that :) . Let's continue the main
conversation in this branch of the thread and abandon the
other one, so that we do not have two parallel branches
about the same thing.

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