[PATCH] TESTS: Removing part of responder_cache_req-tests

[Petr Cech]

Hi,

there is patch, which removes failing tests from responder_cache_req suite.
The reasons are mentioned in commit message.
There will be another patch, which will rewrite those tests.

Petr

[Michal Židek]

On 08/21/2015 02:18 PM, Petr Cech wrote:
> Hi,
>
> there is patch, which removes failing tests from responder_cache_req suite.
> The reasons are mentioned in commit message.
> There will be another patch, which will rewrite those tests.
>
> Petr

Hi,

some of the tests you deleted are valid and
should not be deleted.

Only those tests that rely on time(NULL)
being the same as the time of request
creation are invalid. All those that test old
entries or nonexistent entries are OK.
See comments inline.
> 0001-TESTS-Removing-part-of-responder_cache_req-tests.patch
>
> From 4518350e6dc7ddebad5b4c5d6ac2c560033e91ab Mon Sep 17 00:00:00 2001
> From: Petr Cech
> Date: Fri, 21 Aug 2015 13:57:58 +0200
> Subject: [PATCH] TESTS: Removing part of responder_cache_req-tests
>
> If you call cache_req_[user|group]_by_filter_send() it than later calls
> updated_[users|groups]_by_filter(), which adds filter that is called
> "recent". This filter causes that only [users|groups] added after the
> request started are returned.
>
> This patch removes tests which use
> cache_req_[user|group]_by_filter_send(), because the logic of those
> tests is corrupted. The tests create [users|groups] and after it, they
> call cache_req_[user|group]_by_filter_send(). So it is obvious that it
> is not in the right manner.
>
> Possible fix is rewrite the tests to create the entries in the callback.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2730
>
> src/tests/cmocka/test_responder_cache_req.c |  383 ---------------------------
>   1 files changed, 0 insertions(+), 383 deletions(-)
>
> diff --git a/src/tests/cmocka/test_responder_cache_req.c b/src/tests/cmocka/test_responder_cache_req.c
> index 032fe429ac88b8cc9113976329ea04837f287276..a6fe6737513f9436b1cd4e8f0cd9e6a3867f5105 100644
> --- a/src/tests/cmocka/test_responder_cache_req.c
> +++ b/src/tests/cmocka/test_responder_cache_req.c
> @@ -1710,217 +1710,6 @@ static void cache_req_user_by_filter_test_done(struct tevent_req *req)
>       ctx->tctx->done = true;
>   }
>
> -void test_users_by_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);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    test_ctx->tctx->dom->name,
>                                    "test*");
> 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, 2);
> 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_NAME2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_USER_NAME);
>
> -}
>
> -void test_users_by_filter_filter_old(void **state)

This test seems valid to me.
> -{
> 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;
> /* This user was updated in distant past, so it wont't be reported by
> * the filter search */
> 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, 1);
> assert_int_equal(ret, EOK);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    test_ctx->tctx->dom->name,
>                                    "test*");
> 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);
>
> -}
>
> -void test_users_by_filter_notfound(void **state)

This test looks valid to me as well. Did you see it falling?
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> TALLOC_CTX *req_mem_ctx = NULL;
> struct tevent_req *req = NULL;
> errno_t ret;
> test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    test_ctx->tctx->dom->name,
>                                    "nosuchuser*");
> 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, ENOENT);
> assert_true(check_leaks_pop(req_mem_ctx));
>
> -}
>
> -static void test_users_by_filter_multiple_domains_valid(void **state)
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> struct sss_domain_info *domain = 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);
> domain = find_domain_by_name(test_ctx->tctx->dom,
>                             "responder_cache_req_test_d", true);
> assert_non_null(domain);
> ret = sysdb_store_user(domain, TEST_USER_NAME, "pwd", 1000, 1000,
>                       NULL, NULL, NULL, "cn="TEST_USER_NAME",dc=test", NULL,
>                       NULL, 1000, time(NULL));
> assert_int_equal(ret, EOK);
> ret = sysdb_store_user(domain, 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);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    domain->name,
>                                    "test*");
> 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, 2);
> 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_NAME2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_USER_NAME);
>
> -}
>
> -static void test_users_by_filter_multiple_domains_notfound(void **state)

