[PATCH 5/5] tests: Refactor weston launching syntax

[Pekka Paalanen]

On Thu, 2 Apr 2015 19:35:52 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Thu, Apr 02, 2015 at 02:13:36PM +0300, Pekka Paalanen wrote:
> > On Wed,  1 Apr 2015 19:17:07 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> > > There are only minor differences in the syntax for how weston
> > > is invoked for .so/.la tests vs. other tests, but it's hard
> > > to spot them.  Refactor the command itself out, so it becomes
> > > clearer what the difference is.
> > > 
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > > ---
> > >  tests/weston-tests-env | 35
> > > +++++++++++++++++------------------ 1 file changed, 17
> > > insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/tests/weston-tests-env b/tests/weston-tests-env
> > > index 3cb3073..173b6eb 100755
> > > --- a/tests/weston-tests-env
> > > +++ b/tests/weston-tests-env
> > > @@ -37,24 +37,23 @@ fi
> > >  
> > >  case $TEST_FILE in
> > >  	*.la|*.so)
> > > -		WESTON_BUILD_DIR=$abs_builddir \
> > > -		$WESTON --backend=$MODDIR/$BACKEND \
> > > -			$CONFIG \
> > > -			--shell=$SHELL_PLUGIN \
> > > -			--socket=test-${TEST_NAME} \
> > > -			--modules=$MODDIR/${TEST_FILE/.la/.so},$XWAYLAND_PLUGIN
> > > \
> > > -			--log="$SERVERLOG" \
> > > -			&> "$OUTLOG"
> > > +		TEST_MODULE=$MODDIR/${TEST_FILE/.la/.so}
> > > +		client=
> > >  		;;
> > >  	*)
> > > -		WESTON_BUILD_DIR=$abs_builddir \
> > > -
> > > WESTON_TEST_CLIENT_PATH=$abs_builddir/$TEST_FILE $WESTON \
> > > -			--socket=test-${TEST_NAME} \
> > > -			--backend=$MODDIR/$BACKEND \
> > > -			$CONFIG \
> > > -			--shell=$SHELL_PLUGIN \
> > > -			--log="$SERVERLOG" \
> > > -			--modules=$TEST_PLUGIN,$XWAYLAND_PLUGIN
> > > \
> > > -			$($abs_builddir/$TEST_FILE --params)
> > > \
> > > -			&> "$OUTLOG"
> > > +		export
> > > WESTON_TEST_CLIENT_PATH=$abs_builddir/$TEST_FILE
> > > +		TEST_MODULE=$TEST_PLUGIN
> > > +		client=$($WESTON_TEST_CLIENT_PATH --params)
> > >  esac
> > > +
> > > +export WESTON_BUILD_DIR=$abs_builddir
> > > +$WESTON \
> > > +	--shell=$SHELL_PLUGIN \
> > > +	--socket=test-${TEST_NAME} \
> > > +	--modules=$TEST_MODULE,$XWAYLAND_PLUGIN \
> > > +	--backend=$MODDIR/$BACKEND \
> > > +	--log="$SERVERLOG" \
> > > +	$CONFIG \
> > > +	$client \
> > > +	&> "$OUTLOG"
> > > +
> > 
> > I suppose this is fine in principle, I just have to adapt this
> > to it:
> > http://cgit.collabora.com/git/user/pq/weston.git/tree/tests/weston-tests-env?h=ivi-test-5
> > 
> > ivi-*.la needs to use --ivi-module instead of --modules to load
> > the test plugin.
> > 
> > ivi-*.la and ivi-*.weston need a special config file (all tests
> > use the same file), must not load xwayland.so, and need
> > ivi-shell.so as the shell.
> > 
> > It's going to be messy in any case, I'm afraid. Any good ideas?
> > 
> > 
> > Thanks,
> > pq
> 
> Following on from the patchset I just posted, actually the ivi
> customizations integrate fairly cleanly.
> 
> I didn't know if you want to also look in the src tree, but the
> plumbing is all there now if you do.

