test_emacs_expect_t does ignore Emacs as prerequisite

classic Classic list List threaded Threaded
3 messages Options
Milton Vandersloot Milton Vandersloot
Reply | Threaded
Open this post in threaded view
|

test_emacs_expect_t does ignore Emacs as prerequisite

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


_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: test_emacs_expect_t does ignore Emacs as prerequisite

Milton Vandersloot <[hidden email]> 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() {

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

Re: test_emacs_expect_t does ignore Emacs as prerequisite

On Fri, Apr 24 2020, David Bremner wrote:

> Milton Vandersloot <[hidden email]> 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 ---.
>
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch