Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
1) cmocka's mocking feature Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid); }
2) the -wrap feature of ld It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret; }
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests: * cached user including assertion that DP was not contacted * nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache * search for a user that has not been cache prior to the search * updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are: * writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think. * cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
On Tue, Feb 12, 2013 at 06:12:13PM +0100, Jakub Hrozek wrote:
Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
- cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
- the -wrap feature of ld
It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret;
}
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests:
- cached user including assertion that DP was not contacted
- nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache
- search for a user that has not been cache prior to the search
- updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
- writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think.
- cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
I think this is really great. Being able to test a full request to a responder puts the included test-suite to the next level and will increase the amount of code covered by tests a lot.
I will send a review of the code later but in general I think those patches should be push upstream to encourage people to add more tests for the NSS responder or start new tests for the other responders.
In an ideal world it would be possible to add a new test for the NSS responder without writing code but just dropping a JSON/XML/INI file with the relevant data for a single test in a directory. But I think we need a bit more experience with cmocka based tests to understand what is necessary for this kind of abstraction.
Review will follow.
bye, Sumit
On Tue, 2013-02-12 at 18:12 +0100, Jakub Hrozek wrote:
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Looks great to me.
+1 as the way to go for testing.
Simo.
On 02/12/2013 06:12 PM, Jakub Hrozek wrote:
Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
- cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
- the -wrap feature of ld
It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret;
}
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests:
- cached user including assertion that DP was not contacted
- nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache
- search for a user that has not been cache prior to the search
- updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
- writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think.
- cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
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
Why did you include setjmp.h?
On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
On 02/12/2013 06:12 PM, Jakub Hrozek wrote:
Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
- cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
- the -wrap feature of ld
It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret;
}
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests:
- cached user including assertion that DP was not contacted
- nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache
- search for a user that has not been cache prior to the search
- updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
- writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think.
- cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
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
Why did you include setjmp.h?
from cmocka.c:
/* * These headers or their equivalents should be included prior to * including * this header file. * * #include <stdarg.h> * #include <stddef.h> * #include <setjmp.h> * * This allows test applications to use custom definitions of C standard * library functions and types. */
HTH
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
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.
On 03/01/2013 02:01 PM, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
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.
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.
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
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:
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.
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.
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:
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!
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:
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.
On Tue, Mar 05, 2013 at 05:25:45PM +0100, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
OK, done.
New tests attached.
Maybe you can consider the attached patch for inclusion as well? It will silence a "cast to pointer from integer of different size [-Wint-to-pointer-cast]" compiler warning.
What do you think of asking upstream if they would like to include
#define mock_type(type) ((type) mock())
and maybe
#define mock_ptr_type(type) ((type) (uintptr_t) mock())
or similar? To avoid conflicts I would recommend to add a prefix, e.g. sss_mock_type and sss_mock_ptr_type.
bye, Sumit
On Wed, Mar 06, 2013 at 09:33:12PM +0100, Sumit Bose wrote:
On Tue, Mar 05, 2013 at 05:25:45PM +0100, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
OK, done.
New tests attached.
Maybe you can consider the attached patch for inclusion as well? It will silence a "cast to pointer from integer of different size [-Wint-to-pointer-cast]" compiler warning.
What do you think of asking upstream if they would like to include
#define mock_type(type) ((type) mock())
and maybe
#define mock_ptr_type(type) ((type) (uintptr_t) mock())
or similar? To avoid conflicts I would recommend to add a prefix, e.g. sss_mock_type and sss_mock_ptr_type.
Thank you, I've merged your patch and used prefix for the macros. I will ask cmocka upstream to provide their macros so when/if they are available we can simply use those.
New patches are attached.
On Fri, Mar 08, 2013 at 02:46:10PM +0100, Jakub Hrozek wrote:
On Wed, Mar 06, 2013 at 09:33:12PM +0100, Sumit Bose wrote:
On Tue, Mar 05, 2013 at 05:25:45PM +0100, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
OK, done.
New tests attached.
Maybe you can consider the attached patch for inclusion as well? It will silence a "cast to pointer from integer of different size [-Wint-to-pointer-cast]" compiler warning.
What do you think of asking upstream if they would like to include
#define mock_type(type) ((type) mock())
and maybe
#define mock_ptr_type(type) ((type) (uintptr_t) mock())
or similar? To avoid conflicts I would recommend to add a prefix, e.g. sss_mock_type and sss_mock_ptr_type.
Thank you, I've merged your patch and used prefix for the macros. I will ask cmocka upstream to provide their macros so when/if they are available we can simply use those.
New patches are attached.
Pavel asked me off-list to use an enum for deciding whether we are calling a real function or a wrapper. New patches are attached.
On 03/08/2013 05:25 PM, Jakub Hrozek wrote:
On Fri, Mar 08, 2013 at 02:46:10PM +0100, Jakub Hrozek wrote:
On Wed, Mar 06, 2013 at 09:33:12PM +0100, Sumit Bose wrote:
On Tue, Mar 05, 2013 at 05:25:45PM +0100, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
OK, done.
New tests attached.
Maybe you can consider the attached patch for inclusion as well? It will silence a "cast to pointer from integer of different size [-Wint-to-pointer-cast]" compiler warning.
What do you think of asking upstream if they would like to include
#define mock_type(type) ((type) mock())
and maybe
#define mock_ptr_type(type) ((type) (uintptr_t) mock())
or similar? To avoid conflicts I would recommend to add a prefix, e.g. sss_mock_type and sss_mock_ptr_type.
Thank you, I've merged your patch and used prefix for the macros. I will ask cmocka upstream to provide their macros so when/if they are available we can simply use those.
New patches are attached.
Pavel asked me off-list to use an enum for deciding whether we are calling a real function or a wrapper. New patches are attached.
Ack!
On Fri, Mar 08, 2013 at 06:21:01PM +0100, Pavel Březina wrote:
On 03/08/2013 05:25 PM, Jakub Hrozek wrote:
On Fri, Mar 08, 2013 at 02:46:10PM +0100, Jakub Hrozek wrote:
On Wed, Mar 06, 2013 at 09:33:12PM +0100, Sumit Bose wrote:
On Tue, Mar 05, 2013 at 05:25:45PM +0100, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 02:38:08PM +0100, Pavel Březina wrote:
Create a macro for: *body = (uint8_t *) mock() => mock_as(uint8_t*)
OK, done.
New tests attached.
Maybe you can consider the attached patch for inclusion as well? It will silence a "cast to pointer from integer of different size [-Wint-to-pointer-cast]" compiler warning.
What do you think of asking upstream if they would like to include
#define mock_type(type) ((type) mock())
and maybe
#define mock_ptr_type(type) ((type) (uintptr_t) mock())
or similar? To avoid conflicts I would recommend to add a prefix, e.g. sss_mock_type and sss_mock_ptr_type.
Thank you, I've merged your patch and used prefix for the macros. I will ask cmocka upstream to provide their macros so when/if they are available we can simply use those.
New patches are attached.
Pavel asked me off-list to use an enum for deciding whether we are calling a real function or a wrapper. New patches are attached.
Ack!
Thank you for the thorough review.
Pushed to master.
On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
On 02/12/2013 06:12 PM, Jakub Hrozek wrote:
Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
- cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
- the -wrap feature of ld
It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret;
}
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests:
- cached user including assertion that DP was not contacted
- nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache
- search for a user that has not been cache prior to the search
- updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
- writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think.
- cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
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.
OK, the files are hopefully better split now:
There is common_dom.c for tests that would like some common way to connect to a test sysdb. In future, we should convert even the existing tests like sysdb-test or sysdb-ssh-test to use this common code to avoid code duplication. I will file a ticket about this.
Then there is common_tev.c for tests that require an event loop and may need to issue dummy tevent requests.
Both files are stored in the src/tests. I think we should shuffle the files a bit so that only helpers that do not require neither check nor cmocka would be in the src/tests subdirectory directly. The check-based tests and their specific utils should be moved to src/tests/check. This would be another ticket.
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
Currently there are no mock-related functions that would be reusable by any test (one that would test provider and one that would test responder for example).
The functions you mentioned above were moved to src/tests/cmocka/cmocka_common_resp.c as they would be reused by any cmocka-based test that tests the NSS responder.
Why did you include setjmp.h?
For clarity the setjmp and other required headers are now included in header src/tests/cmocka/common_mock.h
On 03/05/2013 05:31 PM, Jakub Hrozek wrote:
On Fri, Mar 01, 2013 at 01:39:22PM +0100, Pavel Březina wrote:
On 02/12/2013 06:12 PM, Jakub Hrozek wrote:
Hi,
Short version - attached is a patch proposal for a unit test based on cmocka. The patches are an example of a complex, yet isolated unit test and I'd like to get opinions on whether this would be a good way to go.
Long version: Our check based test suite is OK for developing simple tests for APIs like sysdb but not really great for testing of more complex interfaces.
In particular, it's hard to simulate certain pieces of functionality that require interaction with network or the rest of the system such as LDAP searches or logins, especially in environments like buildsystems.
I think we might want to look at Cmocka to provide this missing functionality and test complex interfaces in isolation. Cmocka[1] is a fork of Google's cmockery which is not maintained upstream anymore. Cmocka is maintained by Andreas Schneider.
I've been playing with cmocka lately and wrote a couple of patches that add a unit test for the NSS responder, in particular the getpwnam() function. It was quite easy to simulate the Data Provider and the client using two concepts:
- cmocka's mocking feature
Functions that provide external interface, like the dummy DP functions in my patches, can access a stack of values. The unit test can push some expected return values onto the stack before launching the test and the dummy functions would then pop the values and use them as appropriate. It's quite easy to simulate a Data Provider saving an entry to cache with call like this (pseudocode):
int unit_test() { will_return(sss_dp_get_account_recv, "testuser"); will_return(sss_dp_get_account_recv, 10); will_return(sss_dp_get_account_recv, 11); }
int sss_dp_get_account_recv(TALLOC_CTX *mem_ctx, tevent_req *req) { char *username = mock(); uid_t uid = mock(); gid_t gid = mock();
sysdb_store_user(username, uid, gid);
}
- the -wrap feature of ld
It is possible to selectively override functions from modules linked against to intercept calls using the -wrap feature of ld. In my unit tests I used this feature to check results of responder search with defined callbacks instead of returning the results to the nss client. Here is a simple example of intercepting a call to negative cache to make sure the negative cache is hit when it's supposed to:
int __wrap_sss_ncache_check_user(struct sss_nc_ctx *ctx, int ttl, struct sss_domain_info *dom, const char *name) { int ret;
ret = __real_sss_ncache_check_user(ctx, ttl, dom, name); if (ret == EEXIST) { nss_test_ctx->ncache_hit = true; } return ret;
}
There's a couple of boilerplate functions but once these are in place, they can be reused to cover different pieces of functionality..the attached patches test the following requests:
- cached user including assertion that DP was not contacted
- nonexistant user including assertion that on a subsequent lookup, the request is returned from negative cache
- search for a user that has not been cache prior to the search
- updating an expired user including assertions that the user had been updated from DP and the updated entry is returned.
The downsides of using cmocka as far as I can see are:
- writing the mocking code can be tedious. On the other hand, the NSS responder I picked was probably the most complex code I could test as it communicates with both DP and the client library. Even more isolated testing (the AD sites feature comes to mind) would be much easier I think.
- cmocka is not present in RHEL
I'll be glad to hear comments about this unit test proposal. Patches are attached.
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.
OK, the files are hopefully better split now:
There is common_dom.c for tests that would like some common way to connect to a test sysdb. In future, we should convert even the existing tests like sysdb-test or sysdb-ssh-test to use this common code to avoid code duplication. I will file a ticket about this.
Then there is common_tev.c for tests that require an event loop and may need to issue dummy tevent requests.
Your fingers must have been really painful, so that you was unable to write the remaining three letters :-)
Both files are stored in the src/tests. I think we should shuffle the files a bit so that only helpers that do not require neither check nor cmocka would be in the src/tests subdirectory directly. The check-based tests and their specific utils should be moved to src/tests/check. This would be another ticket.
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
Currently there are no mock-related functions that would be reusable by any test (one that would test provider and one that would test responder for example).
The functions you mentioned above were moved to src/tests/cmocka/cmocka_common_resp.c as they would be reused by any cmocka-based test that tests the NSS responder.
Why did you include setjmp.h?
For clarity the setjmp and other required headers are now included in header src/tests/cmocka/common_mock.h _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org