[SSSD] [PATCH v3] Add integration tests

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Fri May 15 13:39:59 UTC 2015


Hi Michal,

On 05/12/2015 10:51 PM, Michal Židek wrote:
> On 05/06/2015 02:51 PM, Nikolai Kondrashov wrote:
>> Here is the third version of the integration test patches.
>
> Sorry for the delayed answer, I am sort of out of time recently. Some
> comments are below.

Sure, no problem. I used that time to do something else :)

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

Hmm, I was putting these there on purpose, to have each function end with
separating whitespace for ease of re-arranging, and to have some whitespace
below the last function when going to the end of the file, so it doesn't stick
to editor decorations, making it harder to read. These are new files and I'm
not adding spurious whitespace to existing files here.

Is this a problem? I can remove it if it is.

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

Argh, this is a bug. I planned to fix it and forgot.

> Also the 'contains_only' does not make sense for the 'uid/gid=true' and
> 'name=true' cases, but you use it there.

Yeah, that is a bit confusing.

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

Yeah, I understand the general problem. However, it is not a problem for the
function user and I think we can manage it in the function itself. The thing
is, "list" is quite clear and easy to understand. I can't quite think of
something else to use in its place. However, see below.

> 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

We already have assert_passwd_list that does this.

> 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

I agree that these functions are a bit complicated. However, I think there's
still value in having a single function matching the whole of a database in
all the possible ways. It will make things easier for some tests.

What if we try to make them simpler? Let's remove list/uid/gid/name arguments.
Just make diff/assert_passwd/group accept only patterns and always check
everything. This way the difference between "contains" and "contains_only"
will make sense as we're always checking against the entry list retrieved
by get(pw|gr)ent.

Make these functions use separate functions for checking a sequence of entries
retrieved by name and by ID and expose those functions. Make it explicit that
the argument is not a pattern, but a sequence of patterns. This will replace
the functionality of removed name/uid/gid parameters, but would make sense.
Have these prototypes for them (similar for groups):

     def assert_each_passwd_with_name(pattern_seq)
     def assert_each_passwd_with_uid(pattern_seq)

These would actually be similar to the assert_passwd_(name|uid)_dict, which
are actually better be renamed into something like this:

     def assert_each_passwd_by_name(pattern_dict)
     def assert_each_passwd_by_uid(pattern_dict)

All this is likely confusing, so I'm attaching a patch doing the above, for
illustration. It is based on the last revision of the patchset we're
discussing here.

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

Yeah, this can improve things. Will do.

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

I'm not so sure about that. Some corner-case tests might need some special
matching and assertions which won't be worth adding to ent.py. At least until
they prove useful elsewhere. Since we can't make assertion backtrace start at
arbitrary point up-stack, building functions on top of the exposed assert_*
functions wouldn't be desirable, and using "diff" in these custom functions
would be more appropriate. It will also be more difficult to form coherent
error messages on assertion failures if building on top of assert_* functions.

Still, it will be easy to expose "diff" later if necessary, and I can do as
you request.

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

These weren't example tests, just some actual tests to start with. However, I
can add some more tests that would use these, no problem. It will take some
more time, though.

Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-intgcheck-Simplify-ent.assert_-passwd-group.patch
Type: text/x-patch
Size: 14645 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150515/e761e32d/attachment.bin>


More information about the sssd-devel mailing list