[SSSD] [PATCH] Improvements to nested LDAP unit tests

Lukas Slebodnik lslebodn at redhat.com
Wed Jun 25 16:40:26 UTC 2014


On (25/06/14 15:57), Jakub Hrozek wrote:
>On Tue, Jun 24, 2014 at 10:49:34AM +0200, Lukas Slebodnik wrote:
>> On (23/06/14 19:36), Jakub Hrozek wrote:
>> >Hi,
>> >
>> >When I was reviewing PavelB's duplicate members patch, I realized our
>> >nested group unit tests could use some improvements. Please see the
>> >attached patches and their commit messages.
>> 
>> >Subject: [PATCH 3/6] TESTS: Fix group search base
>> >
>> >After fixing the confdb initialization I realized the group DN couldn't
>> >be parsed. This patch fixes that.
>> >---
>> > src/tests/cmocka/test_nested_groups.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/src/tests/cmocka/test_nested_groups.c b/src/tests/cmocka/test_nested_groups.c
>> >index 9f38e3dd388dcafcdaa61cb39136c30fa58b399a..bf678c2b2025e54e6ed50b8b33fe41a78ffec007 100644
>> >--- a/src/tests/cmocka/test_nested_groups.c
>> >+++ b/src/tests/cmocka/test_nested_groups.c
>> >@@ -43,7 +43,7 @@
>> > /* put users and groups under the same container so we can easily run the
>> >  * same tests cases for several search base scenarios */
>> > #define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
>> >-#define GROUP_BASE_DN "cn=groups" OBJECT_BASE_DN
>> >+#define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
>> Could you used constant from SYSDB_GROUPS_CONTAINER from sysdb.h?
>
>It wouldn't be the right thing to do, SYSDB_GROUPS_CONTAINER is a cache
>base DN, we are simulating the original LDAP DN with this test.
>
Sorry, My fault.
I should have noticed OBJECT_BASE_DN is not from sysdb.

>> >From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17 00:00:00 2001
>> >From: Jakub Hrozek <jhrozek at redhat.com>
>> >Date: Mon, 23 Jun 2014 19:07:59 +0200
>> >Subject: [PATCH 6/6] TESTS: Add more tests for nested groups processing
>> >
>> >Adds unit test for basic group retrieval functionality as well as for
>> >testing duplicate members in the LDAP group entry.
>> Could you add reference (git hash) to patch with PB's fix?
>
>OK.
>
Thank you

>> 
>> >---
>> > src/tests/cmocka/test_nested_groups.c | 130 +++++++++++++++++++++++++++++++++-
>> > 1 file changed, 129 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/src/tests/cmocka/test_nested_groups.c b/src/tests/cmocka/test_nested_groups.c
>> >index bf678c2b2025e54e6ed50b8b33fe41a78ffec007..693d0fb33837643566d7a9ab0dc02bd5cff9c849 100644
>> >--- a/src/tests/cmocka/test_nested_groups.c
>> >+++ b/src/tests/cmocka/test_nested_groups.c
>> >@@ -44,6 +44,7 @@
>> >  * same tests cases for several search base scenarios */
>> > #define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
>> > #define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
>> >+#define USER_BASE_DN "cn=users," OBJECT_BASE_DN
>> Could you used constant from SYSDB_USERS_CONTAINER from sysdb.h?
>
>See above.
>
>> >+    /* check return code */
>> >+    assert_int_equal(ret, ERR_OK);
>>                             ^^^^^^
>> >+
>> >+    /* Check the users */
>> >+    assert_int_equal(test_ctx->num_users, 1);
>> >+    assert_int_equal(test_ctx->num_groups, 1);
>> >+
>> >+    ret = sysdb_attrs_get_string(test_ctx->users[0], SYSDB_NAME, &name);
>> >+    assert_int_equal(ret, EOK);
>>                             ^^^
>> You mixed EOK and ERR_OK. According to git history, ERR_OK is newer.
>> We should decide and use just one in next patches.
>
>OK, converted to ERR_OK to maintain the same code in the test.
>
>> 
>> 
>> 
>> >+    assert_string_equal(name, "user1");
>> >+}
>> >+
>> > void nested_groups_test_setup(void **state)
>> > {
>> >     struct nested_groups_test_ctx *test_ctx = NULL;
>> >     static struct sss_test_conf_param params[] = {
>> >         { "ldap_schema", "rfc2307bis" }, /* enable nested groups */
>> >+        { "ldap_search_base", OBJECT_BASE_DN },
>> >+        { "ldap_user_search_base", USER_BASE_DN },
>> >+        { "ldap_group_search_base", GROUP_BASE_DN },
>> >         { NULL, NULL }
>> >     };
>> > 
>> >@@ -163,7 +289,9 @@ int main(int argc, const char *argv[])
>> >     };
>> > 
>> >     const UnitTest tests[] = {
>> >-        new_test(one_group_no_members)
>> >+        new_test(one_group_no_members),
>> >+        new_test(one_group_unique_members),
>> >+        new_test(one_group_dup_users)
>> >     };
>> > 
>> >     /* Set debug level to invalid value so we can deside if -d 0 was used. */
>> 
>> I tested with reverted aptch "nested groups: do not fail if we get one entry
>> twice" and unit test fail. It's good.
>> 
>> PB modified function sdap_nested_group_single_step_process on two places
>>     1) in case SDAP_NESTED_GROUP_DN_USER
>>     2) in case SDAP_NESTED_GROUP_DN_GROUP
>> 
>> Unit test covers only the first part. Do we want ot also test the 2nd one?
>
>Yes, I added two more test cases.
Both cases are covered now.

Almost ACK.

>From 8479a0a88e80c7025b411d6390276b6c97f3488f Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Mon, 23 Jun 2014 19:07:59 +0200
>Subject: [PATCH 6/6] TESTS: Add more tests for nested groups processing
>
>Adds unit test for basic group retrieval functionality as well as for
>testing duplicate members in the LDAP group entry.
>
>These unit tests exercise code added in patch
>a47cb2e08e4004179d2a6b5f9a9340200270fbd0
>---
> src/responder/ifp/ifp_iface_generated.c | 460 ++++++++++++++++----------------
This file should not be in this patch.
It if a know bug in code generation #2344

> src/tests/cmocka/test_nested_groups.c   | 266 +++++++++++++++++-
> 2 files changed, 495 insertions(+), 231 deletions(-)
>

LS



More information about the sssd-devel mailing list