On Thu, Jan 02, 2014 at 11:48:48PM +0545, Pallavi Jha wrote:
> Hi,
>
> I have attached the test patch for negcache.c module along with the mail.
>
> I could not write test for the last function sss_ncache_prepopulate(). I
> have doubt regarding the initialization of the arguments to be passed to
> sss_ncache_prepopulate(). Please review the patch.
>
>
>
>
> Thanks,
> Pallavi

Hi Pallavi,

this is a great start. I have some comments on improving the test, see
below.

First, the patch needs rebasing on top of origin/master as it conflicts
with your authtok patch.

> From c1cc80745496c08ff31cd2fade0b306606199801 Mon Sep 17 00:00:00 2001
> From: Pallavi Jha <pallavikumarijha@gmail.com>
> Date: Tue, 31 Dec 2013 18:04:16 +0530
> Subject: [PATCH] cmocka unit test for negcache module added

[snip]

> +/*
> + SSSD
> +
> +     NSS Responder
> +
> +     Authors:
> +         Pallavi Jha <pallavikumarijha@gmail.com>
> +
> +         Copyright (C) 2013 Red Hat
> +
> +         This program is free software; you can redistribute it and/or modify
> +         it under the terms of the GNU General Public License as published by
> +         the Free Software Foundation; either version 3 of the License, or
> +         (at your option) any later version.
> +
> +         This program is distributed in the hope that it will be useful,
> +         but WITHOUT ANY WARRANTY; without even the implied warranty of
> +         MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +         GNU General Public License for more details.
> +         You should have received a copy of the GNU General Public License
> +         along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <inttypes.h>
> +#include <cmocka.h>
> +
> +#include "util/util.h"
> +#include "responder/common/responder.h"
> +#include "responder/common/negcache.h"
> +
> +#define PORT 21
> +#define SID "10001"

It doesn't really matter in the context of the test, but maybe it would
be more readable to use something that resembles a SID more. sysdb tests
use 'S-1-2-3-4-5', feel free to re-use that identifier.

> +#define PROTO "TCP"
> +#define LIFETIME 200
> +#define NAME "foo_name"
> +
> +
> +struct cli_protocol_version *register_cli_protocol_version(void)
> +{
> +    static struct cli_protocol_version responder_test_cli_protocol_version[] = {
> +        {0, NULL, NULL}
> +    };
> +
> +    return responder_test_cli_protocol_version;
> +}

I don't think the register_cli_protocol_version function is needed at
all. It might be needed in some of the later tests, though, but this one
should do fine without the function, right?

> +
> +struct test_state {
> +    struct sss_nc_ctx *ctx;
> +};
> +
> +static void setup(void **state)
> +{
> +    int ret;
> +    struct test_state *ts;
> +
> +    ts = talloc(NULL, struct test_state);
> +    assert_non_null(ts);
> +
> +    ret  = sss_ncache_init(ts, &ts->ctx);
> +    assert_int_equal(ret, EOK);
> +    assert_non_null(ts->ctx);
> +
> +    *state = (void *)ts;
> +}
> +
> +static void teardown(void **state)
> +{
> +    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> +    talloc_free(ts);
> +}
> +
> +static void test_sss_ncache_init(void **state)
> +{
> +    int ret;
> +    struct test_state *ts;
> +    struct sss_nc_ctx *ctx;
> +
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_ncache_init(ts, &ctx);
> +    assert_int_equal(ret, EOK);
> +    assert_non_null(ctx);

This test is fine, but I don't think it should use the setup/teardown
functions. That way, if this test is run without fixtures and would be
the first to run, you would verify that the fixtures are correct and can
be used for the rest of the tests.

> +}
> +
> +/* @test_sss_ncache_uid : test following functions
> + * sss_ncache_set_uid
> + * sss_ncache_check_uid
> + */
> +static void test_sss_ncache_uid(void **state)
> +{
> +    uid_t uid;
> +    int ret, ttl;
> +    bool permanent;
> +    TALLOC_CTX *memctx;
> +    struct test_state *ts;

I think it would be nice to add one more logic into the test. Make the
lifetime really short (say, 2 seconds) and in the test, verify that
right after setting the entry into negcache, the entry is there. Then
sleep for 2 seconds and verify that the entry is gone already.

Perhaps it would be sufficient to do this just in one test to not
prolong the time it takes to run 'make check'.

> +
> +    ttl = LIFETIME;
> +    uid = getuid();
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +
> +    /* test when uid not present in database */
> +    ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> +    assert_int_equal(ret, ENOENT);
> +
> +    /* test when uid is present in database */
> +    permanent = true;

The 'permanent' test case should make sure the entry stays in the
negative cache even after the TTL passes.

> +    ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> +    assert_int_equal(ret, EEXIST);
> +
> +    permanent = false;

On the contrary, the 'not permanent' tests should make sure that the
entry goes away, as I noted above.

> +    ret = sss_ncache_set_uid(ts->ctx, permanent, uid);
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> +    assert_int_equal(ret, EEXIST);
> +
> +    /* test when ttl is -1 with uid present in database*/
> +    ttl = -1;
> +    ret = sss_ncache_check_uid(ts->ctx, ttl, uid);
> +    assert_int_equal(ret, EEXIST);
> +}
> +

[snip]

> +static void test_sss_ncache_reset_permanent(void **state)
> +{
> +    int ret;
> +    struct test_state *ts;
> +
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_ncache_reset_permament(ts->ctx);

^^ here is a typo, this should read 'permanent'. I suspect you just need
to rebase on top of your own rename patch :-)

> +    assert_int_equal(ret, EOK);
> +}
> +
> +/*
> +static void test_sss_ncache_prepopulate(void **state)
> +{
> +    int ret;
> +    struct resp_ctx *rctx;
> +    struct confdb_ctx *cdb;
> +    struct sss_nc_ctx *ncache;
> +
> +    // initialization need to be done
> +    ret = sss_ncache_prepopulate(ncache, cdb, rctx);o

You can use the mock function mock_rctx() from
src/tests/cmocka/common_mock_resp.c to initialize a dummy responder
context. Then you'd have to create a fake 'confdb' database using a call
to create_dom_test_ctx(). The configuration to be passed is then simply
an array of "struct sss_test_conf_param". For you case the array would
look something like:
    struct sss_test_conf_param params[] = {
            { "filter_users", "testuser1" },
            { "filter_groups", "testgroup1" },
            { NULL, NULL },             /* Sentinel */
    };

Check out how the fake database is created at
src/tests/cmocka/test_nss_srv.c:599

> +    assert_int_equal(ret, EOK);
> +}
> +*/
> +
> +int main(void)
> +{
> +    const UnitTest tests[] = {
> +        unit_test_setup_teardown(test_sss_ncache_init, setup, teardown),

As noted above, this test shouldn't use the setup and teardown routines,
because the test does the same as the fixtures. It can be called simply
with unit_test(test_sss_ncache_init)

(obviously it can't use the state then, but its own local state)

> +        unit_test_setup_teardown(test_sss_ncache_uid, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_gid, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_sid, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_user, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_group, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_netgr, setup, teardown),
> +        unit_test_setup_teardown(test_sss_ncache_service_name, setup,
> +                                 teardown),
> +        unit_test_setup_teardown(test_sss_ncache_service_port, setup,
> +                                  teardown),
> +        unit_test_setup_teardown(test_sss_ncache_reset_permanent, setup,
> +                                  teardown)
> +        //unit_test(test_sss_ncache_prepopulate)
> +    };
> +
> +    return run_tests(tests);
> +}
> --
> 1.8.1.4
>


Thanks for the patch!