[SSSD] [PATCH] AD: Add ad_gpo unit tests

Sumit Bose sbose at redhat.com
Fri May 30 07:48:28 UTC 2014


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 at 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 at 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 at 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.



More information about the sssd-devel mailing list