[SSSD] [PATCH v4] Add integration tests

Lukas Slebodnik lslebodn at redhat.com
Tue May 26 06:25:18 UTC 2015


On (25/05/15 17:55), Nikolai Kondrashov wrote:
>On 05/25/2015 11:44 AM, Lukas Slebodnik wrote:
>>A)
>>The 1st patch is not necessary.
>>CI passed without 1st patch
>>http://sssd-ci.duckdns.org/logs/job/15/89/summary.html (fedora 20 failed due to
>>dyndnd-test)
>>
>>B) You are misusing autoconf macro AM_CONDITIONAL.
>>It shoudl be used just in case you want to have "if conditions" in automake
>>files. ENABLE_INTGCHECK
>>
>>Please try to look into documentation of autoconf macros
>>AC_SUBST
>>AC_DEFINE (AC_DEFINE_UNQUOTED)
>>AM_CONDITIONAL
>>
>>In most cases you just need just AC_SUBST for HAVE_$sth
>>or just AC_CHECK_PROG.
>
>Yes this can make things simpler and more reliable. I remade the configuration
>to just use shell variables, as AC_SUBST is not necessary, because we don't
>use these in any configure-processed files (e.g. Makefile.am's).
>
>Please see the first attached patch to see what I mean.
>
I did few other improvements in autoconf code.
@see attached diff

>>This also reminds me you can squas
>>"small external" files into src/external/intgcheck.m4
>>It does not worth to have one file just with a line AC_CHECK_PROG
>>
>>  src/external/fakeroot.m4      |   2 +
>>  src/external/intgcheck.m4     |  23 ++++
>>  src/external/ldap.m4          |   6 +
>>  src/external/pytest.m4        |   3 +
>
>Regarding this, wouldn't it make the corresponding macros somewhat harder to
>find? We have a few short files under src/external with clear names and
>contents that doesn't quite fit anywhere and I thought that rather neat.
>
>I can stuff fakeroot and py.test checks into intgcheck.m4 without much harm, I
>think. However, would it really be good to move slapd and ldapmodify checking
>out of ldap.m4? Wouldn't it be confusing?
>
ok, detection of ldap command can stay in ldap.m4

>>@see
>>diff --git a/src/external/pytest.m4 b/src/external/pytest.m4
>>index 00f28e5..b6305d1 100644
>>--- a/src/external/pytest.m4
>>+++ b/src/external/pytest.m4
>>@@ -1,3 +1,2 @@
>>  AC_PATH_PROG([PYTEST], [py.test])
>>-AC_SUBST([PYTEST])
>>-AM_CONDITIONAL([HAVE_PYTEST], [test x"$PYTEST" != x])
>>+AC_SUBST([HAVE_PYTEST], [test x"$PYTEST" != x])
>
>Hmm, would the last line really work? To me it looks like not, am I missing
>something?
>
>>C) We can simplify make target intgcheck
>>It significantly improve user experince.
>>(the same as "make distcheck" or make tests in samba)
>
>Ah, good catch, yes. Thank you, Lukas. However, leaving the configure option
>named "--enable-intgcheck" will be confusing, so I tried to rename it to
>something more relevant to what it will be doing now. It still feels a bit
>awkward, but perhaps it's good enough.
>
>Please take a look at the second attached patch, for illustration.
>
You can also change related parts in CI script

>>D) please find better name for "src/tests/intg/misc.py"
>
>I think "misc.py" is perfectly fine, but since our previous discussion on
>similar matter stalled, please suggest a name and I'll name it as you say and
>we'll be done with it.
>
It's better to do not mix utility functions.
So feel free to split file to subfiles if it will be used among many files
or include helper function directly to module. You can later extract helper
function to another utility module when there will be use case.
(run_shell is not used atm)

And here are proposed names. string_utils | utils_string | utils.string,
shell_utils (python already has shutil) ...


>>I will file other tickets with improvements after pushing patch to master.
>
>Thank you, Lukas.
>

