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

Jakub Hrozek jhrozek at redhat.com
Fri Mar 8 13:40:09 UTC 2013


On Wed, Mar 06, 2013 at 03:37:43PM +0100, Pavel Březina wrote:
> 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!

Unfortunately we can't, run_tests is a macro that relies on being in the
same scope as the unit tests because it computes the size of the test[]
array:

#define run_tests(tests) _run_tests(tests, sizeof(tests) / sizeof(tests)[0])

Techniclly we could use the _run_test() function directly but the
underscore functions usually mean that they are not supposed to be used
directly but rather wrapped in a macro.



More information about the sssd-devel mailing list