[PATCH 0/9] test: (hopefully) better test prerequisites

[PATCH 0/9] test: (hopefully) better test prerequisites

[Dmitry Kurochkin]

Hi all.

The following patch series is an attempt to introduce proper
dependencies for external binaries in a less intrusive way than
[1].  The primary aim was to avoid changing every subtest that
uses external binaries.

There are still failing tests if a dependency is
missing (e.g. "Verify that sent messages are
saved/searchable (via FCC)" fails if there is no emacs).  It
happens because such tests depend on others which are skipped.
This issues are not addressed by this patch series.

If others do like the approach and it is pushed, I will work on
updating tests that use the old style prerequisites (atomicity).

A careful review is needed!

Regards,
  Dmitry

[1] id:"1321454035-22023-1-git-send-email-schnouki@schnouki.net"

[PATCH 1/9] test: move subtest variables reset into a dedicated function

[Dmitry Kurochkin]

Currently, there is only one such variable test_subtest_known_broken_.
But more will be added in the future.
---
 test/test-lib.sh |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 1ea7fa9..222b5e4 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -407,41 +407,41 @@ emacs_deliver_message ()
 # number of messages.
 add_email_corpus ()
 {
     rm -rf ${MAIL_DIR}
     if [ -d $TEST_DIRECTORY/corpus.mail ]; then
 	cp -a $TEST_DIRECTORY/corpus.mail ${MAIL_DIR}
     else
 	cp -a $TEST_DIRECTORY/corpus ${MAIL_DIR}
 	notmuch new >/dev/null
 	cp -a ${MAIL_DIR} $TEST_DIRECTORY/corpus.mail
     fi
 }
 
 test_begin_subtest ()
 {
     if [ -n "$inside_subtest" ]; then
 	exec 1>&6 2>&7		# Restore stdout and stderr
 	error "bug in test script: Missing test_expect_equal in ${BASH_SOURCE[1]}:${BASH_LINENO[0]}"
     fi
     test_subtest_name="$1"
-    test_subtest_known_broken_=
+    test_reset_state_
     # Remember stdout and stderr file descriptors and redirect test
     # output to the previously prepared file descriptors 3 and 4 (see
     # below)
     if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
     exec 6>&1 7>&2 >&3 2>&4
     inside_subtest=t
 }
 
 # Pass test if two arguments match
 #
 # Note: Unlike all other test_expect_* functions, this function does
 # not accept a test name. Instead, the caller should call
 # test_begin_subtest before calling this function in order to set the
 # name.
 test_expect_equal ()
 {
 	exec 1>&6 2>&7		# Restore stdout and stderr
 	inside_subtest=
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
@@ -559,84 +559,84 @@ test_ok_ () {
 test_failure_ () {
 	if test "$test_subtest_known_broken_" = "t"; then
 		test_known_broken_failure_ "$@"
 		return
 	fi
 	test_failure=$(($test_failure + 1))
 	test_failure_message_ "FAIL" "$@"
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
 	return 1
 }
 
 test_failure_message_ () {
 	say_color error "%-6s" "$1"
 	echo " $2"
 	shift 2
 	echo "$@" | sed -e 's/^/	/'
 	if test "$verbose" != "t"; then cat test.output; fi
 }
 
 test_known_broken_ok_ () {
-	test_subtest_known_broken_=
+	test_reset_state_
 	test_fixed=$(($test_fixed+1))
 	say_color pass "%-6s" "FIXED"
 	echo " $@"
 }
 
 test_known_broken_failure_ () {
-	test_subtest_known_broken_=
+	test_reset_state_
 	test_broken=$(($test_broken+1))
 	test_failure_message_ "BROKEN" "$@"
 	return 1
 }
 
 test_debug () {
 	test "$debug" = "" || eval "$1"
 }
 
 test_run_ () {
 	test_cleanup=:
 	if test "$verbose" != "t"; then exec 4>test.output 3>&4; fi
 	eval >&3 2>&4 "$1"
 	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
 	return 0
 }
 
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
 	for skp in $NOTMUCH_SKIP_TESTS
 	do
 		case $this_test.$test_count in
 		$skp)
 			to_skip=t
 		esac
 	done
 	if test -z "$to_skip" && test -n "$prereq" &&
 	   ! test_have_prereq "$prereq"
 	then
 		to_skip=t
 	fi
 	case "$to_skip" in
 	t)
-		test_subtest_known_broken_=
+		test_reset_state_
 		say_color skip >&3 "skipping test: $@"
 		say_color skip "%-6s" "SKIP"
 		echo " $1"
 		: true
 		;;
 	*)
 		false
 		;;
 	esac
 }
 
 test_subtest_known_broken () {
 	test_subtest_known_broken_=t
 }
 
 test_expect_success () {
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
 	if ! test_skip "$@"
@@ -842,40 +842,44 @@ test_emacs () {
 	if [ -z "$EMACS_SERVER" ]; then
 		EMACS_SERVER="notmuch-test-suite-$$"
 		# start a detached session with an emacs server
 		# user's TERM is given to dtach which assumes a minimally
 		# VT100-compatible terminal -- and emacs inherits that
 		TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
 			sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
 				--no-window-system \
 				--eval '(setq server-name \"$EMACS_SERVER\")' \
 				--eval '(server-start)' \
 				--eval '(orphan-watchdog $$)'" || return
 		# wait until the emacs server is up
 		until test_emacs '()' 2>/dev/null; do
 			sleep 1
 		done
 	fi
 
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
+test_reset_state_ () {
+	test_subtest_known_broken_=
+}
+
 
 find_notmuch_path ()
 {
     dir="$1"
 
     while [ -n "$dir" ]; do
 	bin="$dir/notmuch"
 	if [ -x "$bin" ]; then
 	    echo "$dir"
 	    return
 	fi
 	dir="$(dirname "$dir")"
 	if [ "$dir" = "/" ]; then
 	    break
 	fi
     done
 }
 
 # Test the binaries we have just built.  The tests are kept in
 # test/ subdirectory and are run in 'trash directory' subdirectory.
-- 
1.7.7.2

[PATCH 2/9] test: set EMACS_SERVER variable only after dtach(1) was successfully started

[Dmitry Kurochkin]

Otherwise, we can set the EMACS_SERVER and return with an error.  And
subsequent calls to test_emacs would assume that emacs server is running.
---
 test/test-lib.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 222b5e4..ff85848 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -823,50 +823,51 @@ export NOTMUCH_CONFIG=$NOTMUCH_CONFIG
 # Here's what we are using here:
 #
 # --no-init-file	Don't load users ~/.emacs
 #
 # --no-site-file	Don't load the site-wide startup stuff
 #
 # --directory		Ensure that the local elisp sources are found
 #
 # --load		Force loading of notmuch.el and test-lib.el
 
 exec emacs --no-init-file --no-site-file \
 	--directory "$TEST_DIRECTORY/../emacs" --load notmuch.el \
 	--directory "$TEST_DIRECTORY" --load test-lib.el \
 	"\$@"
 EOF
 	chmod a+x "$TMP_DIRECTORY/run_emacs"
 }
 
 test_emacs () {
 	if [ -z "$EMACS_SERVER" ]; then
-		EMACS_SERVER="notmuch-test-suite-$$"
+		server_name="notmuch-test-suite-$$"
 		# start a detached session with an emacs server
 		# user's TERM is given to dtach which assumes a minimally
 		# VT100-compatible terminal -- and emacs inherits that
 		TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
 			sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
 				--no-window-system \
-				--eval '(setq server-name \"$EMACS_SERVER\")' \
+				--eval '(setq server-name \"$server_name\")' \
 				--eval '(server-start)' \
 				--eval '(orphan-watchdog $$)'" || return
+		EMACS_SERVER="$server_name"
 		# wait until the emacs server is up
 		until test_emacs '()' 2>/dev/null; do
 			sleep 1
 		done
 	fi
 
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
 test_reset_state_ () {
 	test_subtest_known_broken_=
 }
 
 
 find_notmuch_path ()
 {
     dir="$1"
 
     while [ -n "$dir" ]; do
 	bin="$dir/notmuch"
-- 
1.7.7.2

Re: [PATCH 2/9] test: set EMACS_SERVER variable only after dtach(1) was successfully started

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On Thu, 17 Nov 2011 05:56:19 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Otherwise, we can set the EMACS_SERVER and return with an error.  And
> subsequent calls to test_emacs would assume that emacs server is running.

Hey, Dmitry.  Can you explain this a bit more?  I'm not sure I
understand the case this is protecting against.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 2/9] test: set EMACS_SERVER variable only after dtach(1) was successfully started

[Dmitry Kurochkin]

On Thu, 17 Nov 2011 01:14:13 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Thu, 17 Nov 2011 05:56:19 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Otherwise, we can set the EMACS_SERVER and return with an error.  And
> > subsequent calls to test_emacs would assume that emacs server is running.
> 
> Hey, Dmitry.  Can you explain this a bit more?  I'm not sure I
> understand the case this is protecting against.
> 

We are protectint from the case when dtach is not installed.  Then we
would return from the test_emacs function with an error "dtach ... ||
return".  EMACS_SERVER should not be set in this case.

Regards,
  Dmitry

> jamie.

[PATCH 3/9] test: add test state reset to test_expect_* functions that did not have it

[Dmitry Kurochkin]

---
 test/test-lib.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index ff85848..f21e45e 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -622,82 +622,85 @@ test_skip () {
 		test_reset_state_
 		say_color skip >&3 "skipping test: $@"
 		say_color skip "%-6s" "SKIP"
 		echo " $1"
 		: true
 		;;
 	*)
 		false
 		;;
 	esac
 }
 
 test_subtest_known_broken () {
 	test_subtest_known_broken_=t
 }
 
 test_expect_success () {
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
+	test_reset_state_
 	if ! test_skip "$@"
 	then
 		test_run_ "$2"
 		if [ "$?" = 0 -a "$eval_ret" = 0 ]
 		then
 			test_ok_ "$1"
 		else
 			test_failure_ "$@"
 		fi
 	fi
 }
 
 test_expect_code () {
 	test "$#" = 4 && { prereq=$1; shift; } || prereq=
 	test "$#" = 3 ||
 	error "bug in the test script: not 3 or 4 parameters to test-expect-code"
+	test_reset_state_
 	if ! test_skip "$@"
 	then
 		test_run_ "$3"
 		if [ "$?" = 0 -a "$eval_ret" = "$1" ]
 		then
 			test_ok_ "$2"
 		else
 			test_failure_ "$@"
 		fi
 	fi
 }
 
 # test_external runs external test scripts that provide continuous
 # test output about their progress, and succeeds/fails on
 # zero/non-zero exit code.  It outputs the test output on stdout even
 # in non-verbose mode, and announces the external script with "* run
 # <n>: ..." before running it.  When providing relative paths, keep in
 # mind that all scripts run in "trash directory".
 # Usage: test_external description command arguments...
 # Example: test_external 'Perl API' perl ../path/to/test.pl
 test_external () {
 	test "$#" = 4 && { prereq=$1; shift; } || prereq=
 	test "$#" = 3 ||
 	error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
 	descr="$1"
 	shift
+	test_reset_state_
 	if ! test_skip "$descr" "$@"
 	then
 		# Announce the script to reduce confusion about the
 		# test output that follows.
 		say_color "" " run $test_count: $descr ($*)"
 		# Run command; redirect its stderr to &4 as in
 		# test_run_, but keep its stdout on our stdout even in
 		# non-verbose mode.
 		"$@" 2>&4
 		if [ "$?" = 0 ]
 		then
 			test_ok_ "$descr"
 		else
 			test_failure_ "$descr" "$@"
 		fi
 	fi
 }
 
 # Like test_external, but in addition tests that the command generated
 # no output on stderr.
-- 
1.7.7.2

[PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

There is existing support for general prerequisites in the test suite.
But it is not very convenient to use: every test case has to keep
track for it's dependencies and they have to be explicitly listed.

The patch aims to add better support for a particular type of external
dependencies: external executables.  The main idea is to replace
missing external binaries with shell functions that have the same
name.  These functions always fail and keep track of missing
dependencies for a subtest.  The result reporting functions later can
check that an external binaries are missing and correctly report SKIP
result instead of FAIL.  The primary benefit is that the test cases do
not need to declare their dependencies or be changed in any way.
---
 test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index f21e45e..ab8c6fd 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
 # - Implicitly by specifying the prerequisite tag in the calls to
 #   test_expect_{success,failure,code}.
 #
 # The single parameter is the prerequisite tag (a simple word, in all
 # capital letters by convention).
 
 test_set_prereq () {
 	satisfied="$satisfied$1 "
 }
 satisfied=" "
 
 test_have_prereq () {
 	case $satisfied in
 	*" $1 "*)
 		: yes, have it ;;
 	*)
 		! : nope ;;
 	esac
 }
 
