[SSSD] [PATCH v3] Add integration tests

Michal Židek mzidek at redhat.com
Tue May 12 19:51:40 UTC 2015


On 05/06/2015 02:51 PM, Nikolai Kondrashov wrote:
> Hi everyone,
>
> Here is the third version of the integration test patches.
>
> Changes from the second version include the following.
>
> * Rebased on latest master.
> * Moved verbs to the start of ent.py function names, so assertion function
>    names start with "assert_".
> * Hidden a few of the internal ent.py functions under the underscore
> prefix,
>    removed others, please tell if anything else needs to be hidden.
> * Made passwd/group dictionary matching use get(pw|gr)(nam|uid|gid)
> functions
>    instead of get(gr|pw)ent, as originally requested for decoupling
> requested
>    users/groups from patterns to match, for alias support. This concerns
>    assert_(passwd|group)_(name|uid|gid)_dict functions.
> * Added passing pattern structure descriptions to the diff function, so
>    assertion failure messages provide more clue to what happened. Made some
>    adjustments to message generation for the same purpose.
> * Added support for matching only some entries. E.g. matching only a few
> users
>    returned from getpwent, or matching only a few group members.
> Implemented
>    with tuple patterns.
> * Added "ent.contains" and "ent.contains_only" functions creating tuple and
>    list patterns to possibly help readability.
>
> Thanks to anyone taking on the review of these patches!
>
> Nick
>

Hi Nick!

Sorry for the delayed answer, I am sort of out of time recently. Some
comments are below.

One of the patches generates whitespace warnings:

Applying: Add integration tests
/home/user/gitrepo/sssd/.git/rebase-apply/patch:419: new blank line at EOF.
+
/home/user/gitrepo/sssd/.git/rebase-apply/patch:1163: new blank line at EOF.
+
/home/user/gitrepo/sssd/.git/rebase-apply/patch:1276: new blank line at EOF.
+
/home/user/gitrepo/sssd/.git/rebase-apply/patch:1535: new blank line at EOF.


 From users perspective, it is very strange that the functions
assert_passwd and assert_group do accept pattern created with
'contains_only', but do not accept one created with 'contains' when
uid or name are true. Also the 'contains_only' does not
make sense for the 'uid/gid=true' and 'name=true' cases, but you
use it there.

Also these functions use arguments with name 'list' (avoid using
identifiers that already have meaning in python).

The whole logic around assert_passwd, assert_group, diff_passwd and
diff_group is unnecessary complicated and results in strange
behaviour. Replace those four functions with the following
asserts (untested, just copy-pasted from your code and removed/modified 
some lines):

def assert_passwd_all(pattern):
     d = diff(get_passwd_list(), pattern, DIFF_DESC_PASSWD_ALL)
     assert not d, d

I do not think it makes sense to use lists with the following
functions, so it would be good to add _touple suffix to them.

def assert_passwd_by_name_touple(pattern):
     if not isinstance(pattern, list):
         return "assert_passwd_by_name_touple accepts only touples. Did 
you use function 'contains' instead of 'contains_only'?"
     for pv in pattern:
         pv_name = pv["name"]
         try:
             ent = get_passwd_by_name(pv_name)
         except KeyError, err:
             return "no name " + pformat(pv_name) + ": " + str(err)
         d = diff(ent, pv) <<== consider DIFF_ constant here
         assert not d, "name " + pformat(pv_name) + " mismatch: " + d

def assert_passwd_by_uid_touple(pattern):
- similar to the above

See the DIFF_DESC_PASSWD_ALL. This is a constant. Do not use directly
the description map when calling diff. Define some constants (just
uppercased variables) near the diff function and use those instead.
It will be probably good if the constants are named after the
functions they are passed from as I did in the example.

Also make the diff function _diff, we will not use it outside the
module.

And one more thing. Use all the assert_ functions in the example
tests. It will serve as example/documentation for later usage.

Michal



More information about the sssd-devel mailing list