----- 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(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
>
Revised patches attached.
Thanks,
Yassir.