From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>,
	Jameson Graef Rollins <jrollins@finestructure.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH 0/9] test: (hopefully) better test prerequisites
In-Reply-To: <yf64ny3gcu4.fsf@taco2.nixu.fi>
References: <1321494986-18998-1-git-send-email-dmitry.kurochkin@gmail.com>
	<874ny36rhc.fsf@servo.finestructure.net>
	<yf64ny3gcu4.fsf@taco2.nixu.fi>
User-Agent: Notmuch/0.10_rc1+9~g8270095 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Thu, 17 Nov 2011 17:22:41 +0400
Message-ID: <8739dmopcu.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi Tomi.

On Thu, 17 Nov 2011 14:20:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Thu, 17 Nov 2011 01:14:07 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > 
> > However, when I tried to test the tests with the patch applied I ran
> > into one problem.  If I try to run the test suite with dtach
> > uninstalled, it looks like I'm experiencing a hang on
> > emacs_deliver_message call.  I wonder if either emacs_deliver_message or
> > test_emacs is not doing the right thing in the case of no dtach.  I
> > think emacs_deliver_message should somehow not require dtach, since it's
> > not actually testing any display stuff, but because it's currently using
> > test_emacs, it is somehow implicitly depending on it.  Any thoughts on
> > how to fix that?
> 
> Quick thought
> 
> mk_skip_test_emacs () 
> { test_emacs () 
>   { 
> 	echo SKIPPED
>         false
>   }
>   test_emacs
> }
> 
> mk_run_test_emacs () 
> { test_emacs () 
>   {
> 	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
>   }
>   test_emacs
> }
> 
> test_emacs ()
> {
>         which dtach >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>         which emacs >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
>         which emacsclient >/dev/null 2>&1 || { mk_skip_test_emacs; return; }
> 
>         EMACS_SERVER="notmuch-test-suite-$$"
> 	# start a detached session with an emacs server
> 	# user's TERM is given to dtach which assumes a minimally
> 	# VT100-compatible terminal -- and emacs inherits that
> 	TERM=$ORIGINAL_TERM dtach -n "$TMP_DIRECTORY/emacs-dtach-socket.$$" \
> 		sh -c "stty rows 24 cols 80; exec '$TMP_DIRECTORY/run_emacs' \
> 			--no-window-system \
> 			--eval '(setq server-name \"$EMACS_SERVER\")' \
> 			--eval '(server-start)' \
> 			--eval '(orphan-watchdog $$)'" ||
>                         { mk_skip_test_emacs; return; }
> 	# wait until the emacs server is up
>         for _ in 1 2 3 4 5 6 7 8 9 10 1 2 3 4 5 6 7 8 9 20 1 2 3 4 5 6 7 8 9 30
>         do 
>         	if test_emacs '()' 2>/dev/null
>                 then
>                         mk_run_test_emacs
>                         return
>                 fi
>         done
>         mk_skip_test_emacs
> }
> 
> I.e. dynamically, at run-time, re-create test_emacs function...
> 

Could you please add some human-friendly description on what benefits
the code above has? :)

The only change I see (besides code refactoring) is 30sec timeout (BTW
you can replace the list of numbers with "seq 30") instead of infinite
wait loop.  Which may be a good, but unrelated change.

As for re-creating functions dynamically, I find the above code more
complex than the existing one.  I think the current code is more
shell-style and easier to understand.  Just IMHO.

Regards,
  Dmitry

> > 
> > jamie.
> 
> Tomi