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.