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
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() {
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 ---. >