On (22/05/15 17:55), Nikolai Kondrashov wrote:
On 05/22/2015 05:51 PM, Michal Židek wrote:
>On 05/22/2015 04:33 PM, Michal Židek wrote:
>>On 05/19/2015 10:17 PM, Nikolai Kondrashov wrote:
>>>On 05/18/2015 04:19 PM, Nikolai Kondrashov wrote:
>>>>On 05/15/2015 08:19 PM, Nikolai Kondrashov wrote:
>>>>>On 05/15/2015 07:14 PM, Michal Židek wrote:
>>>>>>Well they are the first tests that use this new code, so they
>>>>>>will be examples for others when writing new tests. It will
>>>>>>be helpful for developers to see the function in real context.
>>>>>>So I think it is worth the effort and the tests can be really
>>>>>>simple. Additionally we will test the assert_ functions
>>>>>>themselves this way (this is IMO important).
>>>>>
>>>>>Alright, will do.
>>>>
>>>>I can't think of good real tests using all of the functions, so
I'll
>>>>write a
>>>>separate test module explicitly testing all ent.py assert_* functions
>>>>with
>>>>passwd/group entries in files (not involving LDAP and sssd) for
>>>>simplicity and
>>>>speed.
>>>>
>>>>I think that will have a good set of examples.
>>>
>>>Alright, here is the fourth version. Changes from v3 include:
>>>
>>>* Simplified assert_passwd/group functions: removed list/name/uid/gid
>>> arguments - everything is now checked unconditionally. Moved the
>>> name/uid/gid retrieval checking out into functions, so they can be
>>>checked
>>> separately.
>>>* Added ent_test.py - an ent.py test suite running basic checks on
>>>exposed
>>> assert_* functions and providing some usage examples.
>>>* Moved pattern structure descriptions to global variables in ent.py -
>>>less
>>> copy-pasting now, more readable.
>>>* Made ent.diff* functions private by prepending their names with
>>>underscore.
>>>* Made py.test run from abs_srcdir so filenames in output are short.
>>>* Added support for supplying extra py.test args via
>>>INTGCHECK_PYTEST_ARGS
>>> variable. Can be used for limiting the set of tests to run.
>>>* Increased py.test output verbosity, so each test name is printed out.
>>>* Reduced amount of blank lines in source to one line between
>>>functions, no
>>> trailing newlines in files.
>>>
>>>CI result:
http://sssd-ci.duckdns.org/logs/job/15/51/summary.html
>>>
>>>Nick
>>>
>>
>>All my concerns were addressed in this version. Other enhancements
>>can be made as separate tickets. Will wait for the CI to finish
>>before I give ACK.
>>
>>Michal
>>
>
>One more thing.
>
>The first patch is probably not needed, because Lukas already
>sent a patch that provides the checks for python2/3 before
>(the patches that conflicted with the CI before).
>
>Lukas kindly promised that he will take a look at the build
>part again so I leave it for him to comment.
Thank you very much for the review and testing, Michal!
Regarding the first patch I can add that one of the conditionals added by it
(HAVE_PYTHON2) is used by AM_ENABLE_INTGCHECK macro in
src/external/intgcheck.m4.
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. 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 +
@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])
C) We can simplify make target intgcheck
It significantly improve user experince.
(the same as "make distcheck" or make tests in samba)
@see
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
####################
D) please find better name for "src/tests/intg/misc.py"
I will file other tickets with improvements after pushing patch to master.
These few comments can be fixed in few minutes.
BTW there is a failed intgtest
http://sssd-ci.duckdns.org/logs/job/15/90/fedora20/ci-build-debug/ci-make...
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
LS