On (25/05/15 18:26), Nikolai Kondrashov wrote:
>On 05/25/2015 05:55 PM, Nikolai Kondrashov wrote:
>>On 05/25/2015 11:44 AM, Lukas Slebodnik wrote:
>>>BTW there is a failed intgtest
>>>http://sssd-ci.duckdns.org/logs/job/15/90/fedora20/ci-build-debug/ci-make-intgcheck.log
>>>It would be good to prevent such fialures. I've already sent patches to fix
>>>negcache-test and SSSDConfig-test. We do not need to have another problematic test
>>
>>Argh, I've seen this, wanted to fix it and forgot. Can't reproduce it at the
>>moment, but I'll keep trying. Meanwhile, do you have any ideas of what might
>>be causing this?
>
>Ah, nevermind, got confused, figured it out now. Thanks.
>
I have no idea what caused failure, but I'm glad you fiure it out.
It might be better idea to do nat catch excetion (or rethrow exception)
to see full stack trace with failure.

>From 9978cc3b56e6e664384deeff2a497ccb377e06d3 Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Mon, 25 May 2015 15:39:08 +0300
>Subject: [PATCH 1/2] intgcheck: Use shell variables directly for dep checks
>
>---
> configure.ac              | 3 ---
> src/external/cwrap.m4     | 2 ++
> src/external/fakeroot.m4  | 3 +--
> src/external/intgcheck.m4 | 4 ++--
> src/external/ldap.m4      | 9 ++++++---
> src/external/pytest.m4    | 6 +++++-
> 6 files changed, 16 insertions(+), 11 deletions(-)
>
>diff --git a/src/external/ldap.m4 b/src/external/ldap.m4
>index be71f4c..6ef8f09 100644
>--- a/src/external/ldap.m4
>+++ b/src/external/ldap.m4
>@@ -93,6 +93,9 @@ LIBS=$SAVE_LIBS
> AC_PATH_PROG([SLAPD], [slapd], ,
>              [$PATH$PATH_SEPARATOR/usr/sbin$PATH_SEPARATOR])
> AC_SUBST([SLAPD])
AC_PATH_PROG uses AC_SUBST so you needn't it here.

>-AM_CONDITIONAL([HAVE_SLAPD], [test x"$SLAPD" != x])
>-AC_CHECK_PROG([HAVE_LDAPMODIFY], [ldapmodify], [true], [false])
>-AM_CONDITIONAL([HAVE_LDAPMODIFY], [$HAVE_LDAPMODIFY])
>+if test -n "$SLAPD"; then
>+    HAVE_SLAPD=yes
>+else
>+    HAVE_SLAPD=no
>+fi

