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.
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
----- 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.
Regards, Yassir.
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
Regards, Yassir.
----- 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.
Regards, Yassir.
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
Regards, Yassir.
----- 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.
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
----- 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.
Thanks, Yassir.
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.
----- 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.
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
On Fri, May 30, 2014 at 03:55:56PM +0200, Sumit Bose wrote:
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
Pushed to master: 0352c371e743d8dae996123f658b5d32c677614e 4e299001aff97d5aaabd7ca490cd7d3d5151823e
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Sumit Bose -
Yassir Elley