This one is also valid.
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> struct sss_domain_info *domain = NULL;
> TALLOC_CTX *req_mem_ctx = NULL;
> struct tevent_req *req = NULL;
> errno_t ret;
> test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> domain = find_domain_by_name(test_ctx->tctx->dom,
>                             "responder_cache_req_test_d", true);
> assert_non_null(domain);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_user_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    domain->name,
>                                    "nosuchuser*");
> 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, ENOENT);
> assert_true(check_leaks_pop(req_mem_ctx));
>
> -}
> static void cache_req_group_by_filter_test_done(struct tevent_req *req)
> {
>     struct cache_req_test_ctx *ctx = NULL;
>
> @@ -1934,168 +1723,6 @@ static void cache_req_group_by_filter_test_done(struct tevent_req *req)
>       ctx->tctx->done = true;
>   }
>
> -void test_groups_by_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_group = true;
> ret = sysdb_store_group(test_ctx->tctx->dom, TEST_GROUP_NAME2,
>                        1001, NULL, 1001, time(NULL));
> assert_int_equal(ret, EOK);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                     test_ctx->rctx,
>                                     test_ctx->tctx->dom->name,
>                                     "test*");
> assert_non_null(req);
> tevent_req_set_callback(req, cache_req_group_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, 2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_GROUP_NAME2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_GROUP_NAME);
>
> -}
>
> -void test_groups_by_filter_notfound(void **state)

This one is aldo valid.
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> TALLOC_CTX *req_mem_ctx = NULL;
> struct tevent_req *req = NULL;
> errno_t ret;
> test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    test_ctx->tctx->dom->name,
>                                    "nosuchgroup*");
> assert_non_null(req);
> tevent_req_set_callback(req, cache_req_group_by_filter_test_done, test_ctx);
> ret = test_ev_loop(test_ctx->tctx);
> assert_int_equal(ret, ENOENT);
> assert_true(check_leaks_pop(req_mem_ctx));
>
> -}
>
> -void test_groups_by_filter_multiple_domains_valid(void **state)
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> struct sss_domain_info *domain = 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);
> domain = find_domain_by_name(test_ctx->tctx->dom,
>                             "responder_cache_req_test_d", true);
> assert_non_null(domain);
> ret = sysdb_store_group(domain, TEST_GROUP_NAME,
>                        1000, NULL, 1000, time(NULL));
> assert_int_equal(ret, EOK);
> ret = sysdb_store_group(domain, TEST_GROUP_NAME2,
>                        1001, NULL, 1001, time(NULL));
> assert_int_equal(ret, EOK);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                     test_ctx->rctx,
>                                     domain->name,
>                                     "test*");
> assert_non_null(req);
> tevent_req_set_callback(req, cache_req_group_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, 2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[0],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_GROUP_NAME2);
> ldbname = ldb_msg_find_attr_as_string(test_ctx->result->msgs[1],
>                                      SYSDB_NAME, NULL);
> assert_non_null(ldbname);
> assert_string_equal(ldbname, TEST_GROUP_NAME);
>
> -}
>
> -void test_groups_by_filter_multiple_domains_notfound(void **state)

This one is also valid test.
> -{
> struct cache_req_test_ctx *test_ctx = NULL;
> struct sss_domain_info *domain = NULL;
> TALLOC_CTX *req_mem_ctx = NULL;
> struct tevent_req *req = NULL;
> errno_t ret;
> test_ctx = talloc_get_type_abort(*state, struct cache_req_test_ctx);
> domain = find_domain_by_name(test_ctx->tctx->dom,
>                             "responder_cache_req_test_d", true);
> assert_non_null(domain);
> req_mem_ctx = talloc_new(global_talloc_context);
> 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();
> req = cache_req_group_by_filter_send(req_mem_ctx, test_ctx->tctx->ev,
>                                    test_ctx->rctx,
>                                    domain->name,
>                                    "nosuchgroup*");
> assert_non_null(req);
> tevent_req_set_callback(req, cache_req_group_by_filter_test_done, test_ctx);
> ret = test_ev_loop(test_ctx->tctx);
> assert_int_equal(ret, ENOENT);
> assert_true(check_leaks_pop(req_mem_ctx));
>
> -}
> int main(int argc, const char *argv[])
> {
>     poptContext pc;
>
> @@ -2144,16 +1771,6 @@ int main(int argc, const char *argv[])
>           new_single_domain_test(group_by_id_missing_notfound),
>           new_multi_domain_test(group_by_id_multiple_domains_found),
>           new_multi_domain_test(group_by_id_multiple_domains_notfound),
>    new_single_domain_test(users_by_filter_valid),
>    new_single_domain_test(users_by_filter_filter_old),
>    new_single_domain_test(users_by_filter_notfound),
>    new_multi_domain_test(users_by_filter_multiple_domains_valid),
>    new_multi_domain_test(users_by_filter_multiple_domains_notfound),
>    new_single_domain_test(groups_by_filter_valid),
>    new_single_domain_test(groups_by_filter_notfound),
>    new_multi_domain_test(groups_by_filter_multiple_domains_valid),
>    new_multi_domain_test(groups_by_filter_multiple_domains_notfound),
> };
>
> /* Set debug level to invalid value so we can deside if -d 0 was used. */
>
> -- 1.7.1

[Petr Cech]

On 08/21/2015 02:35 PM, Michal Židek wrote:
> Hi,
>
> some of the tests you deleted are valid and
> should not be deleted.
>
> Only those tests that rely on time(NULL)
> being the same as the time of request
> creation are invalid. All those that test old
> entries or nonexistent entries are OK.
> See comments inline.

I agree.
Those tests have another logic. So I returned them back.
Petr

[Michal Židek]

On 08/21/2015 04:55 PM, Petr Cech wrote:
> On 08/21/2015 02:35 PM, Michal Židek wrote:
> > Hi,
> >
> > some of the tests you deleted are valid and
> > should not be deleted.
> >
> > Only those tests that rely on time(NULL)
> > being the same as the time of request
> > creation are invalid. All those that test old
> > entries or nonexistent entries are OK.
> > See comments inline.
>
> I agree.
> Those tests have another logic. So I returned them back.
> Petr

ACK.

CI results:
http://sssd-ci.duckdns.org/logs/job/23/75/summary.html

Michal

[Lukas Slebodnik]

On (24/08/15 16:09), Michal Židek wrote:
> On 08/21/2015 04:55 PM, Petr Cech wrote:
> > On 08/21/2015 02:35 PM, Michal Židek wrote:
> > > Hi,
> > >
> > > some of the tests you deleted are valid and
> > > should not be deleted.
> > >
> > > Only those tests that rely on time(NULL)
> > > being the same as the time of request
> > > creation are invalid. All those that test old
> > > entries or nonexistent entries are OK.
> > > See comments inline.
> >
> > I agree.
> > Those tests have another logic. So I returned them back.
> > Petr
>
> ACK.
>
> CI results:
> http://sssd-ci.duckdns.org/logs/job/23/75/summary.html

+1 for patch.

but please do not close the ticket #2730
after pushing to master. This solution is just a temporary
workaround for problematic tests.

LS

[Jakub Hrozek]

On Thu, Aug 27, 2015 at 01:05:00PM +0200, Lukas Slebodnik wrote:
> On (24/08/15 16:09), Michal Židek wrote:
> > On 08/21/2015 04:55 PM, Petr Cech wrote:
> > > On 08/21/2015 02:35 PM, Michal Židek wrote:
> > > > Hi,
> > > >
> > > > some of the tests you deleted are valid and
> > > > should not be deleted.
> > > >
> > > > Only those tests that rely on time(NULL)
> > > > being the same as the time of request
> > > > creation are invalid. All those that test old
> > > > entries or nonexistent entries are OK.
> > > > See comments inline.
> > >
> > > I agree.
> > > Those tests have another logic. So I returned them back.
> > > Petr
> >
> > ACK.
> >
> > CI results:
> > http://sssd-ci.duckdns.org/logs/job/23/75/summary.html
>
> +1 for patch.
>
> but please do not close the ticket #2730

OK, I changed "Resolves" to "Works Around" in the commit message and
pushed the patch to master:
    bdf422fde0fd6b40b3412bad3b200f8fd7ea8693
> after pushing to master. This solution is just a temporary
> workaround for problematic tests.

What do you think would be the solution? I thought Michal and Petr
concluded that the tests were simply invalid..

[Michal Židek]

On 08/31/2015 06:29 PM, Jakub Hrozek wrote:
> On Thu, Aug 27, 2015 at 01:05:00PM +0200, Lukas Slebodnik wrote:
> > On (24/08/15 16:09), Michal Židek wrote:
> > > On 08/21/2015 04:55 PM, Petr Cech wrote:
> > > > On 08/21/2015 02:35 PM, Michal Židek wrote:
> > > > > Hi,
> > > > >
> > > > > some of the tests you deleted are valid and
> > > > > should not be deleted.
> > > > >
> > > > > Only those tests that rely on time(NULL)
> > > > > being the same as the time of request
> > > > > creation are invalid. All those that test old
> > > > > entries or nonexistent entries are OK.
> > > > > See comments inline.
> > > >
> > > > I agree.
> > > > Those tests have another logic. So I returned them back.
> > > > Petr
> > >
> > > ACK.
> > >
> > > CI results:
> > > http://sssd-ci.duckdns.org/logs/job/23/75/summary.html
> >
> > +1 for patch.
> >
> > but please do not close the ticket #2730
>
> OK, I changed "Resolves" to "Works Around" in the commit message and
> pushed the patch to master:
>      bdf422fde0fd6b40b3412bad3b200f8fd7ea8693
> > after pushing to master. This solution is just a temporary
> > workaround for problematic tests.
>
> What do you think would be the solution? I thought Michal and Petr
> concluded that the tests were simply invalid..

The solution is to replace them with valid test. Currently
we do not have properly covered some of the functionality
in tests (we though it is covered, but the tests were just not
valid and did not reflect how the functions are used).

Michal