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@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@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@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@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@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@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.