[SSSD] [Patch] Cmocka unit test patch for authtok module

Pallavi Jha pallavikumarijha at gmail.com
Tue Nov 12 11:23:23 UTC 2013


Hi,

I have made the required changes in the patch attached with the mail.
Kindly review it.

With regards
Pallavi Kumari Jha
Computer Science Engineering


On 11 November 2013 20:24, Jakub Hrozek <jhrozek at redhat.com> wrote:

> 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!
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131112/a8e4fe05/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cmocka-unit-test-for-authtok-module-added.patch
Type: text/x-patch
Size: 10845 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131112/a8e4fe05/attachment.bin>


More information about the sssd-devel mailing list