[SSSD] [PATCH] RFC: Unit test for the NSS responder based on the cmocka library

Pavel Březina pbrezina at redhat.com
Wed Mar 6 14:37:43 UTC 2013


On 03/05/2013 05:25 PM, Jakub Hrozek wrote:
> On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
>> On 03/01/2013 02:01 PM, Jakub Hrozek wrote:
>>> On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
>>>>> [1] http://cmocka.cryptomilk.org/
>>>>
>>>> Very nice.
>>>>
>>>> I would like to increase granularity though. First of all we should
>>>> separate cmocka test files from check files on directory/filename
>>>> level. I'd say that all cmocka stuff should go into tests/cmocka. For
>>>> example, there are files common_check.c, common_dom.c, common.c, but
>>>> only common_dom.c is suitable for cmocka. When more common files and
>>>> tests comes, it will be hard to distinguish them.
>>>>
>>>> Testing with cmocka is mainly about mocking structures and creating
>>>> wrappers around functions. We should try to create reusable mocks and
>>>> wrappers right from the beginning, making them independent on tests.
>>>> For example: mock_rctx(), mock_cctx(), sss_dp_get_account_send(),
>>>> sss_dp_get_account_recv(), ... I believe these and more functions can
>>>> be reused when testing other responders so they should be placed in
>>>> e.g. common_responder.c
>>>>
>>>
>>> Definitely. As I said in the original mail, I didn't want to spend too
>>> much time without getting some kind of feedback.
>>>
>>> I will move the definitions to some common file and resubmit.
>>>
>>>> Why did you include setjmp.h?
>>>
>>> I will add a comment to clarify this.
>>
>> OK, I went through the test code and here are few ideas to improve
>> readability.
>>
>> A wrapper has the ability to execute the original function. And you use
>> will_return(__wrap_fn, true/false) to choose the code flow. A macro
>> would be better:
>>
>> will_return(__wrap_sss_packet_get_body, CALL_REAL);
>> will_return(__wrap_sss_packet_get_body, CALL_WRAPPER);
>>
>> or even better:
>> call_real(fn)
>> call_wrapper(fn)
>>
>> and we should standardize wrappers to always have this ability and that
>> it should be set before any other will_return() is called.
>>
>
> I was experimenting with a wrapper like this but it looked cumbersome to
> me. Mostly because you also need to declare the __real function and if
> the wrapper was in a separate module, both wrapper and real function
> would have to be in the header file. This looked quite ugly to me. I
> would rather keep this suggestion in mind and see in time when other
> tests land if they would also need this feature.

OK, fair enough. But I'd still go with a macro instead of true/false.

>>
>> Also will_return() macro is used to set both parameters and return
>> value. It would be nice to distinguish between those too on semantic
>> level. Something like:
>>
>> mock_param(fn, value) /* we are setting parameter */
>> will_return(fn, value) /* we are setting return value */
>>
>> It would be good if we can provider an order parameter to mock_param()
>> and mock() so we don't have to rely on the calling order. But this
>> probably needs changes in cmocka.
>>
>
> Yes, this sounds like a good idea if it can be done from cmocka's point
> of view.
>
>>
>> Create a macro for:
>> *body = (uint8_t *) mock() => mock_as(uint8_t*)
>
> OK, done.
>
> New tests attached.

We can also split main() to contain only initialization of UnitTest, 
because everything else will be duplicated most of the time.

int main(int argc, const char *argv[])
{
     const UnitTest tests[] = {
         unit_test_setup_teardown(test_nss_getpwnam,
                                  nss_test_setup, nss_test_teardown),
         unit_test_setup_teardown(test_nss_getpwnam_neg,
                                  nss_test_setup, nss_test_teardown),
         unit_test_setup_teardown(test_nss_getpwnam_search,
                                  nss_test_setup, nss_test_teardown),
         unit_test_setup_teardown(test_nss_getpwnam_update,
                                  nss_test_setup, nss_test_teardown),
     };

     return run_tests(argc, argv, tests,
                      TESTS_PATH, TEST_CONF_DB, TEST_SYSDB_FILE);
}

Thanks for the changes. It looks really good now. Good job!




More information about the sssd-devel mailing list