On 05/15/2015 03:39 PM, Nikolai Kondrashov wrote:
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.
I would not call it a problem, but it is better if the git
am applies without unnecessary warnings and I do not think
that the newlines in the end add any value (but it may depend
on editor). I would remove them.
Also I just realized that you use 2 blank lines between functions.
I think it is a custom to use just one in SSSD (at least this
is how it is in most of the SSSD code). IMO one is enough.
> 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.
It is not good to shadow existing standard identifiers.
> 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.
Ok, that sounds good IMO.
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.
You are right that the function could be useful under some
circumstances, but we shall strive toward clean API, not toward
a bunch of inconsistent functions that may be useful under some
circumstances. We have this problem with our sysdb API, where
we have a whole lot of functions useful under some circumstances
and overall it is mess.
The diff function exposes too much internal dirty little secrets
that the user should not care about. For example it takes the
description map as an argument. None of the functions that
take the description map as an argument should ever be made
public. This is for internal use only, to make the asserts
produce better error messages. If we found our selves in the
situation where we would like to use the diff function
directly in the tests, it is probably time to introduce new
assert_ function to the API, that can use the diff internally.
Also the diff_passwd and diff_group should be made static.
The fact that we use some diffs is really of no importance
to the assert_ function caller.
> 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.
Well they are the first tests that use this new code, so they
will be examples for others when writing new tests. It will
be helpful for developers to see the function in real context.
So I think it is worth the effort and the tests can be really
simple. Additionally we will test the assert_ functions
themselves this way (this is IMO important).
Michal