[SSSD] [PATCH] TEST: recent_valid filter testing

Petr Cech pcech at redhat.com
Thu Nov 5 16:29:25 UTC 2015


On 11/04/2015 11:11 AM, Jakub Hrozek wrote:
> Hi,
>
> Sorry it took so long to get back to the review.  I only have some minor
> comments, see inline..
>
> Because the group patches are more or less equivalent, I'll just comment
> here. If you agree with the comments, please also change the group tests
> and resend in a single set.
>
> Thanks for the tests!
>
>> > From e3dd543eec09f6e4386bfe6f1505538575fe5356 Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Fri, 2 Oct 2015 07:34:08 -0400
>> >Subject: [PATCH 1/3] TEST: Add test_user_by_recent_filter_valid
>> >
>> >Test users_by_filter_valid() was removed in past. We will add two new
>> >tests instead of it. Logic of those tests is connected to RECENT
>> >filter. It returns only records which have been wrote or updated after
>> >filter was created (or another given time).
>> >
>> >users_by_filter_valid() --> user_by_recent_filter_valid()
>> >                             users_by_recent_filter_valid()
>> >
>> >The first of new tests, user_by_recent_filter_valid(), counts with two
>> >users. One is stored before filter request creation and the second user
>> >is stored after filter request creation. So filter returns only one
>> >user.
>> >
>> >The second of new tests, users_by_recent_filter_valid(), counts with
>> >three users. One is stored before filter request creation and two users
>> >are stored after filter request creation. So filter returns two users.
>> >
>> >This patch adds user_by_recent_filter_valid().
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/2730
>> >---
>> >  src/tests/cmocka/test_responder_cache_req.c | 50 +++++++++++++++++++++++++++++
>> >  1 file changed, 50 insertions(+)
>> >
>> >diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>> >index 744c8f4a8f7aa4e08f82aca5aea003438b5b59da..3379b17f7feea521966d6c8646afd9859a3c5255 100644
>> >--- a/src/tests/cmocka/test_responder_cache_req.c
>> >+++ b/src/tests/cmocka/test_responder_cache_req.c
>> >@@ -1239,6 +1239,53 @@ static void cache_req_user_by_filter_test_done(struct tevent_req *req)
>> >      ctx->tctx->done = true;
>> >  }
>> >
>> >+void test_user_by_recent_filter_valid(void **state)
>> >+{
>> >+    struct cache_req_test_ctx *test_ctx = NULL;
>> >+    TALLOC_CTX *req_mem_ctx = NULL;
>> >+    struct tevent_req *req = NULL;
>> >+    const char *ldbname = NULL;
>> >+    errno_t ret;
>> >+
>> >+    test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
>> >+    test_ctx->create_user = true;
>> >+
>> >+    ret = sysdb_store_user(test_ctx->tctx->dom, TEST_USER_NAME2, "pwd", 1001, 1001,
>> >+                           NULL, NULL, NULL, "cn="TEST_USER_NAME2",dc=test", NULL,
>> >+                           NULL, 1000, time(NULL));
>> >+    assert_int_equal(ret, EOK);
>> >+
>> >+    sleep(1);
> The purpose of the sleep() here is just to make sure the entry was
> created in the past, right? Would it be equally safe to create the user
> with timestamp time(NULL)-1 to make the test faster?
>
>> >+
>> >+    req_mem_ctx = talloc_new(test_ctx->tctx);
>> >+    check_leaks_push(req_mem_ctx);
>> >+
>> >+    /* Filters always go to DP */
>> >+    will_return(__wrap_sss_dp_get_account_send, test_ctx);
>> >+    mock_account_recv_simple();
> Can you add a comment that the TEST_USER is created with a DP callback
> here?
>
>> >+
>> >+    req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>> >+                                        test_ctx->rctx,
>> >+                                        test_ctx->tctx->dom->name,
>> >+                                        "test*");
> It would read nicer if we had a constant TEST_USER_PREFIX "test_user" #defined,
> or even TEST_USER_FILTER with the asterist.
>
>> >+    assert_non_null(req);
>> >+
>> >+    tevent_req_set_callback(req, cache_req_user_by_filter_test_done, test_ctx);
>> >+
>> >+    ret = test_ev_loop(test_ctx->tctx);
>> >+    assert_int_equal(ret, ERR_OK);
>> >+    assert_true(check_leaks_pop(req_mem_ctx));
>> >+
>> >+    assert_non_null(test_ctx->result);
>> >+    assert_int_equal(test_ctx->result->count, 1);
>> >+
>> >+    ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>> >+                                          SYSDB_NAME, NULL);
>> >+    assert_non_null(ldbname);
>> >+    assert_string_equal(ldbname, TEST_USER_NAME);
>> >+}
>> > From c2e87544dfbc0667e1b935394d697322b34dddeb Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Tue, 27 Oct 2015 03:53:18 -0400
>> >Subject: [PATCH 2/3] TEST: Refactor of test_responder_cache_req.c
>> >
>> >We need little more in background of responder_cache_req tests. There
>> >will be tests which will use three test users. This patch add support
>> >for it.
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/2730
>> >---
>> >  src/tests/cmocka/test_responder_cache_req.c | 59 +++++++++++++++++++++--------
>> >  1 file changed, 44 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>> >index 3379b17f7feea521966d6c8646afd9859a3c5255..a457d5f277314b75cd73b1306995665456df7f9d 100644
>> >--- a/src/tests/cmocka/test_responder_cache_req.c
>> >+++ b/src/tests/cmocka/test_responder_cache_req.c
>> >@@ -39,8 +39,15 @@
>> >  #define TEST_GROUP_NAME "test-group"
>> >  #define TEST_GROUP_ID 1000
>> >
>> >-#define TEST_USER_NAME2 "test-user2"
>> >-#define TEST_GROUP_NAME2 "test-group2"
>> >+#define TEST_USER_ID2 1001
>> >+#define TEST_USER_NAME2 "test_user2"
>> >+#define TEST_GROUP_NAME2 "test_group2"
>> >+#define TEST_GROUP_ID2 1001
>> >+
>> >+#define TEST_USER_ID3 1002
>> >+#define TEST_USER_NAME3 "test_user3"
>> >+#define TEST_GROUP_NAME3 "test_group3"
>> >+#define TEST_GROUP_ID3 1002
>> >
>> >  #define new_single_domain_test(test) \
>> >      cmocka_unit_test_setup_teardown(test_ ## test, \
>> >@@ -82,7 +89,8 @@ struct cache_req_test_ctx {
>> >      struct sss_domain_info *domain;
>> >      char *name;
>> >      bool dp_called;
>> >-    bool create_user;
>> >+    bool create_user1;
>> >+    bool create_user2;
> With just two booleans this might be OK, but if we end up adding more
> users, then additional booleans might get clunky and maybe something
> like a bitshift where the DP callback would test for a particular bit
> set might be more generic? Can you add a comment so that the
> next person touching the code won't add another boolean?
>
>> > From 99e1932b876186e702fcb20b6d0bdd5c7e07e1ba Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Tue, 27 Oct 2015 04:02:46 -0400
>> >Subject: [PATCH 3/3] TEST: Add test_users_by_recent_filter_valid
> [...]
>
>> >+    assert_non_null(test_ctx->result);
>> >+    assert_int_equal(test_ctx->result->count, 2);
>> >+
>> >+    ldbname1 = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>> >+                                           SYSDB_NAME, NULL);
>> >+    assert_non_null(ldbname1);
>> >+
>> >+    ldbname2 = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>> >+                                           SYSDB_NAME, NULL);
>> >+    assert_non_null(ldbname2);
>> >+
>> >+    assert_string_not_equal(ldbname1, ldbname2);
>> >+    comparison1 = strcmp(ldbname1, TEST_USER_NAME) == 0 ? true : false;
>> >+    comparison2 = strcmp(ldbname1, TEST_USER_NAME2) == 0 ? true : false;
>> >+    comparison3 = strcmp(ldbname2, TEST_USER_NAME) == 0 ? true : false;
>> >+    comparison4 = strcmp(ldbname2, TEST_USER_NAME2) == 0 ? true : false;
>> >+    assert_true((comparison1 || comparison2) && (comparison3 || comparison4));
>> >+}
> Can you extract this part into a common function that would accept a
> ldb_result pointer and two usernames and would make sure they are in the
> ldb_messages? The same pattern is used in the group tests.
>
> (I wonder if that could also be written in a nice and generic way for N
> usernames in a string array, but it's not important at the moment)

Hello Jakub,

I addresed all hints. There are fixed patch set attached.

Regards,

Petr

PS: I am still not sure what was the purpose of
   * users_by_filter_multiple_domains_valid
   * groups_by_filter_multiple_domains_valid
tests. How I wrote earlier: "I am afraid I misunderstood their purpose. 
Because users/groups are set with the same domains." Please, do you 
remember their purpose?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TEST-Add-test_user_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4136 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 2046 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 5916 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-TEST-Add-test_users_by_recent_filter_valid.patch
Type: text/x-patch
Size: 5744 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-TEST-Add-test_group_by_recent_filter_valid.patch
Type: text/x-patch
Size: 3760 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-TEST-Refactor-of-test_responder_cache_req.c.patch
Type: text/x-patch
Size: 3120 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-TEST-Add-test_groups_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4521 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151105/353a0395/attachment-0013.bin>


More information about the sssd-devel mailing list