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@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
