On Wed, May 28, 2014 at 05:16:03PM -0400, Yassir Elley wrote:
----- Original Message -----
On (23/05/14 16:43), Yassir Elley wrote:
----- Original Message -----
On Fri, May 23, 2014 at 03:50:18PM -0400, Yassir Elley wrote:
----- Original Message -----
On Fri, May 23, 2014 at 12:29:37PM -0400, Yassir Elley wrote: > > > ----- Original Message ----- > > On Tue, May 20, 2014 at 10:30:15PM -0400, Yassir Elley wrote: > > > Hi, > > > > > > The attached patch adds unit tests for some of the ad_gpo > > > functionality. It > > > also fixes some failure mode processing code in ad_gpo.c that > > > was > > > uncovered by the unit tests. This patch depends on a previously > > > submitted > > > patch ("Remove GPO dependency on libsamba-security"), which has > > > not > > > yet > > > been pushed to master. > > > > > > Regards, > > > Yassir. > > > > Can you send a rebased version on top of the latest version of > > your > > 'AD-GPO: Remove dependency on libsamba-security' patch? > > > > bye, > > Sumit > > _______________________________________________ > > sssd-devel mailing list > > sssd-devel@lists.fedorahosted.org > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > Rebased version of patch attached.
I'm sorry, but the test does not compile on top of you other patches. I think it is due to adding the idmap context:
CC src/tests/cmocka/ad_gpo_tests-test_ad_gpo.o ../src/tests/cmocka/test_ad_gpo.c: In function 'test_ad_gpo_ace_includes_client_sid': ../src/tests/cmocka/test_ad_gpo.c:269:42: warning: passing argument 5 of 'ad_gpo_ace_includes_client_sid' from incompatible pointer type [enabled by default] ace_dom_sid, &includes_client_sid); ^ In file included from ../src/tests/cmocka/test_ad_gpo.c:33:0: ../src/providers/ad/ad_gpo.c:253:1: note: expected 'struct sss_idmap_ctx *' but argument is of type '_Bool *' ad_gpo_ace_includes_client_sid(const char *user_sid, ^ ../src/tests/cmocka/test_ad_gpo.c:269:42: error: too few arguments to function 'ad_gpo_ace_includes_client_sid' ace_dom_sid, &includes_client_sid); ^ In file included from ../src/tests/cmocka/test_ad_gpo.c:33:0: ../src/providers/ad/ad_gpo.c:253:1: note: declared here ad_gpo_ace_includes_client_sid(const char *user_sid, ^ make[3]: *** [src/tests/cmocka/ad_gpo_tests-test_ad_gpo.o] Error 1
bye, Sumit
That was very sloppy of me. Sorry about that. I've attached a revised patch.
The test failed for me because of an issue in test_populate_som_list(). You should either set num_soms = 0 in the declaration or skip
assert_int_equal(num_soms, expected->num_soms);
if ret != EOK. Because otherwise you would compare with an uninitialized value.
I think skipping the assert_int_equal() makes more sense because you do not expect num_soms to be set in the error case.
bye, Sumit
Although it is curious that the tests are failing for you but not for me, I do agree with your comments. There is a similar issue in test_populate_gplink_list. I have fixed them both and attached a revised patch.
Regards, Yassir.
From f075dd0ec7e1538e2f674b6cfd1a3fd4b842e480 Mon Sep 17 00:00:00 2001 From: Yassir Elley yelley@redhat.com Date: Thu, 3 Apr 2014 11:21:43 -0400 Subject: [PATCH] AD-GPO: Add ad_gpo unit test; Fix some failure modes in ad_gpo.c
Makefile.am | 29 +++ src/providers/ad/ad_gpo.c | 26 ++- src/tests/cmocka/test_ad_gpo.c | 388 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 434 insertions(+), 9 deletions(-) create mode 100644 src/tests/cmocka/test_ad_gpo.c
diff --git a/Makefile.am b/Makefile.am index 510cfc78ba59f08fc73ed0f971e77ce6f2288461..134163fee47a923808b866daed92453b147fd79d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -177,6 +177,7 @@ if HAVE_CMOCKA test_sss_idmap \ test_ipa_idmap \ test_utils \
ad_gpo_tests \ ad_common_tests \ dp_opt_tests \ responder-get-domains-tests \@@ -1723,6 +1724,34 @@ ad_access_filter_tests_LDADD = \ libsss_ad_common.la \ libsss_test_common.la
+ad_gpo_tests_SOURCES = \
- $(sssd_be_SOURCES) \
- src/util/sss_ldap.c \
- src/util/sss_krb5.c \
- src/util/find_uid.c \
- src/util/user_info_msg.c \
- src/providers/ad/ad_common.c \
- src/tests/cmocka/test_ad_gpo.c
+ad_gpo_tests_CFLAGS = \
- $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \
- $(NDR_NBT_CFLAGS) \
- -DUNIT_TESTING
+ad_gpo_tests_LDADD = \
- $(PAM_LIBS) \
- $(CMOCKA_LIBS) \
- $(SSSD_LIBS) \
- $(CARES_LIBS) \
- $(KRB5_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- $(SYSTEMD_LOGIN_LIBS) \
- $(NDR_NBT_LIBS) \
- libsss_ldap_common.la \
- libsss_idmap.la \
- libsss_krb5_common.la \
- libsss_ad_common.la \
- libsss_test_common.la
There were recent changes in Makefile.am and some lines can be removed. src/util/user_info_msg.c was moved to libsss_util.so src/util/find_uid.c was moved to libsss_util.so src/util/sss_ldap.c was moved to libsss_ldap_common.so src/util/sss_krb5.c was moved to libsss_krb5_common.so
Here is diff: ad_gpo_tests_SOURCES = \ $(sssd_be_SOURCES) \
- src/util/sss_ldap.c \
- src/util/sss_krb5.c \
- src/util/find_uid.c \
- src/util/user_info_msg.c \ src/providers/ad/ad_common.c \ src/tests/cmocka/test_ad_gpo.c
ad_gpo_tests_CFLAGS = \ $(AM_CFLAGS) \
- $(SYSTEMD_LOGIN_CFLAGS) \ $(NDR_NBT_CFLAGS) \ -DUNIT_TESTING
ad_gpo_tests_LDADD = \ @@ -1752,9 +1747,7 @@ ad_gpo_tests_LDADD = \ $(CMOCKA_LIBS) \ $(SSSD_LIBS) \ $(CARES_LIBS) \
- $(KRB5_LIBS) \ $(SSSD_INTERNAL_LTLIBS) \
- $(SYSTEMD_LOGIN_LIBS) \ $(NDR_NBT_LIBS) \ libsss_ldap_common.la \ libsss_idmap.la \
Test passed without any problem. I let code review for Sumit.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Revised patch attached.
Thank you. I only have two cosmetic changes left. Please put the code change in a separate patch so that the second patch only contains the test. And please no '//' comments.
bye, Sumit
Thanks, Yassir.