[SSSD] [PATCH] TEST: recent_valid filter testing

Jakub Hrozek jhrozek at redhat.com
Wed Nov 4 10:11:41 UTC 2015


On Tue, Oct 27, 2015 at 09:42:20AM +0100, Petr Cech wrote:
> On 10/23/2015 12:57 PM, Jakub Hrozek wrote:
> >Thank you, I think your approach is correct. Your test essentially tests
> >that testuser2 was on the server but was removed, so only testuser1 is
> >returned.
> >
> >It's correct, but because the interface is able to return more users, I
> >would prefer if we tested that as well.
> >
> >I have one more minor remark inline, but in general, please go
> >ahead and add back the other tests..
> 
> Hello Jakub and everyone!
> 
> The first patch set is attached.
> 
> The removed tests were:
>  * users_by_filter_valid
>  * users_by_filter_multiple_domains_valid
>  * groups_by_filter_valid
>  * groups_by_filter_multiple_domains_valid
> 
> This patch set covers users_by_filter_valid by two new tests:
>  * user_by_recent_filter_valid
>  * users_by_recent_filter_valid
> 
> The first of them tests the recent filter. The seconds tests interface
> ability to return more users.
> 
> Regards,
> 
> Petr

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)


More information about the sssd-devel mailing list