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
>From 061245df852a8368e2a6539b40fb6c4946c08f99 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov <nikolai.kondras...@redhat.com>
Date: Fri, 15 May 2015 16:34:49 +0300
Subject: [PATCH 1/1] intgcheck: Simplify ent.assert_(passwd|group)
---
src/tests/intg/ent.py | 280 ++++++++++++++++++++++++++------------------
src/tests/intg/ldap_test.py | 8 +-
2 files changed, 169 insertions(+), 119 deletions(-)
diff --git a/src/tests/intg/ent.py b/src/tests/intg/ent.py
index 527071c..89d1811 100644
--- a/src/tests/intg/ent.py
+++ b/src/tests/intg/ent.py
@@ -242,80 +242,105 @@ def assert_passwd_list(pattern):
assert not d, d
-def assert_passwd_name_dict(pattern):
+def diff_each_passwd_by_name(pattern_dict):
"""
- Assert each passwd entry with name from the dictionary pattern keys
- matches corresponding pattern value.
+ Describe difference between each pattern_dict value and a passwd entry
+ retrieved by name being the corresponding key.
"""
try:
- ent = dict((k, get_passwd_by_name(k)) for k in pattern.keys())
+ ent = dict((k, get_passwd_by_name(k)) for k in pattern_dict.keys())
except KeyError, err:
- assert False, err
- d = diff(ent, pattern, {None: ("user", {})})
- assert not d, d
+ return str(err)
+ return diff(ent, pattern_dict, {None: ("user", {})})
-def assert_passwd_uid_dict(pattern):
+def diff_each_passwd_by_uid(pattern_dict):
"""
- Assert each passwd entry with UID from the dictionary pattern keys
- matches corresponding pattern value.
+ Describe difference between each pattern_dict value and a passwd entry
+ retrieved by UID being the corresponding key.
"""
try:
- ent = dict((k, get_passwd_by_uid(k)) for k in pattern.keys())
+ ent = dict((k, get_passwd_by_uid(k)) for k in pattern_dict.keys())
except KeyError, err:
- assert False, err
- d = diff(ent, pattern, {None: ("user", {})})
+ return str(err)
+ return diff(ent, pattern_dict, {None: ("user", {})})
+
+
+def diff_each_passwd_with_name(pattern_seq):
+ """
+ Describe difference between each pattern in pattern_seq sequence and a
+ passwd entry retrieved by name being the pattern's "name" value.
+ """
+ return diff_each_passwd_by_name(dict((p["name"], p) for p in pattern_seq))
+
+
+def diff_each_passwd_with_uid(pattern_seq):
+ """
+ Describe difference between each pattern in pattern_seq sequence and a
+ passwd entry retrieved by UID being the pattern's "uid" value.
+ """
+ return diff_each_passwd_by_uid(dict((p["uid"], p) for p in pattern_seq))
+
+
+def assert_each_passwd_by_name(pattern_dict):
+ """
+ Assert each pattern_dict value matches a passwd entry retrieved by
+ name being the corresponding key.
+ """
+ d = diff_each_passwd_by_name(pattern_dict)
assert not d, d
-def diff_passwd(pattern, list=False, name=False, uid=False):
+def assert_each_passwd_by_uid(pattern_dict):
+ """
+ Assert each pattern_dict value matches a passwd entry retrieved by
+ UID being the corresponding key.
+ """
+ d = diff_each_passwd_by_uid(pattern_dict)
+ assert not d, d
+
+
+def assert_each_passwd_with_name(pattern_seq):
+ """
+ Assert each pattern in pattern_seq sequence matches a passwd entry
+ retrieved by name being the pattern's "name" value.
+ """
+ d = diff_each_passwd_with_name(pattern_seq)
+ assert not d, d
+
+
+def assert_each_passwd_with_uid(pattern_seq):
+ """
+ Assert each pattern in pattern_seq sequence matches a passwd entry
+ retrieved by UID being the pattern's "uid" value.
+ """
+ d = diff_each_passwd_with_uid(pattern_seq)
+ assert not d, d
+
+
+def diff_passwd(pattern):
"""
Describe difference between passwd database and a pattern.
- If "list" is True, diff against entry list.
- If "name" is True, diff every pattern item against an entry
- retrieved by name from the item's "name" key.
- If "uid" is True, diff every pattern item against an entry retrieved
- by UID from the item's "uid" key.
- """
- if list:
- d = diff(get_passwd_list(), pattern, {None: ("user", {})})
- if d:
- return "list mismatch: " + d
- if name or uid:
- if not isinstance(pattern, type([])):
- return "pattern is not a list"
- for pv in pattern:
- if name:
- 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)
- if d:
- return "name " + pformat(pv_name) + " mismatch: " + d
- if uid:
- pv_uid = pv["uid"]
- try:
- ent = get_passwd_by_uid(pv_uid)
- except KeyError, err:
- return "no UID " + pformat(pv_uid) + ": " + str(err)
- d = diff(ent, pv)
- if d:
- return "UID " + pformat(pv_uid) + " mismatch: " + d
+ Each pattern entry must have "name" and "uid" attribute.
+ """
+ d = diff(get_passwd_list(), pattern, {None: ("user", {})})
+ if d:
+ return "list mismatch: " + d
+ d = diff_each_passwd_with_name(pattern)
+ if d:
+ return "name retrieval mismatch: " + d
+ d = diff_each_passwd_with_uid(pattern)
+ if d:
+ return "UID retrieval mismatch: " + d
return None
-def assert_passwd(pattern, list=False, name=False, uid=False):
+def assert_passwd(pattern):
"""
Assert passwd database matches a pattern.
- If "list" is True, match against entry list.
- If "name" is True, match every pattern item against an entry
- retrieved by name from the item's "name" key.
- If "uid" is True, match every pattern item against an entry retrieved
- by UID from the item's "uid" key.
+ Each pattern entry must have "name" and "uid" attribute.
"""
- d = diff_passwd(pattern, list=list, name=name, uid=uid)
+ d = diff_passwd(pattern)
assert not d, d
@@ -380,89 +405,114 @@ def assert_group_list(pattern):
assert not d, d
-def assert_group_name_dict(pattern):
+def diff_each_group_by_name(pattern_dict):
"""
- Assert each group entry with name from the dictionary pattern keys
- matches corresponding pattern value.
+ Describe difference between each pattern_dict value and a group entry
+ retrieved by name being the corresponding key.
"""
try:
- ent = dict((k, get_group_by_name(k)) for k in pattern.keys())
+ ent = dict((k, get_group_by_name(k)) for k in pattern_dict.keys())
except KeyError, err:
- assert False, err
- d = diff(ent, pattern,
- {None: ("group",
- {"mem": ("member list",
- {None: ("member", {})})})})
- assert not d, d
+ return str(err)
+ return diff(ent, pattern_dict,
+ {None: ("group",
+ {"mem": ("member list",
+ {None: ("member", {})})})})
-def assert_group_gid_dict(pattern):
+def diff_each_group_by_gid(pattern_dict):
"""
- Assert each group entry with GID from the dictionary pattern keys
- matches corresponding pattern value.
+ Describe difference between each pattern_dict value and a group entry
+ retrieved by GID being the corresponding key.
"""
try:
- ent = dict((k, get_group_by_gid(k)) for k in pattern.keys())
+ ent = dict((k, get_group_by_gid(k)) for k in pattern_dict.keys())
except KeyError, err:
- assert False, err
- d = diff(ent, pattern,
- {None: ("group",
- {"mem": ("member list",
- {None: ("member", {})})})})
+ return str(err)
+ return diff(ent, pattern_dict,
+ {None: ("group",
+ {"mem": ("member list",
+ {None: ("member", {})})})})
+
+
+def diff_each_group_with_name(pattern_seq):
+ """
+ Describe difference between each pattern in pattern_seq sequence and a
+ group entry retrieved name being the pattern's "name" value.
+ """
+ return diff_each_group_by_name(dict((p["name"], p) for p in pattern_seq))
+
+
+def diff_each_group_with_gid(pattern_seq):
+ """
+ Describe difference between each pattern in pattern_seq sequence and a
+ group entry retrieved by GID being the pattern's "gid" value.
+ """
+ return diff_each_group_by_gid(dict((p["gid"], p) for p in pattern_seq))
+
+
+def assert_each_group_by_name(pattern_dict):
+ """
+ Assert each pattern_dict value matches a group entry retrieved by
+ name being the corresponding key.
+ """
+ d = diff_each_group_by_name(pattern_dict)
+ assert not d, d
+
+
+def assert_each_group_by_gid(pattern_dict):
+ """
+ Assert each pattern_dict value matches a group entry retrieved by
+ GID being the corresponding key.
+ """
+ d = diff_each_group_by_gid(pattern_dict)
+ assert not d, d
+
+
+def assert_each_group_with_name(pattern_seq):
+ """
+ Assert each pattern in pattern_seq sequence matches a group entry
+ retrieved by name being the pattern's "name" value.
+ """
+ d = diff_each_group_with_name(pattern_seq)
+ assert not d, d
+
+
+def assert_each_group_with_gid(pattern_seq):
+ """
+ Assert each pattern in pattern_seq sequence matches a group entry
+ retrieved by GID being the pattern's "gid" value.
+ """
+ d = diff_each_group_with_gid(pattern_seq)
assert not d, d
-def diff_group(pattern, list=False, name=False, gid=False):
+def diff_group(pattern):
"""
Describe difference between group database and a pattern.
- If "list" is True, diff against entry list.
- If "name" is True, diff every pattern item against an entry
- retrieved by name from the item's "name" key.
- If "gid" is True, diff every pattern item against an entry retrieved
- by GID from the item's "gid" key.
- """
- if list:
- d = diff(get_group_list(), pattern,
- {None: ("group",
- {"mem": ("member list",
- {None: ("member", {})})})})
- if d:
- return "list mismatch: " + d
- if name or gid:
- if not isinstance(pattern, type([])):
- return "pattern is not a list"
- for pv in pattern:
- if name:
- pv_name = pv["name"]
- try:
- ent = get_group_by_name(pv_name)
- except KeyError, err:
- return "no name " + pformat(pv_name) + ": " + str(err)
- d = diff(ent, pv)
- if d:
- return "name " + pformat(pv_name) + " mismatch: " + d
- if gid:
- pv_gid = pv["gid"]
- try:
- ent = get_group_by_gid(pv_gid)
- except KeyError, err:
- return "no GID " + pformat(pv_gid) + ": " + str(err)
- d = diff(ent, pv)
- if d:
- return "GID " + pformat(pv_gid) + " mismatch: " + d
+ Each pattern entry must have "name" and "gid" attribute.
+ """
+ d = diff(get_group_list(), pattern,
+ {None: ("group",
+ {"mem": ("member list",
+ {None: ("member", {})})})})
+ if d:
+ return "list mismatch: " + d
+ d = diff_each_group_with_name(pattern)
+ if d:
+ return "name retrieval mismatch: " + d
+ d = diff_each_group_with_gid(pattern)
+ if d:
+ return "GID retrieval mismatch: " + d
return None
-def assert_group(pattern, list=False, name=False, gid=False):
+def assert_group(pattern):
"""
Assert group database matches a pattern.
- If "list" is True, match against entry list.
- If "name" is True, match every pattern item against an entry
- retrieved by name from the item's "name" key.
- If "gid" is True, match every pattern item against an entry retrieved
- by GID from the item's "gid" key.
+ Each pattern entry must have "name" and "gid" attribute.
"""
- d = diff_group(pattern, list=list, name=name, gid=gid)
+ d = diff_group(pattern)
assert not d, d
diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py
index 084ab73..6d598c9 100644
--- a/src/tests/intg/ldap_test.py
+++ b/src/tests/intg/ldap_test.py
@@ -195,7 +195,7 @@ def test_sanity_rfc2307(ldap_conn, sanity_rfc2307):
dict(name='user2', passwd='*', uid=1002, gid=2002, gecos='1002', dir='/home/user2', shell='/bin/bash'),
dict(name='user3', passwd='*', uid=1003, gid=2003, gecos='1003', dir='/home/user3', shell='/bin/bash')
)
- ent.assert_passwd(passwd_pattern, list=True, name=True, uid=True)
+ ent.assert_passwd(passwd_pattern)
group_pattern = ent.contains_only(
dict(name='group1', passwd='*', gid=2001, mem=ent.contains_only()),
@@ -204,7 +204,7 @@ def test_sanity_rfc2307(ldap_conn, sanity_rfc2307):
dict(name='empty_group', passwd='*', gid=2010, mem=ent.contains_only()),
dict(name='two_user_group', passwd='*', gid=2012, mem=ent.contains_only("user1", "user2"))
)
- ent.assert_group(group_pattern, list=True, name=True, gid=True)
+ ent.assert_group(group_pattern)
with pytest.raises(KeyError):
pwd.getpwnam("non_existent_user")
@@ -222,7 +222,7 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis):
dict(name='user2', passwd='*', uid=1002, gid=2002, gecos='1002', dir='/home/user2', shell='/bin/bash'),
dict(name='user3', passwd='*', uid=1003, gid=2003, gecos='1003', dir='/home/user3', shell='/bin/bash')
)
- ent.assert_passwd(passwd_pattern, list=True, name=True, uid=True)
+ ent.assert_passwd(passwd_pattern)
group_pattern = ent.contains_only(
dict(name='group1', passwd='*', gid=2001, mem=ent.contains_only()),
@@ -239,7 +239,7 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis):
dict(name='group_two_user_group', passwd='*', gid=2018, mem=ent.contains_only("user1", "user2")),
dict(name='group_two_one_user_groups', passwd='*', gid=2019, mem=ent.contains_only("user1", "user2"))
)
- ent.assert_group(group_pattern, list=True, name=True, gid=True)
+ ent.assert_group(group_pattern)
with pytest.raises(KeyError):
pwd.getpwnam("non_existent_user")
--
2.1.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
