[SSSD] [PATCH] TEST: recent_valid filter testing

Petr Cech pcech at redhat.com
Tue Nov 10 14:59:03 UTC 2015


On 11/09/2015 04:28 PM, Jakub Hrozek wrote:
> On Thu, Nov 05, 2015 at 05:29:25PM +0100, Petr Cech wrote:
>> >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?
> Are these removed tests? In general for the wildcard test you're right
> that we should try to add users into multiple domains..

These tests are next removed tests. You can see them here:
https://git.fedorahosted.org/cgit/sssd.git/commit/?id=bdf422fde0fd6b40b3412bad3b200f8fd7ea8693

They have broken logic like all removed in this ticket. But there is 
something more in their names is 'mulitple_domains' but I don't see that 
they actually work over multiple domains.

>
>> > From c380f1c1df0e891e89bded8e68eefb53a05131c7 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/7] TEST: Add test_user_by_recent_filter_valid
> ACK except the call to sysdb_store_user() overflows 80 characters.
Fixed.

>
>> > From ab97caaf39b3b65416fa8a4a72a104a9a0391a48 Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Wed, 4 Nov 2015 06:50:33 -0500
>> >Subject: [PATCH 2/7] TEST: Refactor of test_responder_cache_req.c
> ACK
>
>> > From 21f44a019f2e4fcd97d3f3d25f4ab2ddedd1b93e 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 3/7] 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.
> ACK
>
>> > From beb13c2bd236355666127f8476966b29e6f33295 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 4/7] TEST: Add test_users_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 users_by_recent_filter_valid().
>> >
>> >Resolves:
>> >https://fedorahosted.org/sssd/ticket/2730
>> >---
>> >  src/tests/cmocka/test_responder_cache_req.c | 88 ++++++++++++++++++++++++++++-
>> >  1 file changed, 87 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
>> >index e5f92329fd06019eaccc18aa57756e59b72e815c..c25256a9d4685d66a818512e9d46edcacbdc65bd 100644
>> >--- a/src/tests/cmocka/test_responder_cache_req.c
>> >+++ b/src/tests/cmocka/test_responder_cache_req.c
>> >@@ -113,6 +113,27 @@ struct cli_protocol_version *register_cli_protocol_version(void)
>> >      return version;
>> >  }
>> >
>> >+/* Return true if all values are in ldb_results */
>> >+static bool are_values_in_ldb_result(const char **ldb_results,
>> >+                                     const char **values)
> Nice, thanks!
Thanks.

>
>> >+{
>> >+    bool is_value_in_result = false;
>> >+    bool is_value_in_results = false;
>> >+    bool result = true;
>> >+
>> >+    for (unsigned int i = 0; i < talloc_array_length(values); ++i) {
> It's just a tradition, but we tend to prefer postfix instead of prefix.
>
>> >+        is_value_in_results = false;
>> >+        for (size_t j = 0; j < talloc_array_length(ldb_results); ++j) {
> Please choose size_t or unsigned int for both (unless there is a reason
> not to).
Fixed.

>
>> >+            is_value_in_result = strcmp(ldb_results[i], values[j]) == 0 ? \
>> >+                                 true : false;
>> >+            is_value_in_results = is_value_in_results || is_value_in_result;
>> >+        }
>> >+        result = result && is_value_in_results;
>> >+    }
>> >+
>> >+    return result;
>> >+}
>> >+
> It would be better to split this function into a separate patch and add
> it to the common test code, it is generally useful, not just for our
> tests.
>
> But I would also like another version of this function that doesn't rely
> on talloc. Then the shorthand with talloc_array_lenght could use the
> generic function:
>      static bool are_values_in_ldb_result(const char **ldb_results,
>                                           size_t results_len,
>                                           const char **values,
>                                           size_t values_len);
>
>      #define tc_are_values_in_ldb_result(ldb_results, values)              \
>              are_values_in_ldb_result(ldb_results,                         \
>                                       talloc_array_length(ldb_results),    \
>                                       values,                              \
>                                       talloc_array_length(values))
>
> (I haven't tried this at all, if it's not possible or practical for some
> reason, just push back please)
Fixed.

> Another useful wrapper might accept just struct ldb_result and find the
> attribute like you do find SYSDB_NAME in the test.
It could be done in next patch set.

>> > From c464211d04e0dd5faff3a69e21808ca23d091309 Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Sun, 1 Nov 2015 07:09:28 -0500
>> >Subject: [PATCH 5/7] TEST: Add test_group_by_recent_filter_valid
> ACK
>
>> > From 60883b72a6f2d46ca1a20b335ce5658afca327c2 Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Sun, 1 Nov 2015 07:21:18 -0500
>> >Subject: [PATCH 6/7] TEST: Refactor of test_responder_cache_req.c
> ACK
>
>> > From b84ebcdfbe5786b5ac1e641cefcd3a20cc3c509a Mon Sep 17 00:00:00 2001
>> >From: Petr Cech<pcech at redhat.com>
>> >Date: Sun, 1 Nov 2015 07:45:56 -0500
>> >Subject: [PATCH 7/7] TEST: Add test_groups_by_recent_filter_valid
> Mostly ack..
Thanks for review.

>> >+    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, 2);
>> >+
>> >+    tmp_ctx = talloc_zero(NULL, void *);
> you can use test_ctx instead of NULL here. Normally we only use NULL
> context to be able to run valgrind and see the leaks, but we might be
> moving away from this pattern soon, so please just test_ctx in the
> tests...
I refactored it by Pavels opinions.

Petr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-TEST-Add-test_user_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4164 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151110/28d808ba/attachment-0008.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/20151110/28d808ba/attachment-0009.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: 5878 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151110/28d808ba/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-TEST-Add-common-function-are_values_in_array.patch
Type: text/x-patch
Size: 2391 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151110/28d808ba/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-TEST-Add-test_users_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4785 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151110/28d808ba/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-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/20151110/28d808ba/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-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/20151110/28d808ba/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-TEST-Add-test_groups_by_recent_filter_valid.patch
Type: text/x-patch
Size: 4523 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151110/28d808ba/attachment-0015.bin>


More information about the sssd-devel mailing list