----- 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(a)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(a)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(a)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.