On Mon, 27 Jun 2011 16:02:12 -0400, Austin Clements <amdragon at mit.edu> wrote:
> This looks like a great idea! The test suite has been getting irritating
> slow.
>
> A few minor comments: This patch would be clearer if it the
> setq-to-let translation were a separate patch. It would also be worth
> adding a big comment at the top of the test explaining why all of the
> tests let-bind everything instead of setq'ing, primarily for the
> benefit of people writing new tests.
>
Agreed, will separate and add the warning.
> I might just be having trouble reading the patch, but the difference
> between emacs_start and emacs_server_start seems unclear. Perhaps the
> comments should explain how somebody would use these scripts?
>
emacs_start start a normal Emacs in non-daemon mode. Something you
might prefer when debugging.
>
> My bigger concern with this change is that it may leave behind stale
> emacs daemons if the script gets interrupted.
That is an issue indeed. I would probably do smth like a periodic check
inside Emacs that the shell is alive, but your approach below looks more
reliable.
> The only way I know to
> reliably kill a child process is to open a pipe to it and have it exit
> on its own when it reads EOF. Unfortunately, I couldn't find a way to
> do this with an emacs daemon (it appears daemon mode aggressively
> cleans up things like pipes), but here's a different approach:
>
> coproc emacs --batch --eval "(while t (eval (read)))"
> EMACSFD=${COPROC[1]}
> trap "echo '(kill-emacs)' >&$EMACSFD" EXIT
>
> echo '(message "Hi")' >&$EMACSFD
> # ...
>
> This is, basically, a poor man's emacs server, but the coprocess pipe
> binds it tightly to the shell. If the shell exits for *any* reason,
> the pipe will be closed by the kernel, emacs will read an EOF, and
> exit.
I like this idea.
> The trap is there just to cleanly shut down in case of a normal
> exit [1].
For normal exit we should just put this into test_done. Otherwise it is
not a normal exit and we do not care about Emacs error message. No?
> This also has the advantage that read-from-minibuffer still
> works:
>
> echo '(message (read-from-minibuffer ""))' >&$EMACSFD
> echo 'Test' >&$EMACSFD
>
> Thoughts?
>
I like it and I will implement it. Thanks for the idea.
Regards,
Dmitry
> [1] If you don't do this, emacs complains that it can't read from
> stdin before it exits. It would be nice to catch this condition in
> the elisp code and not bother with the trap, but the error thrown is
> just an 'error, so I don't think we can catch and ignore it without
> catching and ignoring *all* errors.
>
> On Sun, Jun 26, 2011 at 11:54 PM, Dmitry Kurochkin
> <dmitry.kurochkin at gmail.com> wrote:
> > Before the change, every Emacs tests ran in a separate Emacs
> > instance. ?Starting Emacs many times wastes considerable time and
> > it gets worse as the test suite grows. ?The patch solves this by
> > using a single Emacs server and emacsclient(1) to run multiple
> > tests. ?Emacs server is started on the first test_emacs call and
> > stopped when test_done is called or the test is killed by a
> > signal. ?Several auxiliary scripts useful for debugging and test
> > development are generated instead of the run_emacs script:
> >
> > ?* emacs_server_start - start Emacs server
> > ?* emacs_server_stop ?- stop Emacs server
> > ?* emacs_start ? ? ? ?- start Emacs
> > ?* emacs_run ? ? ? ? ?- execute ELisp expressions in running Emacs server
> >
> > Since multiple tests are run in a single Emacs instance, they
> > must not change Emacs environment because it may affect other
> > tests. ?For now, the only Emacs environment modifications done by
> > the tests are variable settings. ?Before the change, variables
> > were set with `setq' which affected other tests. ?The patch
> > changes all variables to use `let', so the scope of the change is
> > limited to a single test.
> > ---
> > ?test/emacs ? ? ? | ? 74 +++++++++++++-------------
> > ?test/test-lib.el | ? ?6 ++
> > ?test/test-lib.sh | ?149
> > ++++++++++++++++++++++++++++++++++++++++++-----------
> > ?3 files changed, 161 insertions(+), 68 deletions(-)
> >
> > diff --git a/test/emacs b/test/emacs
> > index 4f16b41..f1939dc 100755
> > --- a/test/emacs
> > +++ b/test/emacs
> > @@ -12,20 +12,20 @@ test_emacs '(notmuch-hello)
> > ?test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello
> >
> > ?test_begin_subtest "Saved search with 0 results"
> > -test_emacs '(setq notmuch-show-empty-saved-searches t)
> > - ? ? ? ? ? (setq notmuch-saved-searches
> > - ? ? ? ? ? ? ? ? '\''(("inbox" . "tag:inbox")
> > - ? ? ? ? ? ? ? ? ? ? ?("unread" . "tag:unread")
> > - ? ? ? ? ? ? ? ? ? ? ?("empty" . "tag:doesnotexist")))
> > - ? ? ? ? ? (notmuch-hello)
> > - ? ? ? ? ? (test-output)'
> > +test_emacs '(let ((notmuch-show-empty-saved-searches t)
> > + ? ? ? ? ? ? ? ? (notmuch-saved-searches
> > + ? ? ? ? ? ? ? ? ?'\''(("inbox" . "tag:inbox")
> > + ? ? ? ? ? ? ? ? ? ? ? ("unread" . "tag:unread")
> > + ? ? ? ? ? ? ? ? ? ? ? ("empty" . "tag:doesnotexist"))))
> > + ? ? ? ? ? ? (notmuch-hello)
> > + ? ? ? ? ? ? (test-output))'
> > ?test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello-with-empty
> >
> > ?test_begin_subtest "No saved searches displayed (all with 0 results)"
> > -test_emacs '(setq notmuch-saved-searches
> > - ? ? ? ? ? ? ? ? '\''(("empty" . "tag:doesnotexist")))
> > - ? ? ? ? ? (notmuch-hello)
> > - ? ? ? ? ? (test-output)'
> > +test_emacs '(let ((notmuch-saved-searches
> > + ? ? ? ? ? ? ? ? ?'\''(("empty" . "tag:doesnotexist"))))
> > + ? ? ? ? ? ? (notmuch-hello)
> > + ? ? ? ? ? ? (test-output))'
> > ?test_expect_equal_file OUTPUT $EXPECTED/notmuch-hello-no-saved-searches
> >
> > ?test_begin_subtest "Basic notmuch-search view in emacs"
> > @@ -147,9 +147,9 @@ output=$(notmuch search 'subject:"testing message sent
> > via SMTP"' | notmuch_sear
> > ?test_expect_equal "$output" "thread:XXX ? 2000-01-01 [1/1] Notmuch Test
> > Suite; Testing message sent via SMTP (inbox)"
> >
> > ?test_begin_subtest "notmuch-fcc-dirs set to nil"
> > -test_emacs "(setq notmuch-fcc-dirs nil)
> > - ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? (test-output)"
> > +test_emacs "(let ((notmuch-fcc-dirs nil))
> > + ? ? ? ? ? ? (notmuch-mua-mail)
> > + ? ? ? ? ? ? (test-output))"
> > ?cat <<EOF >EXPECTED
> > ?From: Notmuch Test Suite <test_suite at notmuchmail.org>
> > ?To:
> > @@ -164,9 +164,9 @@ mkdir -p mail/sent-string/new
> > ?mkdir -p mail/sent-string/tmp
> >
> > ?test_begin_subtest "notmuch-fcc-dirs set to a string"
> > -test_emacs "(setq notmuch-fcc-dirs \"sent-string\")
> > - ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? (test-output)"
> > +test_emacs "(let ((notmuch-fcc-dirs \"sent-string\"))
> > + ? ? ? ? ? ? (notmuch-mua-mail)
> > + ? ? ? ? ? ? (test-output))"
> > ?cat <<EOF >EXPECTED
> > ?From: Notmuch Test Suite <test_suite at notmuchmail.org>
> > ?To:
> > @@ -185,11 +185,11 @@ mkdir -p mail/failure/new
> > ?mkdir -p mail/failure/tmp
> >
> > ?test_begin_subtest "notmuch-fcc-dirs set to a list (with match)"
> > -test_emacs "(setq notmuch-fcc-dirs
> > - ? ? ? ? ? ? ? ? '((\"notmuchmail.org\" . \"sent-list-match\")
> > - ? ? ? ? ? ? ? ? ? (\".*\" . \"failure\")))
> > - ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? (test-output)"
> > +test_emacs "(let ((notmuch-fcc-dirs
> > + ? ? ? ? ? ? ? ? ?'((\"notmuchmail.org\" . \"sent-list-match\")
> > + ? ? ? ? ? ? ? ? ? ?(\".*\" . \"failure\"))))
> > + ? ? ? ? ? ? (notmuch-mua-mail)
> > + ? ? ? ? ? ? (test-output))"
> > ?cat <<EOF >EXPECTED
> > ?From: Notmuch Test Suite <test_suite at notmuchmail.org>
> > ?To:
> > @@ -205,11 +205,11 @@ mkdir -p mail/sent-list-catch-all/new
> > ?mkdir -p mail/sent-list-catch-all/tmp
> >
> > ?test_begin_subtest "notmuch-fcc-dirs set to a list (catch-all)"
> > -test_emacs "(setq notmuch-fcc-dirs
> > - ? ? ? ? ? ? ? ? '((\"example.com\" . \"failure\")
> > - ? ? ? ? ? ? ? ? ? (\".*\" . \"sent-list-catch-all\")))
> > - ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? (test-output)"
> > +test_emacs "(let ((notmuch-fcc-dirs
> > + ? ? ? ? ? ? ? ? ?'((\"example.com\" . \"failure\")
> > + ? ? ? ? ? ? ? ? ? ?(\".*\" . \"sent-list-catch-all\"))))
> > + ? ? ? ? ? ? (notmuch-mua-mail)
> > + ? ? ? ? ? ? (test-output))"
> > ?cat <<EOF >EXPECTED
> > ?From: Notmuch Test Suite <test_suite at notmuchmail.org>
> > ?To:
> > @@ -220,11 +220,11 @@ EOF
> > ?test_expect_equal_file OUTPUT EXPECTED
> >
> > ?test_begin_subtest "notmuch-fcc-dirs set to a list (no match)"
> > -test_emacs "(setq notmuch-fcc-dirs
> > - ? ? ? ? ? ? ? ? '((\"example.com\" . \"failure\")
> > - ? ? ? ? ? ? ? ? ? (\"nomatchhere.net\" . \"failure\")))
> > - ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? (test-output)"
> > +test_emacs "(let ((notmuch-fcc-dirs
> > + ? ? ? ? ? ? ? ? ?'((\"example.com\" . \"failure\")
> > + ? ? ? ? ? ? ? ? ? ?(\"nomatchhere.net\" . \"failure\"))))
> > + ? ? ? ? ? ? (notmuch-mua-mail)
> > + ? ? ? ? ? ? (test-output))"
> > ?cat <<EOF >EXPECTED
> > ?From: Notmuch Test Suite <test_suite at notmuchmail.org>
> > ?To:
> > @@ -253,15 +253,15 @@ test_expect_equal_file OUTPUT EXPECTED
> >
> > ?test_begin_subtest "Save attachment from within emacs using
> > notmuch-show-save-attachments"
> > ?# save as archive to test that Emacs does not re-compress .gz
> > -echo ./attachment1.gz |
> > -test_emacs '(notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a
> > at mail.gmail.com")
> > - ? ? ? ? ? (notmuch-show-save-attachments)' > /dev/null 2>&1
> > +test_emacs '(let ((standard-input "\"attachment1.gz\""))
> > + ? ? ? ? ? ? (notmuch-show
> > "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a at mail.gmail.com")
> > + ? ? ? ? ? ? (notmuch-show-save-attachments))'
> > ?test_expect_equal_file "$EXPECTED/attachment" attachment1.gz
> >
> > ?test_begin_subtest "Save attachment from within emacs using
> > notmuch-show-save-part"
> > ?# save as archive to test that Emacs does not re-compress .gz
> > -echo ./attachment2.gz |
> > -test_emacs '(notmuch-show-save-part
> > "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a at mail.gmail.com" 5)' >
> > /dev/null 2>&1
> > +test_emacs '(let ((standard-input "\"attachment2.gz\""))
> > + ? ? ? ? ? ? (notmuch-show-save-part
> > "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a at mail.gmail.com" 5))' >
> > /dev/null 2>&1
> > ?test_expect_equal_file "$EXPECTED/attachment" attachment2.gz
> >
> > ?test_begin_subtest "View raw message within emacs"
> > diff --git a/test/test-lib.el b/test/test-lib.el
> > index 4e7f5cf..a5a3125 100644
> > --- a/test/test-lib.el
> > +++ b/test/test-lib.el
> > @@ -23,6 +23,12 @@
> > ?;; avoid crazy 10-column default of --batch
> > ?(set-frame-width (window-frame (get-buffer-window)) 80)
> >
> > +;; `read-file-name' by default uses `completing-read' function to read
> > +;; user input. ?It does not respect `standard-input' variable which we
> > +;; use in tests to provide user input. ?So replace it with a plain
> > +;; `read' call.
> > +(setq read-file-name-function (lambda (&rest _) (read)))
> > +
> > ?(defun notmuch-test-wait ()
> > ? "Wait for process completion."
> > ? (while (get-buffer-process (current-buffer))
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index ad1506c..1c1581b 100755
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -57,6 +57,9 @@ unset CDPATH
> >
> > ?unset GREP_OPTIONS
> >
> > +# PID of running Emacs server
> > +emacs_server_pid=
> > +
> > ?# Convenience
> > ?#
> > ?# A regexp to match 5 and 40 hexdigits
> > @@ -174,6 +177,7 @@ test_success=0
> >
> > ?die () {
> > ? ? ? ?code=$?
> > + ? ? ? emacs_server_stop
> > ? ? ? ?if test -n "$GIT_EXIT_OK"
> > ? ? ? ?then
> > ? ? ? ? ? ? ? ?exit $code
> > @@ -394,19 +398,20 @@ emacs_deliver_message ()
> > ? ? mkdir -p "$MAIL_DIR"/sent/{cur,new,tmp}
> > ? ? ../smtp-dummy sent_message &
> > ? ? smtp_dummy_pid=$!
> > - ? ?test_emacs "(setq message-send-mail-function 'message-smtpmail-send-it)
> > - ? ? ? ? ? ? ? (setq smtpmail-smtp-server \"localhost\")
> > - ? ? ? ? ? ? ? (setq smtpmail-smtp-service \"25025\")
> > - ? ? ? ? ? ? ? (notmuch-hello)
> > - ? ? ? ? ? ? ? (notmuch-mua-mail)
> > - ? ? ? ? ? ? ? (message-goto-to)
> > - ? ? ? ? ? ? ? (insert \"test_suite at notmuchmail.org\nDate: 01 Jan 2000
> > 12:00:00 -0000\")
> > - ? ? ? ? ? ? ? (message-goto-subject)
> > - ? ? ? ? ? ? ? (insert \"${subject}\")
> > - ? ? ? ? ? ? ? (message-goto-body)
> > - ? ? ? ? ? ? ? (insert \"${body}\")
> > - ? ? ? ? ? ? ? $@
> > - ? ? ? ? ? ? ? (message-send-and-exit)" >/dev/null 2>&1
> > + ? ?test_emacs \
> > + ? ? ? "(let ((message-send-mail-function 'message-smtpmail-send-it)
> > + ? ? ? ? ? ? ?(smtpmail-smtp-server \"localhost\")
> > + ? ? ? ? ? ? ?(smtpmail-smtp-service \"25025\"))
> > + ? ? ? ? ?(notmuch-hello)
> > + ? ? ? ? ?(notmuch-mua-mail)
> > + ? ? ? ? ?(message-goto-to)
> > + ? ? ? ? ?(insert \"test_suite at notmuchmail.org\nDate: 01 Jan 2000
> > 12:00:00 -0000\")
> > + ? ? ? ? ?(message-goto-subject)
> > + ? ? ? ? ?(insert \"${subject}\")
> > + ? ? ? ? ?(message-goto-body)
> > + ? ? ? ? ?(insert \"${body}\")
> > + ? ? ? ? ?$@
> > + ? ? ? ? ?(message-send-and-exit))" >/dev/null 2>&1
> > ? ? wait ${smtp_dummy_pid}
> > ? ? notmuch new >/dev/null
> > ?}
> > @@ -828,6 +833,8 @@ test_done () {
> >
> > ? ? ? ?echo
> >
> > + ? ? ? emacs_server_stop
> > +
> > ? ? ? ?if [ "$test_failure" = "0" ]; then
> > ? ? ? ? ? ?if [ "$test_broken" = "0" ]; then
> > ? ? ? ? ? ? ? ?rm -rf "$remove_tmp"
> > @@ -838,24 +845,26 @@ test_done () {
> > ? ? ? ?fi
> > ?}
> >
> > -test_emacs () {
> > - ? ? ? # Construct a little test script here for the benefit of the user,
> > - ? ? ? # (who can easily run "run_emacs" to get the same emacs environment
> > - ? ? ? # for investigating any failures).
> > - ? ? ? cat <<EOF > run_emacs
> > +# Generate some scripts for running Emacs tests. ?These scripts are
> > +# used by Emacs tests and help investigating failures. ?The following
> > +# scripts are generated:
> > +#
> > +# * emacs_server_start - start Emacs server
> > +# * emacs_server_stop ?- stop Emacs server
> > +# * emacs_start ? ? ? ? ? ? ? ?- start Emacs
> > +# * emacs_run ? ? ? ? ?- execute ELisp expressions in running Emacs server
> > +emacs_generate_scripts ()
> > +{
> > + ? ? ? server_name="notmuch-test-suite-$$"
> > +
> > + ? ? ? cat <<EOF > "$TMP_DIRECTORY/emacs_server_start"
> > ?#!/bin/sh
> > ?export PATH=$PATH
> > ?export NOTMUCH_CONFIG=$NOTMUCH_CONFIG
> >
> > -# We assume that the user will give a command-line argument only if
> > -# wanting to run in batch mode.
> > -if [ \$# -gt 0 ]; then
> > - ? ? ? BATCH=--batch
> > -fi
> > -
> > ?# Here's what we are using here:
> > ?#
> > -# --batch: ? ? ? ? ? ? Quit after given commands and print all (messages)
> > +# --daemon ? ? ? ? ? ? Start Emacs as a daemon
> > ?#
> > ?# --no-init-file ? ? ? Don't load users ~/.emacs
> > ?#
> > @@ -865,13 +874,90 @@ fi
> > ?#
> > ?# --load ? ? ? ? ? ? ? Force loading of notmuch.el and test-lib.el
> >
> > -emacs \$BATCH --no-init-file --no-site-file \
> > - ? ? ? --directory ../../emacs --load notmuch.el \
> > - ? ? ? --directory .. --load test-lib.el \
> > - ? ? ? --eval "(progn \$@)"
> > +emacs --daemon --no-init-file --no-site-file \
> > + ? ? ? --directory "$TEST_DIRECTORY/../emacs" --load notmuch.el \
> > + ? ? ? --directory "$TEST_DIRECTORY" --load test-lib.el \
> > + ? ? ? --eval '(setq server-name "$server_name")'
> > +EOF
> > + ? ? ? chmod a+x "$TMP_DIRECTORY/emacs_server_start"
> > +
> > + ? ? ? cat <<EOF > "$TMP_DIRECTORY/emacs_server_stop"
> > +#!/bin/sh
> > +
> > +dir=\$(dirname "\$0")
> > +"\$dir"/emacs_run '(kill-emacs)'
> > ?EOF
> > - ? ? ? chmod a+x ./run_emacs
> > - ? ? ? ./run_emacs "$@"
> > + ? ? ? chmod a+x "$TMP_DIRECTORY/emacs_server_stop"
> > +
> > + ? ? ? cat <<EOF > "$TMP_DIRECTORY/emacs_start"
> > +#!/bin/sh
> > +export PATH=$PATH
> > +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
> > +
> > +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/emacs_start"
> > +
> > + ? ? ? cat <<EOF > "$TMP_DIRECTORY/emacs_run"
> > +#!/bin/sh
> > +
> > +# Here's what we are using here:
> > +#
> > +# --socket-name ? ? ? ? ? ? ? ?Emacs server name
> > +#
> > +# --eval ? ? ? ? ? ? ? Evaluate ELisp expressions
> > +
> > +emacsclient --socket-name "$server_name" --eval "(progn \$@)"
> > +EOF
> > + ? ? ? chmod a+x "$TMP_DIRECTORY/emacs_run"
> > +}
> > +
> > +# Start Emacs server if it is not running.
> > +emacs_server_start ()
> > +{
> > + ? ? ? [ -n "$emacs_server_pid" ] && return
> > +
> > + ? ? ? output=$("$TMP_DIRECTORY/emacs_server_start" 2>&1)
> > + ? ? ? if [ "$?" -ne 0 ]; then
> > + ? ? ? ? ? ? ? echo "$output"
> > + ? ? ? ? ? ? ? return 1
> > + ? ? ? fi
> > +
> > + ? ? ? emacs_server_pid=$("$TMP_DIRECTORY/emacs_run" '(emacs-pid)')
> > + ? ? ? [ "$?" -eq 0 -a -n "$emacs_server_pid" ]
> > +}
> > +
> > +# Stop Emacs server if it is running.
> > +emacs_server_stop ()
> > +{
> > + ? ? ? [ -z "$emacs_server_pid" ] && return
> > +
> > + ? ? ? emacs_server_pid=
> > + ? ? ? output=$("$TMP_DIRECTORY/emacs_server_stop" 2>&1)
> > + ? ? ? if [ "$?" -ne 0 ]; then
> > + ? ? ? ? ? ? ? echo "$output"
> > + ? ? ? ? ? ? ? return 1
> > + ? ? ? fi
> > +}
> > +
> > +# Evaluate ELisp expressions in Emacs server. ?Server is started if it
> > +# is not running.
> > +test_emacs () {
> > + ? ? ? emacs_server_start || return
> > +
> > + ? ? ? "$TMP_DIRECTORY/emacs_run" "$@"
> > ?}
> >
> >
> > @@ -999,6 +1085,7 @@ primary_email=test_suite at notmuchmail.org
> > ?other_email=test_suite_other at notmuchmail.org;test_suite at
> > otherdomain.org
> > ?EOF
> >
> > +emacs_generate_scripts
> >
> > ?# Use -P to resolve symlinks in our working directory so that the cwd
> > ?# in subprocesses like git equals our $PWD (for pathname comparisons).
> > --
> > 1.7.5.4
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
> >
