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.
>From bc8fd57de30abad8c2b5e74def22927bb2870830 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Mon, 23 Jun 2014 17:50:45 +0200
>Subject: [PATCH 1/6] TESTS: Add confdb domain base DN to sss_test_ctx
>
>Creation of the path to the domain's confdb entry was duplicated in the
>tests. Rather than adding yet another duplication, I added the path as
>another field of the sss_test_ctx structure.
>---
From patch, it is clean that it was not used on many places.
It will be usefull in future.
ACK
>From 532cee578213b3d0d9d13229c72adefeb126f8cf Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Mon, 23 Jun 2014 17:52:03 +0200
>Subject: [PATCH 2/6] TESTS: Use the right confdb path
>
>The nested group test only worked by accident. Its confdb settings were
>not applied because a wrong confdb path was used.
>---
ACK
>From 09ca16557b3eb556f38a959d1749160c42a33578 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Mon, 23 Jun 2014 18:07:51 +0200
>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.
>From 0cd01ebe4f7f61e830cb06c907b04ff07200dbd1 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Mon, 23 Jun 2014 19:06:52 +0200
>Subject: [PATCH 4/6] TESTS: Do not require replies from mocked
> sdap_get_generic_recv to be talloc contexts
>
>While it's beneficial for the real implementation of
>sdap_get_generic_recv() to move memory around, the mocked implementation
>can just pass around pointer.
>---
make sense.
ACK
>From 67a524e5ce1b7c48b99104a8726b18a090550ae4 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)redhat.com>
>Date: Mon, 23 Jun 2014 19:07:41 +0200
>Subject: [PATCH 5/6] TESTS: Change how mock_sysdb_user() is implemented
>
>For the purpose of unit tests, it's better to create a user object with
>a UID and a name.
>---
ACK
>From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek(a)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.
>---
> 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.