+# declare prerequisite for the given external binary
+test_declare_external_prereq () {
+	binary="$1"
+	test "$#" = 2 && name=$2 || name="$binary(1)"
+
+        hash $binary 2>/dev/null || eval "
+$1 () {
+	echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||
+	test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"
+	false
+}"
+}
+
 # You are not expected to call test_ok_ and test_failure_ directly, use
 # the text_expect_* functions instead.
 
 test_ok_ () {
 	if test "$test_subtest_known_broken_" = "t"; then
 		test_known_broken_ok_ "$@"
 		return
 	fi
 	test_success=$(($test_success + 1))
 	say_color pass "%-6s" "PASS"
 	echo " $@"
 }
 
 test_failure_ () {
 	if test "$test_subtest_known_broken_" = "t"; then
 		test_known_broken_failure_ "$@"
 		return
 	fi
 	test_failure=$(($test_failure + 1))
 	test_failure_message_ "FAIL" "$@"
@@ -602,82 +615,101 @@ test_run_ () {
 	return 0
 }
 
 test_skip () {
 	test_count=$(($test_count+1))
 	to_skip=
 	for skp in $NOTMUCH_SKIP_TESTS
 	do
 		case $this_test.$test_count in
 		$skp)
 			to_skip=t
 		esac
 	done
 	if test -z "$to_skip" && test -n "$prereq" &&
 	   ! test_have_prereq "$prereq"
 	then
 		to_skip=t
 	fi
 	case "$to_skip" in
 	t)
-		test_reset_state_
-		say_color skip >&3 "skipping test: $@"
-		say_color skip "%-6s" "SKIP"
-		echo " $1"
-		: true
+		test_report_skip_ "$@"
 		;;
 	*)
-		false
+		test_check_missing_external_prereqs_ "$@"
 		;;
 	esac
 }
 
