On Mon, 27 Jun 2011 16:02:12 -0400, Austin Clements <amdragon@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@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@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@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@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@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@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@mail.gmail.com") > > - (notmuch-show-save-attachments)' > /dev/null 2>&1 > > +test_emacs '(let ((standard-input "\"attachment1.gz\"")) > > + (notmuch-show "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@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@mail.gmail.com" 5)' > /dev/null 2>&1 > > +test_emacs '(let ((standard-input "\"attachment2.gz\"")) > > + (notmuch-show-save-part "id:cf0c4d610911171136h1713aa59w9cf9aa31f052ad0a@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@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@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@notmuchmail.org > > other_email=test_suite_other@notmuchmail.org;test_suite@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@notmuchmail.org > > http://notmuchmail.org/mailman/listinfo/notmuch > >