>From cbd044012654be11991f8af0ff5d97658eaa5841 Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Mon, 25 May 2015 17:21:06 +0300
>Subject: [PATCH 2/2] intgcheck: Rename --enable-intgcheck to
> --enable-intgcheck-reqs
>
>---
> Makefile.am               |  3 +--
> configure.ac              |  2 +-
> src/external/intgcheck.m4 | 11 +++++------
> 3 files changed, 7 insertions(+), 9 deletions(-)
>
>diff --git a/src/external/intgcheck.m4 b/src/external/intgcheck.m4
>index b1efd30..bd2c71b 100644
>--- a/src/external/intgcheck.m4
>+++ b/src/external/intgcheck.m4
>@@ -1,12 +1,12 @@
> AC_DEFUN([AM_INTGCHECK_REQ], [
>     if test x"$$1" != xyes; then
>-        AC_MSG_ERROR([cannot enable integration tests: $2 not found])
>+        AC_MSG_ERROR([cannot run integration tests: $2 not found])
>     fi
> ])
>-AC_DEFUN([AM_ENABLE_INTGCHECK], [
Feel free to rename macro but it shoudl not have a prefix AC_ or
AM_. It is a "namespace" for macros from autoconf or automake.

>-    AC_ARG_ENABLE(intgcheck,
>-                  [AS_HELP_STRING([--enable-intgcheck],
>-                                  [enable integration test support (intgcheck target) [default=no]])],
>+AC_DEFUN([AM_ENABLE_INTGCHECK_REQS], [
>+    AC_ARG_ENABLE(intgcheck-reqs,
>+                  [AS_HELP_STRING([--enable-intgcheck-reqs],
>+                                  [enable checking for integration test requirements [default=no]])],
>                   enable_intgcheck="$enableval",
>                   enable_intgcheck="no")
>     if test x"$enable_intgcheck" == xyes; then
>@@ -19,5 +19,4 @@ AC_DEFUN([AM_ENABLE_INTGCHECK], [
>         AM_INTGCHECK_REQ([HAVE_PYTEST],         [pytest])
>         AM_INTGCHECK_REQ([HAVE_PY2MOD_LDAP],    [python-ldap])
                                              ^^^^
                Such might look good but if there is a longer 1st argument
                you will need to reindent it.
                BTW it causes many pep8 warnings in python code.
-------------- next part --------------
diff --git a/Makefile.am b/Makefile.am
index f952988..a2a7da6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2419,7 +2419,6 @@ endif
 # Integration tests #
 #####################
 
-if ENABLE_INTGCHECK
 intgcheck:
 	set -e; \
 	rm -Rf intg; \
@@ -2430,6 +2429,7 @@ intgcheck:
 	cd intg/bld; \
 	$(abs_top_srcdir)/configure \
 	    --prefix="$$prefix" \
+	    --enable-intgcheck \
 	    --with-ldb-lib-dir="$$prefix"/lib/ldb \
 	    $(INTGCHECK_CONFIGURE_FLAGS); \
 	$(MAKE) $(AM_MAKEFLAGS); \
@@ -2440,7 +2440,6 @@ intgcheck:
 	$(MAKE) $(AM_MAKEFLAGS) -C src/tests/intg intgcheck-installed; \
 	cd ../..; \
 	rm -Rf "$$prefix" intg
-endif # ENABLE_INTGCHECK
 
 ####################
 # Client Libraries #
diff --git a/configure.ac b/configure.ac
index 518e6c1..0eacd3f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -318,7 +318,6 @@ AM_CONDITIONAL([BUILD_PYTHON_BINDINGS],
                      -o x"$with_python3_bindings" = xyes])
 
 AM_PYTHON2_MODULE([ldap])
-AM_CONDITIONAL([HAVE_PY2MOD_LDAP], [test x"$HAVE_PY2MOD_LDAP" == xyes])
 
 if test x$HAVE_SELINUX != x; then
     AM_CHECK_SELINUX
@@ -392,7 +391,7 @@ AM_CHECK_CMOCKA
 AM_CHECK_UID_WRAPPER
 AM_CHECK_NSS_WRAPPER
 
-AM_ENABLE_INTGCHECK
+SSS_ENABLE_INTGCHECK
 
 AM_CONDITIONAL([HAVE_DEVSHM], [test -d /dev/shm])
 
diff --git a/src/external/cwrap.m4 b/src/external/cwrap.m4
index 0bd0bc9..b03d1ef 100644
--- a/src/external/cwrap.m4
+++ b/src/external/cwrap.m4
@@ -4,20 +4,19 @@ dnl     AM_CHECK_WRAPPER(name, conditional)
 dnl If the cwrap library is found, sets the HAVE_$name conditional
 AC_DEFUN([AM_CHECK_WRAPPER],
 [
-    FOUND_WRAPPER=0
-
     AC_MSG_CHECKING([for $1])
     PKG_CHECK_EXISTS([$1],
                      [
                         AC_MSG_RESULT([yes])
-                        FOUND_WRAPPER=1
+                        AC_SUBST([$2], [yes])
                      ],
                      [
                         AC_MSG_RESULT([no])
+                        AC_SUBST([$2], [no])
                         AC_MSG_WARN([cwrap library $1 not found, some tests will not run])
                      ])
 
-    AM_CONDITIONAL($2, [ test x$FOUND_WRAPPER = x1])
+    AM_CONDITIONAL($2, [ test x$2 = xyes])
 ])
 
 AC_DEFUN([AM_CHECK_UID_WRAPPER],
diff --git a/src/external/fakeroot.m4 b/src/external/fakeroot.m4
index 685eaf8..91c2ceb 100644
--- a/src/external/fakeroot.m4
+++ b/src/external/fakeroot.m4
@@ -1,2 +1 @@
-AC_CHECK_PROG([HAVE_FAKEROOT], [fakeroot], [true], [false])
-AM_CONDITIONAL([HAVE_FAKEROOT], [$HAVE_FAKEROOT])
+AC_CHECK_PROG([HAVE_FAKEROOT], [fakeroot], [yes], [no])
diff --git a/src/external/intgcheck.m4 b/src/external/intgcheck.m4
index 1f919bd..432e14b 100644
--- a/src/external/intgcheck.m4
+++ b/src/external/intgcheck.m4
@@ -1,23 +1,27 @@
-AC_DEFUN([AM_INTGCHECK_REQ], [
-    AM_COND_IF([$1], , [
-        AC_MSG_ERROR([cannot enable integration tests: $2 not found])
-    ])
+dnl Check for for variable and fail unless value is "yes"
+dnl The second argument will be printed in error message in case of error
+dnl Usage:
+dnl     SSS_INTGCHECK_REQ(variable, message)
+
+AC_DEFUN([SSS_INTGCHECK_REQ], [
+    AS_IF([test x$$1 = xyes], , [
+        AC_MSG_ERROR([cannot enable integration tests: $2 not found])])
 ])
-AC_DEFUN([AM_ENABLE_INTGCHECK], [
+
+AC_DEFUN([SSS_ENABLE_INTGCHECK], [
     AC_ARG_ENABLE(intgcheck,
-                  [AS_HELP_STRING([--enable-intgcheck],
-                                  [enable integration test support (intgcheck target) [default=no]])],
-                  enable_intgcheck="$enableval",
-                  enable_intgcheck="no")
-    if test x"$enable_intgcheck" == xyes; then
-        AM_INTGCHECK_REQ([HAVE_UID_WRAPPER],    [uid_wrapper])
-        AM_INTGCHECK_REQ([HAVE_NSS_WRAPPER],    [nss_wrapper])
-        AM_INTGCHECK_REQ([HAVE_SLAPD],          [slapd])
-        AM_INTGCHECK_REQ([HAVE_LDAPMODIFY],     [ldapmodify])
-        AM_INTGCHECK_REQ([HAVE_FAKEROOT],       [fakeroot])
-        AM_INTGCHECK_REQ([HAVE_PYTHON2],        [python2])
-        AM_INTGCHECK_REQ([HAVE_PYTEST],         [pytest])
-        AM_INTGCHECK_REQ([HAVE_PY2MOD_LDAP],    [python-ldap])
+        [AS_HELP_STRING([--enable-intgcheck],
+                        [enable integration test support (intgcheck target) [default=no]])],
+        [enable_intgcheck="$enableval"],
+        [enable_intgcheck="no"])
+    if test x"$enable_intgcheck" = xyes; then
+        SSS_INTGCHECK_REQ([HAVE_UID_WRAPPER], [uid_wrapper])
+        SSS_INTGCHECK_REQ([HAVE_NSS_WRAPPER], [nss_wrapper])
+        SSS_INTGCHECK_REQ([HAVE_SLAPD], [slapd])
+        SSS_INTGCHECK_REQ([HAVE_LDAPMODIFY], [ldapmodify])
+        SSS_INTGCHECK_REQ([HAVE_FAKEROOT], [fakeroot])
+        SSS_INTGCHECK_REQ([HAVE_PYTHON2], [python2])
+        SSS_INTGCHECK_REQ([HAVE_PYTEST], [pytest])
+        SSS_INTGCHECK_REQ([HAVE_PY2MOD_LDAP], [python-ldap])
     fi
-    AM_CONDITIONAL([ENABLE_INTGCHECK], [test x"$enable_intgcheck" == xyes])
 ])
diff --git a/src/external/ldap.m4 b/src/external/ldap.m4
index be71f4c..33feb97 100644
--- a/src/external/ldap.m4
+++ b/src/external/ldap.m4
@@ -93,6 +93,5 @@ LIBS=$SAVE_LIBS
 AC_PATH_PROG([SLAPD], [slapd], ,
              [$PATH$PATH_SEPARATOR/usr/sbin$PATH_SEPARATOR])
 AC_SUBST([SLAPD])
-AM_CONDITIONAL([HAVE_SLAPD], [test x"$SLAPD" != x])
-AC_CHECK_PROG([HAVE_LDAPMODIFY], [ldapmodify], [true], [false])
-AM_CONDITIONAL([HAVE_LDAPMODIFY], [$HAVE_LDAPMODIFY])
+AS_IF([test -n "$SLAPD"], [HAVE_SLAPD=yes], [HAVE_SLAPD=no])
+AC_CHECK_PROG([HAVE_LDAPMODIFY], [ldapmodify], [yes], [no])
diff --git a/src/external/pytest.m4 b/src/external/pytest.m4
index 00f28e5..2d7c0dd 100644
--- a/src/external/pytest.m4
+++ b/src/external/pytest.m4
@@ -1,3 +1,2 @@
 AC_PATH_PROG([PYTEST], [py.test])
-AC_SUBST([PYTEST])
-AM_CONDITIONAL([HAVE_PYTEST], [test x"$PYTEST" != x])
+AS_IF([test -n "$PYTEST"], [HAVE_PYTEST=yes], [HAVE_PYTEST=no])


More information about the sssd-devel mailing list