+test_check_missing_external_prereqs_ () {
+	if test -n "$test_subtest_missing_external_prereqs_"; then
+		say_color skip >&3 "missing prerequisites:"
+		echo "$test_subtest_missing_external_prereqs_" >&3
+		test_report_skip_ "$@"
+	else
+		false
+	fi
+}
+
+test_report_skip_ () {
+	test_reset_state_
+	say_color skip >&3 "skipping test: $@"
+	say_color skip "%-6s" "SKIP"
+	echo " $1"
+}
+
 test_subtest_known_broken () {
 	test_subtest_known_broken_=t
 }
 
 test_expect_success () {
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
 	test_reset_state_
 	if ! test_skip "$@"
 	then
 		test_run_ "$2"
-		if [ "$?" = 0 -a "$eval_ret" = 0 ]
+		run_ret="$?"
+		# test_run_ may update missing external prerequisites
+		test_check_missing_external_prereqs_ "$@" ||
+		if [ "$run_ret" = 0 -a "$eval_ret" = 0 ]
 		then
 			test_ok_ "$1"
 		else
 			test_failure_ "$@"
 		fi
 	fi
 }
 
 test_expect_code () {
 	test "$#" = 4 && { prereq=$1; shift; } || prereq=
 	test "$#" = 3 ||
 	error "bug in the test script: not 3 or 4 parameters to test-expect-code"
 	test_reset_state_
 	if ! test_skip "$@"
 	then
 		test_run_ "$3"
-		if [ "$?" = 0 -a "$eval_ret" = "$1" ]
+		run_ret="$?"
+		# test_run_ may update missing external prerequisites,
+		test_check_missing_external_prereqs_ "$@" ||
+		if [ "$run_ret" = 0 -a "$eval_ret" = "$1" ]
 		then
 			test_ok_ "$2"
 		else
 			test_failure_ "$@"
 		fi
 	fi
 }
 
 # test_external runs external test scripts that provide continuous
 # test output about their progress, and succeeds/fails on
 # zero/non-zero exit code.  It outputs the test output on stdout even
 # in non-verbose mode, and announces the external script with "* run
 # <n>: ..." before running it.  When providing relative paths, keep in
 # mind that all scripts run in "trash directory".
 # Usage: test_external description command arguments...
 # Example: test_external 'Perl API' perl ../path/to/test.pl
 test_external () {
 	test "$#" = 4 && { prereq=$1; shift; } || prereq=
 	test "$#" = 3 ||
 	error >&5 "bug in the test script: not 3 or 4 parameters to test_external"
@@ -848,40 +880,41 @@ test_emacs () {
 		# user's TERM is given to dtach which assumes a minimally
 		# VT100-compatible terminal -- and emacs inherits that
 		TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
 			sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
 				--no-window-system \
 				--eval '(setq server-name \"$server_name\")' \
 				--eval '(server-start)' \
 				--eval '(orphan-watchdog $$)'" || return
 		EMACS_SERVER="$server_name"
 		# wait until the emacs server is up
 		until test_emacs '()' 2>/dev/null; do
 			sleep 1
 		done
 	fi
 
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
 test_reset_state_ () {
 	test_subtest_known_broken_=
+	test_subtest_missing_external_prereqs_=
 }
 
 
 find_notmuch_path ()
 {
     dir="$1"
 
     while [ -n "$dir" ]; do
 	bin="$dir/notmuch"
 	if [ -x "$bin" ]; then
 	    echo "$dir"
 	    return
 	fi
 	dir="$(dirname "$dir")"
 	if [ "$dir" = "/" ]; then
 	    break
 	fi
     done
 }
 
-- 
1.7.7.2

Re: [PATCH 4/9] test: add support for external executable dependencies

[Tomi Ollila]

On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> There is existing support for general prerequisites in the test suite.
> But it is not very convenient to use: every test case has to keep
> track for it's dependencies and they have to be explicitly listed.
> 
> The patch aims to add better support for a particular type of external
> dependencies: external executables.  The main idea is to replace
> missing external binaries with shell functions that have the same
> name.  These functions always fail and keep track of missing
> dependencies for a subtest.  The result reporting functions later can
> check that an external binaries are missing and correctly report SKIP
> result instead of FAIL.  The primary benefit is that the test cases do
> not need to declare their dependencies or be changed in any way.
> ---
>  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index f21e45e..ab8c6fd 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh
> @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
>  # - Implicitly by specifying the prerequisite tag in the calls to
>  #   test_expect_{success,failure,code}.
>  #
>  # The single parameter is the prerequisite tag (a simple word, in all
>  # capital letters by convention).
>  
>  test_set_prereq () {
>  	satisfied="$satisfied$1 "
>  }
>  satisfied=" "
>  
>  test_have_prereq () {
>  	case $satisfied in
>  	*" $1 "*)
>  		: yes, have it ;;
>  	*)
>  		! : nope ;;
>  	esac
>  }
>  
> +# declare prerequisite for the given external binary
> +test_declare_external_prereq () {
> +	binary="$1"
> +	test "$#" = 2 && name=$2 || name="$binary(1)"
> +
> +        hash $binary 2>/dev/null || eval "
> +$1 () {
> +	echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||
> +	test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"
> +	false
> +}"
> +}
> +

Does this work right ? ... the grep -e \" $name \"   -- part requires
spaces on both side of, but next line does not write trailing space...
... and is there leading space (and also the latest
$test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?

This could perhaps be written like the above test_set_prereq &
test_save_prereq:
...
        hash $binary 2>/dev/null || eval "
$binary () {
	test_missing_external_prereq_${binary}_=t
	case \$test_subtest_missing_external_prereqs_ in
        	*' $name '*) ;;
                *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"
        esac
	false
}

and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_

(I took some code from current git head.... also perhaps instead of 
setting test_missing_external_prereq_${binary}_=t, (bash) associative
arrays could be taken into use -- the eval to read that information
is a bit hairy -- well, at least that part works for sure :D )

Hmm... how about this:

        hash $binary 2>/dev/null || eval "
$binary () {
        if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
        then
        	test_missing_external_prereq_[$binary]=t
                test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"
        fi
	false
}

and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.

Tomi

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > There is existing support for general prerequisites in the test suite.
> > But it is not very convenient to use: every test case has to keep
> > track for it's dependencies and they have to be explicitly listed.
> > 
> > The patch aims to add better support for a particular type of external
> > dependencies: external executables.  The main idea is to replace
> > missing external binaries with shell functions that have the same
> > name.  These functions always fail and keep track of missing
> > dependencies for a subtest.  The result reporting functions later can
> > check that an external binaries are missing and correctly report SKIP
> > result instead of FAIL.  The primary benefit is that the test cases do
> > not need to declare their dependencies or be changed in any way.
> > ---
> >  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------
> >  1 files changed, 41 insertions(+), 8 deletions(-)
> > 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index f21e45e..ab8c6fd 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> >  # - Implicitly by specifying the prerequisite tag in the calls to
> >  #   test_expect_{success,failure,code}.
> >  #
> >  # The single parameter is the prerequisite tag (a simple word, in all
> >  # capital letters by convention).
> >  
> >  test_set_prereq () {
> >  	satisfied="$satisfied$1 "
> >  }
> >  satisfied=" "
> >  
> >  test_have_prereq () {
> >  	case $satisfied in
> >  	*" $1 "*)
> >  		: yes, have it ;;
> >  	*)
> >  		! : nope ;;
> >  	esac
> >  }
> >  
> > +# declare prerequisite for the given external binary
> > +test_declare_external_prereq () {
> > +	binary="$1"
> > +	test "$#" = 2 && name=$2 || name="$binary(1)"
> > +
> > +        hash $binary 2>/dev/null || eval "
> > +$1 () {
> > +	echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||
> > +	test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"
> > +	false
> > +}"
> > +}
> > +
> 
> Does this work right ?

It does not.

Moreover, there is a missing backslash before
$test_subtest_missing_external_prereqs_ (and that is why I did not
notice the bug during my poor testing).

> ... the grep -e \" $name \"   -- part requires
> spaces on both side of, but next line does not write trailing space...
> ... and is there leading space (and also the latest
> $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?
> 
> This could perhaps be written like the above test_set_prereq &
> test_save_prereq:
> ...
>         hash $binary 2>/dev/null || eval "
> $binary () {
> 	test_missing_external_prereq_${binary}_=t
> 	case \$test_subtest_missing_external_prereqs_ in
>         	*' $name '*) ;;
>                 *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"
>         esac
> 	false
> }
> 
> and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> 

Well, this approach would obviously be better, at least because it does
work.  But IMO it is too complex, and essentially has the same problem
as the current code: it mixes the check with diagnostic message.

We already have a proper way to check if dependency is missing already:
test_missing_external_prereq_${binary}_.  We should check it and add the
binary to test_subtest_missing_external_prereqs_ if it is not set.  And
move setting of test_missing_external_prereq_${binary}_ below.

> (I took some code from current git head.... also perhaps instead of 
> setting test_missing_external_prereq_${binary}_=t, (bash) associative
> arrays could be taken into use -- the eval to read that information
> is a bit hairy -- well, at least that part works for sure :D )
> 

Yes!  I like using hash here.

> Hmm... how about this:
> 
>         hash $binary 2>/dev/null || eval "
> $binary () {
>         if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
>         then
>         	test_missing_external_prereq_[$binary]=t
>                 test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"
>         fi
> 	false
> }
> 

There is some inconsistent indenting in the above code (mixed tabs and
spaces).  Also test_require_external_prereq() should be changed as well.

Otherwise I like it.  Would you please submit a patch?

Regards,
  Dmitry

> and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.
> 
> Tomi

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > There is existing support for general prerequisites in the test suite.
> > > But it is not very convenient to use: every test case has to keep
> > > track for it's dependencies and they have to be explicitly listed.
> > > 
> > > The patch aims to add better support for a particular type of external
> > > dependencies: external executables.  The main idea is to replace
> > > missing external binaries with shell functions that have the same
> > > name.  These functions always fail and keep track of missing
> > > dependencies for a subtest.  The result reporting functions later can
> > > check that an external binaries are missing and correctly report SKIP
> > > result instead of FAIL.  The primary benefit is that the test cases do
> > > not need to declare their dependencies or be changed in any way.
> > > ---
> > >  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------
> > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > index f21e45e..ab8c6fd 100755
> > > --- a/test/test-lib.sh
> > > +++ b/test/test-lib.sh
> > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > >  #   test_expect_{success,failure,code}.
> > >  #
> > >  # The single parameter is the prerequisite tag (a simple word, in all
> > >  # capital letters by convention).
> > >  
> > >  test_set_prereq () {
> > >  	satisfied="$satisfied$1 "
> > >  }
> > >  satisfied=" "
> > >  
> > >  test_have_prereq () {
> > >  	case $satisfied in
> > >  	*" $1 "*)
> > >  		: yes, have it ;;
> > >  	*)
> > >  		! : nope ;;
> > >  	esac
> > >  }
> > >  
> > > +# declare prerequisite for the given external binary
> > > +test_declare_external_prereq () {
> > > +	binary="$1"
> > > +	test "$#" = 2 && name=$2 || name="$binary(1)"
> > > +
> > > +        hash $binary 2>/dev/null || eval "
> > > +$1 () {
> > > +	echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||
> > > +	test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"
> > > +	false
> > > +}"
> > > +}
> > > +
> > 
> > Does this work right ?
> 
> It does not.
> 
> Moreover, there is a missing backslash before
> $test_subtest_missing_external_prereqs_ (and that is why I did not
> notice the bug during my poor testing).
> 
> > ... the grep -e \" $name \"   -- part requires
> > spaces on both side of, but next line does not write trailing space...
> > ... and is there leading space (and also the latest
> > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?
> > 
> > This could perhaps be written like the above test_set_prereq &
> > test_save_prereq:
> > ...
> >         hash $binary 2>/dev/null || eval "
> > $binary () {
> > 	test_missing_external_prereq_${binary}_=t
> > 	case \$test_subtest_missing_external_prereqs_ in
> >         	*' $name '*) ;;
> >                 *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"
> >         esac
> > 	false
> > }
> > 
> > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > 
> 
> Well, this approach would obviously be better, at least because it does
> work.  But IMO it is too complex, and essentially has the same problem
> as the current code: it mixes the check with diagnostic message.
> 
> We already have a proper way to check if dependency is missing already:
> test_missing_external_prereq_${binary}_.  We should check it and add the
> binary to test_subtest_missing_external_prereqs_ if it is not set.  And
> move setting of test_missing_external_prereq_${binary}_ below.
> 
> > (I took some code from current git head.... also perhaps instead of 
> > setting test_missing_external_prereq_${binary}_=t, (bash) associative
> > arrays could be taken into use -- the eval to read that information
> > is a bit hairy -- well, at least that part works for sure :D )
> > 
> 
> Yes!  I like using hash here.
> 
> > Hmm... how about this:
> > 
> >         hash $binary 2>/dev/null || eval "
> > $binary () {
> >         if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
> >         then
> >         	test_missing_external_prereq_[$binary]=t
> >                 test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"
> >         fi
> > 	false
> > }
> > 
> 
> There is some inconsistent indenting in the above code (mixed tabs and
> spaces).  Also test_require_external_prereq() should be changed as well.
> 
> Otherwise I like it.  Would you please submit a patch?
> 

Actually, test_missing_external_prereq_${binary}_ must be set outside of
the $binary() function.  It indicates that the binary is missing, not
that it is required.  I will send some patches to fix these bugs.  Then
you can change it to use hashes.

Regards,
  Dmitry

> Regards,
>   Dmitry
> 
> > and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.
> > 
> > Tomi

Re: [PATCH 4/9] test: add support for external executable dependencies

[Dmitry Kurochkin]

