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!