test_emacs_expect_t does ignore Emacs as prerequisite

test_emacs_expect_t does ignore Emacs as prerequisite

[Milton Vandersloot]

Dear notmuch Developers

test_emacs_expect_t ignores that it needs Emacs as a prerequisite.
It seems (by comparing the logic of this function with the logic of other test_* functions, e.g. test_expect_success) that the test for that was introduced later and forgotten in this method.
There might also be more places/other test_* methods which miss this check but I have not checked that as I'm not familiar with the codebase.
Below is a patch for the test_emacs_expect_t function.

Regards
Milton

[PATCH] Let test_emacs_expect_t respect missing external prerequisites

test_emacs_expect_t did not test for missing prerequisites (even though
it called test_emacs which does it). Fix that by testing for missing
prerequisites.

--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -548,6 +548,8 @@ test_emacs_expect_t () {
 		inside_subtest=

 		# Report success/failure.
+		test_check_missing_external_prereqs_ "$test_subtest_name" ||
+		{
 		result=$(cat OUTPUT)
 		if [ "$result" = t ]
 		then
@@ -555,6 +557,7 @@ test_emacs_expect_t () {
 		else
 			test_failure_ "${result}"
 		fi
+		}
 	else
 		# Restore state after the (non) test.
 		exec 1>&6 2>&7		# Restore stdout and stderr

Re: test_emacs_expect_t does ignore Emacs as prerequisite

[David Bremner]

Milton Vandersloot <miltonrobertvandersloot1412@protonmail.com> writes:
>
> [PATCH] Let test_emacs_expect_t respect missing external prerequisites
>
> test_emacs_expect_t did not test for missing prerequisites (even though
> it called test_emacs which does it). Fix that by testing for missing
> prerequisites.
>

I agree there's a bug here, but I'm not sure this is the best/cleanest
fix. Maybe Tomi (in Cc) can comment. The logic for prerequisite checking
is already opaque. For example test_skip is already calling
test_check_missing_external_prereqs_ as a side effect. For starters I
wonder if test_emacs should use a return value to indicate failure,
along the lines of the patch at the end of the message.

BTW, it will make our life easier if you follow
https://notmuchmail.org/contributing/#index5h2; in particular using
git-send-email and keeping the discussion/notes after ---.


diff --git a/test/test-lib.sh b/test/test-lib.sh
index 7f8a3a4d..eecc52f7 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -543,9 +543,8 @@ test_emacs_expect_t () {
 	# Run the test.
 	if ! test_skip "$test_subtest_name"
 	then
-		test_emacs "(notmuch-test-run $1)" >/dev/null
-
-		# Restore state after the test.
+		test_emacs "(notmuch-test-run $1)" || return
+                # Restore state after the test.
 		exec 1>&6 2>&7		# Restore stdout and stderr
 		inside_subtest=
 
@@ -997,7 +996,7 @@ test_emacs () {
 	test_require_external_prereq dtach || missing_dependencies=1
 	test_require_external_prereq emacs || missing_dependencies=1
 	test_require_external_prereq ${TEST_EMACSCLIENT} || missing_dependencies=1
-	test -z "$missing_dependencies" || return
+	test -z "$missing_dependencies" || return 1
 
 	if [ -z "$EMACS_SERVER" ]; then
 		emacs_tests="$NOTMUCH_SRCDIR/test/${this_test_bare}.el"
@@ -1034,6 +1033,7 @@ test_emacs () {
 	touch OUTPUT
 
 	${TEST_EMACSCLIENT} --socket-name="$EMACS_SERVER" --eval "(notmuch-test-progn $*)"
+        return 0
 }
 
 test_python() {

Re: test_emacs_expect_t does ignore Emacs as prerequisite

[Tomi Ollila]

On Fri, Apr 24 2020, David Bremner wrote:

> Milton Vandersloot <miltonrobertvandersloot1412@protonmail.com> writes:
>>
>> [PATCH] Let test_emacs_expect_t respect missing external prerequisites
>>
>> test_emacs_expect_t did not test for missing prerequisites (even though
>> it called test_emacs which does it). Fix that by testing for missing
>> prerequisites.
>>
>
> I agree there's a bug here, but I'm not sure this is the best/cleanest
> fix. Maybe Tomi (in Cc) can comment. The logic for prerequisite checking
> is already opaque. For example test_skip is already calling
> test_check_missing_external_prereqs_ as a side effect. For starters I
> wonder if test_emacs should use a return value to indicate failure,
> along the lines of the patch at the end of the message.

I'd like David's approach, but in that case we don't get the 
"missing prerequisities" messages. Milton's solution looks like
something that works =D. 

Just that the content inside {} needs to be indented, and opening
brace ({) should be after || in same line...

In case of test_skip it doesn't know about missing emacs prerequisities
as the "subtest prerequisities" infomation is cleaned before every
test and the information is regained in test_emacs...

Tomi

> BTW, it will make our life easier if you follow
> https://notmuchmail.org/contributing/#index5h2; in particular using
> git-send-email and keeping the discussion/notes after ---.
>