On Tue, 29 Nov 2011 02:13:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Tue, 29 Nov 2011 01:53:49 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > On Mon, 28 Nov 2011 23:16:27 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > > On Thu, 17 Nov 2011 05:56:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > > There is existing support for general prerequisites in the test suite.
> > > > But it is not very convenient to use: every test case has to keep
> > > > track for it's dependencies and they have to be explicitly listed.
> > > > 
> > > > The patch aims to add better support for a particular type of external
> > > > dependencies: external executables.  The main idea is to replace
> > > > missing external binaries with shell functions that have the same
> > > > name.  These functions always fail and keep track of missing
> > > > dependencies for a subtest.  The result reporting functions later can
> > > > check that an external binaries are missing and correctly report SKIP
> > > > result instead of FAIL.  The primary benefit is that the test cases do
> > > > not need to declare their dependencies or be changed in any way.
> > > > ---
> > > >  test/test-lib.sh |   49 +++++++++++++++++++++++++++++++++++++++++--------
> > > >  1 files changed, 41 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > > index f21e45e..ab8c6fd 100755
> > > > --- a/test/test-lib.sh
> > > > +++ b/test/test-lib.sh
> > > > @@ -526,40 +526,53 @@ notmuch_json_show_sanitize ()
> > > >  # - Implicitly by specifying the prerequisite tag in the calls to
> > > >  #   test_expect_{success,failure,code}.
> > > >  #
> > > >  # The single parameter is the prerequisite tag (a simple word, in all
> > > >  # capital letters by convention).
> > > >  
> > > >  test_set_prereq () {
> > > >  	satisfied="$satisfied$1 "
> > > >  }
> > > >  satisfied=" "
> > > >  
> > > >  test_have_prereq () {
> > > >  	case $satisfied in
> > > >  	*" $1 "*)
> > > >  		: yes, have it ;;
> > > >  	*)
> > > >  		! : nope ;;
> > > >  	esac
> > > >  }
> > > >  
> > > > +# declare prerequisite for the given external binary
> > > > +test_declare_external_prereq () {
> > > > +	binary="$1"
> > > > +	test "$#" = 2 && name=$2 || name="$binary(1)"
> > > > +
> > > > +        hash $binary 2>/dev/null || eval "
> > > > +$1 () {
> > > > +	echo -n \"\$test_subtest_missing_external_prereqs_\" | grep -e \" $name \" ||
> > > > +	test_subtest_missing_external_prereqs_=\"$test_subtest_missing_external_prereqs_ $name\"
> > > > +	false
> > > > +}"
> > > > +}
> > > > +
> > > 
> > > Does this work right ?
> > 
> > It does not.
> > 
> > Moreover, there is a missing backslash before
> > $test_subtest_missing_external_prereqs_ (and that is why I did not
> > notice the bug during my poor testing).
> > 
> > > ... the grep -e \" $name \"   -- part requires
> > > spaces on both side of, but next line does not write trailing space...
> > > ... and is there leading space (and also the latest
> > > $test_subtest_missing_external_prereqs_ (just before 'false') is evaluated) ?
> > > 
> > > This could perhaps be written like the above test_set_prereq &
> > > test_save_prereq:
> > > ...
> > >         hash $binary 2>/dev/null || eval "
> > > $binary () {
> > > 	test_missing_external_prereq_${binary}_=t
> > > 	case \$test_subtest_missing_external_prereqs_ in
> > >         	*' $name '*) ;;
> > >                 *) test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_$name \"
> > >         esac
> > > 	false
> > > }
> > > 
> > > and  test_subtest_missing_external_prereqs_=' '  done in test_reset_state_
> > > 
> > 
> > Well, this approach would obviously be better, at least because it does
> > work.  But IMO it is too complex, and essentially has the same problem
> > as the current code: it mixes the check with diagnostic message.
> > 
> > We already have a proper way to check if dependency is missing already:
> > test_missing_external_prereq_${binary}_.  We should check it and add the
> > binary to test_subtest_missing_external_prereqs_ if it is not set.  And
> > move setting of test_missing_external_prereq_${binary}_ below.
> > 
> > > (I took some code from current git head.... also perhaps instead of 
> > > setting test_missing_external_prereq_${binary}_=t, (bash) associative
> > > arrays could be taken into use -- the eval to read that information
> > > is a bit hairy -- well, at least that part works for sure :D )
> > > 
> > 
> > Yes!  I like using hash here.
> > 
> > > Hmm... how about this:
> > > 
> > >         hash $binary 2>/dev/null || eval "
> > > $binary () {
> > >         if [ -z \"\${test_missing_external_prereq_[$binary]}\" ]
> > >         then
> > >         	test_missing_external_prereq_[$binary]=t
> > >                 test_subtest_missing_external_prereqs_=\"\$test_subtest_missing_external_prereqs_ $name\"
> > >         fi
> > > 	false
> > > }
> > > 
> > 
> > There is some inconsistent indenting in the above code (mixed tabs and
> > spaces).  Also test_require_external_prereq() should be changed as well.
> > 
> > Otherwise I like it.  Would you please submit a patch?
> > 
> 
> Actually, test_missing_external_prereq_${binary}_ must be set outside of
> the $binary() function.  It indicates that the binary is missing, not
> that it is required.  I will send some patches to fix these bugs.  Then
> you can change it to use hashes.

See id:1322520067-15227-1-git-send-email-dmitry.kurochkin@gmail.com.

Regards,
  Dmitry

> 
> Regards,
>   Dmitry
> 
> > Regards,
> >   Dmitry
> > 
> > > and test_subtest_missing_external_prereqs_ cleared in test_reset_state_ like now.
> > > 
> > > Tomi

[PATCH 5/9] test: fix "skipping test" verbose output

[Dmitry Kurochkin]

---
 test/test-lib.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index ab8c6fd..acac8ca 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -635,41 +635,42 @@ test_skip () {
 		test_report_skip_ "$@"
 		;;
 	*)
 		test_check_missing_external_prereqs_ "$@"
 		;;
 	esac
 }
 
 test_check_missing_external_prereqs_ () {
 	if test -n "$test_subtest_missing_external_prereqs_"; then
 		say_color skip >&3 "missing prerequisites:"
 		echo "$test_subtest_missing_external_prereqs_" >&3
 		test_report_skip_ "$@"
 	else
 		false
 	fi
 }
 
 test_report_skip_ () {
 	test_reset_state_
-	say_color skip >&3 "skipping test: $@"
+	say_color skip >&3 "skipping test:"
+	echo " $@" >&3
 	say_color skip "%-6s" "SKIP"
 	echo " $1"
 }
 
 test_subtest_known_broken () {
 	test_subtest_known_broken_=t
 }
 
 test_expect_success () {
 	test "$#" = 3 && { prereq=$1; shift; } || prereq=
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test-expect-success"
 	test_reset_state_
 	if ! test_skip "$@"
 	then
 		test_run_ "$2"
 		run_ret="$?"
 		# test_run_ may update missing external prerequisites
 		test_check_missing_external_prereqs_ "$@" ||
 		if [ "$run_ret" = 0 -a "$eval_ret" = 0 ]
-- 
1.7.7.2

[PATCH 6/9] test: skip all subtests if external dependencies are missing during init

[Dmitry Kurochkin]

Some tests (e.g. crypto) do a common initialization required for all
subtests.  The patch adds a check for missing external dependencies
during this initialization.  If any prerequisites are missing, all
subtests are skipped.

The check is run on the first call of test_reset_state_ function, so
no changes for the tests are needed.
---
 test/test-lib.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index acac8ca..c11493d 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -880,44 +880,54 @@ test_emacs () {
 		# start a detached session with an emacs server
 		# user's TERM is given to dtach which assumes a minimally
 		# VT100-compatible terminal -- and emacs inherits that
 		TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
 			sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
 				--no-window-system \
 				--eval '(setq server-name \"$server_name\")' \
 				--eval '(server-start)' \
 				--eval '(orphan-watchdog $$)'" || return
 		EMACS_SERVER="$server_name"
 		# wait until the emacs server is up
 		until test_emacs '()' 2>/dev/null; do
 			sleep 1
 		done
 	fi
 
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
 
 test_reset_state_ () {
+	test -z "$test_init_done_" && test_init_
+
 	test_subtest_known_broken_=
 	test_subtest_missing_external_prereqs_=
 }
 
+# called once before the first subtest
+test_init_ () {
+	test_init_done_=t
+
+	# skip all tests if there were external prerequisites missing during init
+	test_check_missing_external_prereqs_ "all tests in $this_test" && test_done
+}
+
 
 find_notmuch_path ()
 {
     dir="$1"
 
     while [ -n "$dir" ]; do
 	bin="$dir/notmuch"
 	if [ -x "$bin" ]; then
 	    echo "$dir"
 	    return
 	fi
 	dir="$(dirname "$dir")"
 	if [ "$dir" = "/" ]; then
 	    break
 	fi
     done
 }
 
 # Test the binaries we have just built.  The tests are kept in
 # test/ subdirectory and are run in 'trash directory' subdirectory.
-- 
1.7.7.2

[PATCH 7/9] test: declare external dependencies for the tests

[Dmitry Kurochkin]

That are: dtach(1), emacs(1), emacsclient(1), gdb(1) and gpg(1).
---
 test/test-lib.sh |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index c11493d..840c86c 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1101,20 +1101,27 @@ case $(uname -s) in
 	pwd () {
 		builtin pwd -W
 	}
 	# no POSIX permissions
 	# backslashes in pathspec are converted to '/'
 	# exec does not inherit the PID
 	;;
 *)
 	test_set_prereq POSIXPERM
 	test_set_prereq BSLASHPSPEC
 	test_set_prereq EXECKEEPSPID
 	;;
 esac
 
 test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PYTHON" && test_set_prereq PYTHON
 
 # test whether the filesystem supports symbolic links
 ln -s x y 2>/dev/null && test -h y 2>/dev/null && test_set_prereq SYMLINKS
 rm -f y
+
+# declare prerequisites for external binaries used in tests
+test_declare_external_prereq dtach
+test_declare_external_prereq emacs
+test_declare_external_prereq emacsclient
+test_declare_external_prereq gdb
+test_declare_external_prereq gpg
-- 
1.7.7.2

[PATCH 8/9] test: check if emacs is available in the beginning of test_emacs

[Dmitry Kurochkin]

Unfortunately, this is needed to avoid the emacs waiting loop.
---
 test/test-lib.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index 840c86c..5bd5bd6 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -858,40 +858,44 @@ export NOTMUCH_CONFIG=$NOTMUCH_CONFIG
 
 # Here's what we are using here:
 #
 # --no-init-file	Don't load users ~/.emacs
 #
 # --no-site-file	Don't load the site-wide startup stuff
 #
 # --directory		Ensure that the local elisp sources are found
 #
 # --load		Force loading of notmuch.el and test-lib.el
 
 exec emacs --no-init-file --no-site-file \
 	--directory "$TEST_DIRECTORY/../emacs" --load notmuch.el \
 	--directory "$TEST_DIRECTORY" --load test-lib.el \
 	"\$@"
 EOF
 	chmod a+x "$TMP_DIRECTORY/run_emacs"
 }
 
 test_emacs () {
+	# test dependencies beforehand to avoid the waiting loop below
+	which emacs >/dev/null || emacs || return
+	which emacsclient >/dev/null || emacsclient || return
+
 	if [ -z "$EMACS_SERVER" ]; then
 		server_name="notmuch-test-suite-$$"
 		# start a detached session with an emacs server
 		# user's TERM is given to dtach which assumes a minimally
 		# VT100-compatible terminal -- and emacs inherits that
 		TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
 			sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
 				--no-window-system \
 				--eval '(setq server-name \"$server_name\")' \
 				--eval '(server-start)' \
 				--eval '(orphan-watchdog $$)'" || return
 		EMACS_SERVER="$server_name"
 		# wait until the emacs server is up
 		until test_emacs '()' 2>/dev/null; do
 			sleep 1
 		done
 	fi
 
 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
 }
-- 
1.7.7.2

Re: [PATCH 8/9] test: check if emacs is available in the beginning of test_emacs

[Tomi Ollila]

On Thu, 17 Nov 2011 05:56:25 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Unfortunately, this is needed to avoid the emacs waiting loop.
> ---

>  test/test-lib.sh |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)

[ context reduced ]

> diff --git a/test/test-lib.sh b/test/test-lib.sh
> index 840c86c..5bd5bd6 100755
> --- a/test/test-lib.sh
> +++ b/test/test-lib.sh

I have to patch the full set to my clone of the repo and do 
fuller review, but until that...