Hi,

where is that tree?

> From 76e6ab37fc0cea11e1fd16704dd66639f5673460 Mon Sep 17 00:00:00
> 2001 From: Bryce Harrington <bryce at osg.samsung.com>
> Date: Thu, 2 Apr 2015 19:30:22 -0700
> Subject: [PATCH] tests: Integrate overrides for ivi shell
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
>  tests/weston-tests-env | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/weston-tests-env b/tests/weston-tests-env
> index c0a211e..8055939 100755
> --- a/tests/weston-tests-env
> +++ b/tests/weston-tests-env
> @@ -21,15 +21,10 @@ BACKEND=${BACKEND:-headless-backend.so}
>  MODDIR=$abs_builddir/.libs
>  TEST_MODULE=$MODDIR/${TEST_FILE/.la/.so}
>  SHELL_PLUGIN=$MODDIR/desktop-shell.so
> +TEST_PLUGIN=$MODDIR/weston-test.so
>  XWAYLAND_PLUGIN=$MODDIR/xwayland.so
> -
> -if [[ ${TEST_MODULE} != *.so ]]; then
> -	export WESTON_TEST_CLIENT_PATH=$abs_builddir/$TEST_FILE
> -	client=$($WESTON_TEST_CLIENT_PATH --params)
> -	TEST_MODULE=$MODDIR/weston-test.so
> -fi
> -
>  CONFIG_FILE="${TEST_NAME}.ini"
> +
>  if [ -e "${abs_builddir}/${CONFIG_FILE}" ]; then
>  	CONFIG="--config=${abs_builddir}/${CONFIG_FILE}"
>  elif [ -e "${abs_top_srcdir}/tests/${CONFIG_FILE}" ]; then
> @@ -38,11 +33,23 @@ else
>  	CONFIG="--no-config"
>  fi
>  
> +if [[ ${TEST_MODULE} != *.so ]]; then
> +	export WESTON_TEST_CLIENT_PATH=$abs_builddir/$TEST_FILE
> +	client=$($WESTON_TEST_CLIENT_PATH --params)
> +	MODULES=$TEST_PLUGIN,$XWAYLAND_PLUGIN
> +fi
> +
> +if [[ ${TEST_MODULE} == ivi-* ]]; then
> +	SHELL_PLUGIN=$MODDIR/ivi-shell.so
> +	CONFIG="--config=${abs_builddir}/tests/weston-ivi.ini"
> +	MODULES=$TEST_PLUGIN
> +fi
> +
>  export WESTON_BUILD_DIR=$abs_builddir
>  $WESTON \
>  	--shell=$SHELL_PLUGIN \
>  	--socket=test-${TEST_NAME} \
> -	--modules=$TEST_MODULE,$XWAYLAND_PLUGIN \
> +	--modules=$MODULES \
>  	--backend=$MODDIR/$BACKEND \
>  	--log="$SERVERLOG" \
>  	$CONFIG \
> -- 
> 1.9.1
> 

Is this the diff needed to support the ivi tests? Not bad, but it's
not handling the ivi-*.so case properly AFAICS: --modules vs.
--ivi-module.

My point is, sure you can make this style work, but is it any more
clear than the big switch-case?

To me this looks more fragmented, while it does avoid copying
errors by reducing redundant lines. Set a variable, then add an
exception to set it to something else, then add another exception
on another condition... I think it becomes hard to follow what will
eventually be executed, because the reader needs to parse through
all conditionals to see what a single case does.

Isn't the point of this refactoring to improve readability? I'm not
completely sure it achieves that.

An opposite direction would be to use a different weston-tests-env
script for each type of test while collecting the common parts into
yet another script included by the others, but a) that's a bit too
far in the other direction, and b) I'm not sure we can drive that
from Makefile.am.


Thanks,
pq