[SSSD] [PATCH v3] Add integration tests

Michal Židek mzidek at redhat.com
Tue May 12 19:59:10 UTC 2015


On 05/12/2015 09:51 PM, Michal Židek wrote:
> 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):
                                   ^^^^
Of course this should have been touple, not list.

>          return "assert_passwd_by_name_touple accepts only touples. Did
> you use function 'contains' instead of 'contains_only'?"
                     ^^^^^^^^              ^^^^^^^^^^^^^
And this should have been the other way around, but you get the point :)


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