On Fri, May 30, 2014 at 09:22:51AM -0400, Yassir Elley wrote:
----- Original Message -----
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
Revised patches attached.
Thanks, Yassir.
ACK
bye, Sumit