On Mon, Nov 11, 2013 at 05:23:04PM +0530, Pallavi Jha wrote:
> Hello,
>
> I am extremely sorry for the inconvenience. The patch is attached with the
> mail. Kindly have a look. I will shortly send another patch for authtor
> module with the suggested fix.
>
> Thanking You,
> Pallavi

This code is a good start, thank you. It needs some changes before it can be
accepted, but we can fix them quite simply.

I noticed you use setup and teardown functions with most tests, but not
test_sss_authtok_new(). I think you can use the setup and teardown
everywhere and simplify the code a bit, because the "state" pointer can
already act as a talloc context. By using the setup and teardown, the
test could be simplified to:

static void test_sss_authtok_new(void **state)
{
    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
    struct sss_auth_token *authtoken;

    authtoken = sss_authtok_new(ts);
    assert_non_null(authtoken);

    talloc_free(authtoken);
}

Notice there's no talloc called here, we just use the "struct
test_state". Another interation of the patch might also check for leaks
as well, but let's not worry about that now.

Also, the test_sss_authtok_new() attempts to return ENOMEM, even though it's
a void-typed function. In the test cases that are void-typed you should use
the assert macros instead, so if you need to allocate memory next time, you can
do:
    mem_ctx = talloc_new(parent_ctx);
    assert_non_null(mem_ctx);

The same issue is in test_sss_authtok_copy().

About test_sss_authtok() and test_sss_authtok_set() -- I think it would
make sense to combine them into a single test case or if you prefer,
one test case per authtok type.

The flow in the test case is fine, I think, the case assigns some data
and then retrieves them back with getter and compares, which is the
point. I'm just thinking that having the tests per authtok type might
make them more readable, then each case would have the same flow that
would cover the whole "lifecycle" of the authtok:

In the same test, this code is not necessary:
    size_t *ret_len = (size_t *)malloc(sizeof(size_t *));
    const char **pwd = (const char **)malloc(sizeof(const char **));

    ret = sss_authtok_get_password(ts->authtoken, pwd, ret_len);

You can just use local variables:
    size_t ret_len;
    const char *pwd;

    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);

But please move declarations of all variables to the top of the function
(that's just a style choice of the SSSD).

And lastly -- please remove the tests with NULL authtok from the current
test and submit them only when they work along with the improvement to
return error on NULL authtok.

Thank you very much for the contribution!