>  test_emacs () {
> +	# test dependencies beforehand to avoid the waiting loop below
> +	which emacs >/dev/null || emacs || return
> +	which emacsclient >/dev/null || emacsclient || return
> +

If emacs not found using which (what happened to hash), then try
to execute emacs (??) and if that fails return.

Same thing with emacsclient.

In case of emacs: DISPLAY set, TERM=dumb -- emacs starts interactively
on my desktop; grep DISPLAY test/* yields nothing. (maybe we should unset
DISPLAY for tests ?). And, if DISPLAY unset, emacs exits with nonzero
value, making || return happen anyway.

... and this is tested every time test_emacs() is executed...

>  	if [ -z "$EMACS_SERVER" ]; then
>  		server_name="notmuch-test-suite-$$"
> -- 
> 1.7.7.2

Tomi

Re: [PATCH 8/9] test: check if emacs is available in the beginning of test_emacs

[Dmitry Kurochkin]

On Thu, 17 Nov 2011 11:43:36 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 17 Nov 2011 05:56:25 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Unfortunately, this is needed to avoid the emacs waiting loop.
> > ---
> 
> >  test/test-lib.sh |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> [ context reduced ]
> 
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index 840c86c..5bd5bd6 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> 
> I have to patch the full set to my clone of the repo and do 
> fuller review, but until that...
> 
> >  test_emacs () {
> > +	# test dependencies beforehand to avoid the waiting loop below
> > +	which emacs >/dev/null || emacs || return
> > +	which emacsclient >/dev/null || emacsclient || return
> > +
> 
> If emacs not found using which (what happened to hash), then try
> to execute emacs (??) and if that fails return.
> 
> Same thing with emacsclient.
> 
> In case of emacs: DISPLAY set, TERM=dumb -- emacs starts interactively
> on my desktop; grep DISPLAY test/* yields nothing. (maybe we should unset
> DISPLAY for tests ?). And, if DISPLAY unset, emacs exits with nonzero
> value, making || return happen anyway.
> 
> ... and this is tested every time test_emacs() is executed...
> 

Either you did not apply other patches, or I made some stupid mistake.

I am not very happy with the above lines.  They feel hackish.  Perhaps
others would suggest something better.

Here is a more detailed explanation of what they do (or should do):

  which emacs || # check that emacs binary exists  (hash would succeed
                 # for functions)
  emacs || # if the binary does not exist, run the "replacement"
           # function which would register the missing dependency
  return # and return with an error

Regards,
  Dmitry

> >  	if [ -z "$EMACS_SERVER" ]; then
> >  		server_name="notmuch-test-suite-$$"
> > -- 
> > 1.7.7.2
> 
> Tomi

Re: [PATCH 8/9] test: check if emacs is available in the beginning of test_emacs

[Dmitry Kurochkin]

On Thu, 17 Nov 2011 15:13:40 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Thu, 17 Nov 2011 11:43:36 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > On Thu, 17 Nov 2011 05:56:25 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > Unfortunately, this is needed to avoid the emacs waiting loop.
> > > ---
> > 
> > >  test/test-lib.sh |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > [ context reduced ]
> > 
> > > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > > index 840c86c..5bd5bd6 100755
> > > --- a/test/test-lib.sh
> > > +++ b/test/test-lib.sh
> > 
> > I have to patch the full set to my clone of the repo and do 
> > fuller review, but until that...
> > 
> > >  test_emacs () {
> > > +	# test dependencies beforehand to avoid the waiting loop below
> > > +	which emacs >/dev/null || emacs || return
> > > +	which emacsclient >/dev/null || emacsclient || return
> > > +
> > 
> > If emacs not found using which (what happened to hash), then try
> > to execute emacs (??) and if that fails return.
> > 
> > Same thing with emacsclient.
> > 
> > In case of emacs: DISPLAY set, TERM=dumb -- emacs starts interactively
> > on my desktop; grep DISPLAY test/* yields nothing. (maybe we should unset
> > DISPLAY for tests ?). And, if DISPLAY unset, emacs exits with nonzero
> > value, making || return happen anyway.
> > 
> > ... and this is tested every time test_emacs() is executed...
> > 
> 
> Either you did not apply other patches, or I made some stupid mistake.
> 
> I am not very happy with the above lines.  They feel hackish.  Perhaps
> others would suggest something better.
> 
> Here is a more detailed explanation of what they do (or should do):
> 
>   which emacs || # check that emacs binary exists  (hash would succeed
>                  # for functions)
>   emacs || # if the binary does not exist, run the "replacement"
>            # function which would register the missing dependency
>   return # and return with an error
> 

I have sent a new version of this patch series [1] which moves the above
tricks into a dedicated function.

Regards,
  Dmitry

[1] id:"1321535163-4895-1-git-send-email-dmitry.kurochkin@gmail.com"

> Regards,
>   Dmitry
> 
> > >  	if [ -z "$EMACS_SERVER" ]; then
> > >  		server_name="notmuch-test-suite-$$"
> > > -- 
> > > 1.7.7.2
> > 
> > Tomi

[PATCH 9/9] test: fix "Stashing in notmuch-search" test when emacs is not available

[Dmitry Kurochkin]

If emacs is not available, test_expect_equal would be called with only
one argument.  The patch fixes this by quoting the (possibly empty)
$(cat OUTPUT) argument.
---
 test/emacs |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/test/emacs b/test/emacs
index 75a0a74..9883f40 100755
--- a/test/emacs
+++ b/test/emacs
@@ -364,41 +364,41 @@ test_emacs '(notmuch-show "id:\"bought\"")
         (switch-to-buffer
           (generate-new-buffer "*test-stashing*"))
         (dotimes (i 9)
           (yank)
           (insert "\n")
           (rotate-yank-pointer 1))
         (reverse-region (point-min) (point-max))
 	    (test-output)'
 sed -i -e 's/^.*tmp.emacs\/mail.*$/FILENAME/' OUTPUT
 test_expect_equal_file OUTPUT $EXPECTED/emacs-stashing
 
 test_begin_subtest "Stashing in notmuch-search"
 test_emacs '(notmuch-search "id:\"bought\"")
         (notmuch-test-wait)
         (notmuch-search-stash-thread-id)
         (switch-to-buffer
           (generate-new-buffer "*test-stashing*"))
         (yank)
 	    (test-output)'
 sed -i -e 's/^thread:.*$/thread:XXX/' OUTPUT
-test_expect_equal $(cat OUTPUT) "thread:XXX"
+test_expect_equal "$(cat OUTPUT)" "thread:XXX"
 
 test_begin_subtest 'Hiding message following HTML part'
 test_subtest_known_broken
 id='html-message@notmuchmail.org'
 emacs_deliver_message \
     'HTML message' \
     '' \
     "(message-goto-eoh)
      (insert \"Message-ID: <$id>\n\")
      (message-goto-body)
      (mml-insert-part \"text/html\")
      (insert \"<body>This is a test HTML message</body>\")"
 emacs_deliver_message \
     'Reply to HTML message' \
     'This is a reply to the test HTML message' \
     "(message-goto-eoh)
      (insert \"In-Reply-To: <$id>\n\")"
 test_emacs "(notmuch-show \"id:$id\") \
             (notmuch-show-next-message) \
             (command-execute (key-binding (kbd \"RET\"))) \
-- 
1.7.7.2

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

On Thu, 17 Nov 2011 05:56:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The following patch series is an attempt to introduce proper
> dependencies for external binaries in a less intrusive way than
> [1].  The primary aim was to avoid changing every subtest that
> uses external binaries.

Hey, Dmitry.  Thanks so much for reworking Pieter's original test suite
improvements.  I think this new approach is a bit less invasive and a
little more elegant to work with.

I've looked through the patchs, and they all look ok on first glance
(barring any possibly needed modifications that I'm not seeing).  I have
a couple of comments to follow.

However, when I tried to test the tests with the patch applied I ran
into one problem.  If I try to run the test suite with dtach
uninstalled, it looks like I'm experiencing a hang on
emacs_deliver_message call.  I wonder if either emacs_deliver_message or
test_emacs is not doing the right thing in the case of no dtach.  I
think emacs_deliver_message should somehow not require dtach, since it's
not actually testing any display stuff, but because it's currently using
test_emacs, it is somehow implicitly depending on it.  Any thoughts on
how to fix that?

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Dmitry Kurochkin]

Hi Jameson.

On Thu, 17 Nov 2011 01:14:07 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Thu, 17 Nov 2011 05:56:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > The following patch series is an attempt to introduce proper
> > dependencies for external binaries in a less intrusive way than
> > [1].  The primary aim was to avoid changing every subtest that
> > uses external binaries.
> 
> Hey, Dmitry.  Thanks so much for reworking Pieter's original test suite
> improvements.  I think this new approach is a bit less invasive and a
> little more elegant to work with.
> 
> I've looked through the patchs, and they all look ok on first glance
> (barring any possibly needed modifications that I'm not seeing).  I have
> a couple of comments to follow.
> 
> However, when I tried to test the tests with the patch applied I ran
> into one problem.  If I try to run the test suite with dtach
> uninstalled, it looks like I'm experiencing a hang on
> emacs_deliver_message call.  I wonder if either emacs_deliver_message or
> test_emacs is not doing the right thing in the case of no dtach.  I
> think emacs_deliver_message should somehow not require dtach, since it's
> not actually testing any display stuff, but because it's currently using
> test_emacs, it is somehow implicitly depending on it.  Any thoughts on
> how to fix that?
> 

I bet you stumbled upon a hanging smtp-dummy.  There is a patch [1] to
fix that.  Hopefully it would get pushed soon.

As for not requiring dtach for emacs_deliver_message, that is definitely
possible.  Currently, there is only one way to run emacs to keep it
simple.  And I am not sure if running emacs_deliver_message without
dtach costs extra code.

Regards,
  Dmitry

[1] id:"yf639dnsqtc.fsf@taco2.nixu.fi"

> jamie.

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Tomi Ollila]

On Thu, 17 Nov 2011 01:14:07 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> 
> However, when I tried to test the tests with the patch applied I ran
> into one problem.  If I try to run the test suite with dtach
> uninstalled, it looks like I'm experiencing a hang on
> emacs_deliver_message call.  I wonder if either emacs_deliver_message or
> test_emacs is not doing the right thing in the case of no dtach.  I
> think emacs_deliver_message should somehow not require dtach, since it's
> not actually testing any display stuff, but because it's currently using
> test_emacs, it is somehow implicitly depending on it.  Any thoughts on
> how to fix that?

Quick thought

mk_skip_test_emacs () 
{ test_emacs () 
  { 
	echo SKIPPED
        false
  }
  test_emacs
}

mk_run_test_emacs () 
{ test_emacs () 
  {
	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
  }
  test_emacs
}

test_emacs ()
{
        which dtach >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
        which emacs >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
        which emacsclient >/dev/null 2>&1 || { mk_skip_test_emacs; return; }

        EMACS_SERVER="notmuch-test-suite-$$"
	# start a detached session with an emacs server
	# user's TERM is given to dtach which assumes a minimally
	# VT100-compatible terminal -- and emacs inherits that
	TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
		sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
			--no-window-system \
			--eval '(setq server-name \"$EMACS_SERVER\")' \
			--eval '(server-start)' \
			--eval '(orphan-watchdog $$)'" ||
                        { mk_skip_test_emacs; return; }
	# wait until the emacs server is up
        for _ in 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 20 1 2 3 4 5 6 7 8 9 30
        do 
        	if test_emacs '()' 2>/dev/null
                then
                        mk_run_test_emacs
                        return
                fi
        done
        mk_skip_test_emacs
}

I.e. dynamically, at run-time, re-create test_emacs function...

> 
> jamie.

Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Tomi Ollila]

On Thu, 17 Nov 2011 14:20:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:

FIXES INLINE; sorry about the duplication.

> On Thu, 17 Nov 2011 01:14:07 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > 
> > However, when I tried to test the tests with the patch applied I ran
> > into one problem.  If I try to run the test suite with dtach
> > uninstalled, it looks like I'm experiencing a hang on
> > emacs_deliver_message call.  I wonder if either emacs_deliver_message or
> > test_emacs is not doing the right thing in the case of no dtach.  I
> > think emacs_deliver_message should somehow not require dtach, since it's
> > not actually testing any display stuff, but because it's currently using
> > test_emacs, it is somehow implicitly depending on it.  Any thoughts on
> > how to fix that?
> 
> Quick thought
> 
> mk_skip_test_emacs () 
> { test_emacs () 
>   { 
> 	echo SKIPPED
>       false
>   }
>   test_emacs
> }
> 
> mk_run_test_emacs () 
> { test_emacs () 
>   {
> 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>   }
>   test_emacs
    test_emacs "$@"
> }
> 
> test_emacs ()
> {
>       which dtach >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>       which emacs >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>       which emacsclient >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
> 
>       EMACS_SERVER="notmuch-test-suite-$$"
> 	# start a detached session with an emacs server
> 	# user's TERM is given to dtach which assumes a minimally
> 	# VT100-compatible terminal -- and emacs inherits that
> 	TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
> 		sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
> 			--no-window-system \
> 			--eval '(setq server-name \"$EMACS_SERVER\")' \
> 			--eval '(server-start)' \
> 			--eval '(orphan-watchdog $$)'" ||
>                         { mk_skip_test_emacs; return; }
> 	# wait until the emacs server is up
>       for _ in 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 20 1 2 3 4 5 6 7 8 9 30
>       do 
>         	if test_emacs '()' 2>/dev/null
>                 then
>                         mk_run_test_emacs
                          mk_run_test_emacs "$@"
>                         return
>                 fi
>       done
>       mk_skip_test_emacs
> }
> 
> I.e. dynamically, at run-time, re-create test_emacs function...
> 
> > 
> > jamie.
> 
> Tomi

Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Dmitry Kurochkin]

Hi Tomi.

On Thu, 17 Nov 2011 14:20:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 17 Nov 2011 01:14:07 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > 
> > However, when I tried to test the tests with the patch applied I ran
> > into one problem.  If I try to run the test suite with dtach
> > uninstalled, it looks like I'm experiencing a hang on
> > emacs_deliver_message call.  I wonder if either emacs_deliver_message or
> > test_emacs is not doing the right thing in the case of no dtach.  I
> > think emacs_deliver_message should somehow not require dtach, since it's
> > not actually testing any display stuff, but because it's currently using
> > test_emacs, it is somehow implicitly depending on it.  Any thoughts on
> > how to fix that?
> 
> Quick thought
> 
> mk_skip_test_emacs () 
> { test_emacs () 
>   { 
> 	echo SKIPPED
>         false
>   }
>   test_emacs
> }
> 
> mk_run_test_emacs () 
> { test_emacs () 
>   {
> 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>   }
>   test_emacs
> }
> 
> test_emacs ()
> {
>         which dtach >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>         which emacs >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>         which emacsclient >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
> 
>         EMACS_SERVER="notmuch-test-suite-$$"
> 	# start a detached session with an emacs server
> 	# user's TERM is given to dtach which assumes a minimally
> 	# VT100-compatible terminal -- and emacs inherits that
> 	TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
> 		sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
> 			--no-window-system \
> 			--eval '(setq server-name \"$EMACS_SERVER\")' \
> 			--eval '(server-start)' \
> 			--eval '(orphan-watchdog $$)'" ||
>                         { mk_skip_test_emacs; return; }
> 	# wait until the emacs server is up
>         for _ in 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 20 1 2 3 4 5 6 7 8 9 30
>         do 
>         	if test_emacs '()' 2>/dev/null
>                 then
>                         mk_run_test_emacs
>                         return
>                 fi
>         done
>         mk_skip_test_emacs
> }
> 
> I.e. dynamically, at run-time, re-create test_emacs function...
> 

Could you please add some human-friendly description on what benefits
the code above has? :)

The only change I see (besides code refactoring) is 30sec timeout (BTW
you can replace the list of numbers with "seq 30") instead of infinite
wait loop.  Which may be a good, but unrelated change.

As for re-creating functions dynamically, I find the above code more
complex than the existing one.  I think the current code is more
shell-style and easier to understand.  Just IMHO.

Regards,
  Dmitry

> > 
> > jamie.
> 
> Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Tomi Ollila]

On Thu, 17 Nov 2011 17:22:41 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Tomi.
> 
> On Thu, 17 Nov 2011 14:20:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > 
> > I.e. dynamically, at run-time, re-create test_emacs function...
> > 
> 
> Could you please add some human-friendly description on what benefits
> the code above has? :)

"All problems in computer science can be solved by another level of indirection"

> The only change I see (besides code refactoring) is 30sec timeout (BTW
> you can replace the list of numbers with "seq 30") instead of infinite
> wait loop.  Which may be a good, but unrelated change.

Can't do `seq 30` it is not portable. BSD world uses different command.

> As for re-creating functions dynamically, I find the above code more
> complex than the existing one.  I think the current code is more
> shell-style and easier to understand.  Just IMHO.

Think it as a function pointer (I should have renamed that as test_emacs_p ;)

Shell is hyper-dynamic being (like python). New functionality can be
written 'on-the-fly' inside functions (often without eval) Even variables 
can be referenced as function names. Hmm, even perl4 has this way of 
working supported...

IMHO it is simpler when one get's used to.

... just that the test "framework" needs some refactoring... along the
way all of these "practises" should be defined. code style, variable names
consistency, etc...  

You've done good work with this 'prereq' -thing. It's just hard to see
the elegance here. I know this very well as I have to maintain my
own 'evolutionary' developed code -- when priority is on short-term
cost savings over code consistency..

> 
> Regards,
>   Dmitry
> 
> > 
> > Tomi

Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Dmitry Kurochkin]

On Thu, 17 Nov 2011 16:02:46 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 17 Nov 2011 17:22:41 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi Tomi.
> > 
> > On Thu, 17 Nov 2011 14:20:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > > 
> > > I.e. dynamically, at run-time, re-create test_emacs function...
> > > 
> > 
> > Could you please add some human-friendly description on what benefits
> > the code above has? :)
> 
> "All problems in computer science can be solved by another level of indirection"
> 
> > The only change I see (besides code refactoring) is 30sec timeout (BTW
> > you can replace the list of numbers with "seq 30") instead of infinite
> > wait loop.  Which may be a good, but unrelated change.
> 
> Can't do `seq 30` it is not portable. BSD world uses different command.
> 
> > As for re-creating functions dynamically, I find the above code more
> > complex than the existing one.  I think the current code is more
> > shell-style and easier to understand.  Just IMHO.
> 
> Think it as a function pointer (I should have renamed that as test_emacs_p ;)
> 
> Shell is hyper-dynamic being (like python). New functionality can be
> written 'on-the-fly' inside functions (often without eval) Even variables 
> can be referenced as function names. Hmm, even perl4 has this way of 
> working supported...
> 
> IMHO it is simpler when one get's used to.
> 

I have no problem with thinking about it as a function pointer.  The
problem is that shell does not have function pointers :)

I am all for abstraction.  And I like lisp and haskell :) But it should
make things easier and more clear, not more complex.  IMO the old code
is easier to follow.  E.g. at any moment I understand what the function
would do, depending on the EMACS_SERVER value.  With functions being
dynamically overwritten, I have no idea what code would be executed when
the function is called.

Another point: EMACS_SERVER variable and the "function pointer" are kind
of duplicating each other.  And both have to be in sync which brings
possibility for new bugs.

If we follow this pattern than all code like:

  f() {
    if (!done_once)
        do_once

    do_more
  }

Should be rewritten using dynamic functions. I do not think I agree with
this :)

Anyway, all above is just IMHO.  You should probably go ahead and
prepare a patch implementing this approach for others to review.

> ... just that the test "framework" needs some refactoring... along the
> way all of these "practises" should be defined. code style, variable names
> consistency, etc...  
> 

Yeah, there are some rough edges.  But it is just a test suite.  I care
more about core notmuch and emacs UI :)

> You've done good work with this 'prereq' -thing.

Thanks.

> It's just hard to see
> the elegance here.

Come on, it is sooo obvious :)

Regards,
  Dmitry

> I know this very well as I have to maintain my
> own 'evolutionary' developed code -- when priority is on short-term
> cost savings over code consistency..
> 
> > 
> > Regards,
> >   Dmitry
> > 
> > > 
> > > Tomi
> 
> Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Tomi Ollila]

On Thu, 17 Nov 2011 19:17:16 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> 
> If we follow this pattern than all code like:
> 
>   f() {
>     if (!done_once)
>         do_once
> 
>     do_more
>   }
> 
> Should be rewritten using dynamic functions. I do not think I agree with
> this :)
> 
> Anyway, all above is just IMHO.  You should probably go ahead and
> prepare a patch implementing this approach for others to review.

I probably won't. While I was looking something in your patch and I was
thinking how to fix I just got this idea and wrote it to see whether
other's see as I do. The discussion got a bit side-tracked as I just
look this tiny part of the whole. Your later patch looks more
understandable than the previous (which emacs || emacs || return) and
it is something I can live with :) -- Just for now I'm not going to 
work on the whole anyway.

> 
> Regards,
>   Dmitry
> 

Thanks,
Tomi

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Thomas Jost]

[-- Attachment #1: Type: text/plain, Size: 2026 bytes --]

On Thu, 17 Nov 2011 05:56:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi all.
> 
> The following patch series is an attempt to introduce proper
> dependencies for external binaries in a less intrusive way than
> [1].  The primary aim was to avoid changing every subtest that
> uses external binaries.
> 
> There are still failing tests if a dependency is
> missing (e.g. "Verify that sent messages are
> saved/searchable (via FCC)" fails if there is no emacs).  It
> happens because such tests depend on others which are skipped.
> This issues are not addressed by this patch series.
> 
> If others do like the approach and it is pushed, I will work on
> updating tests that use the old style prerequisites (atomicity).
> 
> A careful review is needed!
> 
> Regards,
>   Dmitry
> 
> [1] id:"1321454035-22023-1-git-send-email-schnouki@schnouki.net"

Hi Dmitry,

This series looks quite good to me. It's a good approach, cleaner than
explicitely adding the prereqs to each test as in my previous patches
(and Pieter's).

Now, a few questions:

- same as Jamie: emacs_deliver_message hangs if dtach is not installed.
  In my patches I had to do this: "test_have_prereq EMACS &&
  emacs_deliver_message ...". Any idea how to handle this?

- what about indirect, "hidden" dependencies? The crypto test need to
  have a signed message delivered by emacs, so actually *all* the crypto
  tests depend on emacs. Right now, when dtach is not installed, the
  first test ("emacs delivery of signed message") is skipped and all the
  others fail. Would it be possible to handle this case without having
  to add explicit prereqs?

- right now functions like test_expect_success can be used as
  "test_expect_success COMMAND" or "test_expect_success PREREQ COMMAND".
  If we use your approach (and I hope we will!), do we need to keep that
  second syntax available in test-lib.sh too, or should we do some
  cleanup to get rid of it?

Thanks,

-- 
Thomas/Schnouki

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH 0/9] test: (hopefully) better test prerequisites

[Dmitry Kurochkin]

On Thu, 17 Nov 2011 10:46:58 +0100, Thomas Jost <schnouki@schnouki.net> wrote:
> On Thu, 17 Nov 2011 05:56:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > Hi all.
> > 
> > The following patch series is an attempt to introduce proper
> > dependencies for external binaries in a less intrusive way than
> > [1].  The primary aim was to avoid changing every subtest that
> > uses external binaries.
> > 
> > There are still failing tests if a dependency is
> > missing (e.g. "Verify that sent messages are
> > saved/searchable (via FCC)" fails if there is no emacs).  It
> > happens because such tests depend on others which are skipped.
> > This issues are not addressed by this patch series.
> > 
> > If others do like the approach and it is pushed, I will work on
> > updating tests that use the old style prerequisites (atomicity).
> > 
> > A careful review is needed!
> > 
> > Regards,
> >   Dmitry
> > 
> > [1] id:"1321454035-22023-1-git-send-email-schnouki@schnouki.net"
> 
> Hi Dmitry,
> 
> This series looks quite good to me. It's a good approach, cleaner than
> explicitely adding the prereqs to each test as in my previous patches
> (and Pieter's).
> 
> Now, a few questions:
> 
> - same as Jamie: emacs_deliver_message hangs if dtach is not installed.
>   In my patches I had to do this: "test_have_prereq EMACS &&
>   emacs_deliver_message ...". Any idea how to handle this?
> 

Answered to Jamie already.  That is a separate bug which is fixed in [1].

> - what about indirect, "hidden" dependencies? The crypto test need to
>   have a signed message delivered by emacs, so actually *all* the crypto
>   tests depend on emacs. Right now, when dtach is not installed, the
>   first test ("emacs delivery of signed message") is skipped and all the
>   others fail. Would it be possible to handle this case without having
>   to add explicit prereqs?
> 

Dependencies between the tests are not trivial (e.g. a test expects
notmuch to have a message delivered by the previous one).  I see two
solutions here:

* Remove such dependencies.  E.g. the code which sends a message may be
  put in a function.  Every test who needs it will call that function.
  No dependency on a previous test.

* Declare explicit prerequisites in some tests and use them in other
  tests.  IMO this is the only proper way to handle dependencies between
  tests.

We can (and probably should) use both approaches to resolving such
dependencies.

We can make it easier to implement the latter approach: add a function
like test_other_subtest_prereq which takes ID of subtests this one
depends on.  This way every subtest implicitly provides a prerequisite
later test can depend on.  Still we would have to explicitly check the
prerequisite in every test case that needs it.  I see no way to get rid
of it.

Note that inter-subtest dependencies is an existing issue.  Currently,
you can skip a single test.  Then all others that depend on it would
fail.

Also, we can move common stuff that is used by all subtests to the test
initialization (before the first subtest).  Then if it fails, all
subtests would be skipped.  Because of this all crypto tests would be
skipped if gpg is not installed.

> - right now functions like test_expect_success can be used as
>   "test_expect_success COMMAND" or "test_expect_success PREREQ COMMAND".
>   If we use your approach (and I hope we will!), do we need to keep that
>   second syntax available in test-lib.sh too, or should we do some
>   cleanup to get rid of it?
> 

At first, I wanted to clean it up.  But then I decided to leave it.  The
"old-style" prerequisites are more general.  So this patch series does
not fully replace it.  Besides there are tests that use it now
(atomicity at least), I have not updated them yet.

We may use general prerequisites for inter-subtest dependencies (though,
we probably can make it a little more convenient, see above).

I am not sure if we should remove general prerequisites even if they are
not used.  They work and they do not hurt.  I will left it for others to
decide.

Regards,
  Dmitry

[1] id:"yf639dnsqtc.fsf@taco2.nixu.fi"

> Thanks,
> 
> -- 
> Thomas/Schnouki