[PATCH weston v2 0/9] Enable test configuration

[Pekka Paalanen]

On Tue, 7 Apr 2015 15:32:51 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Tue, Apr 07, 2015 at 02:44:59PM +0300, Pekka Paalanen wrote:
> > On Thu,  2 Apr 2015 19:16:49 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> > > This enables tests to provide their own .ini files for configuring
> > > weston, via the recently added --config option.  Also included in this
> > > set is some related cleanup in weston-tests-env.
> > > 
> > > [Update v2]
> > > Implemented review comments from pekka:
> > >  * Dropped ~/.config as default location of weston.ini in help text.
> > >  * Look for generated .ini files in build dir, as well as static ini
> > >    files in the source tree (in tests/ subdir)
> > >  * Simplified the .la/.so logic.
> > >  * Added a test case for config loading
> > > Also did some further fussy cleanup of weston-tests-env.

> > Now, previously I was concerned that these clean-ups (patches 2, 5, 6)
> > would actually make weston-tests-env harder to read. I still wasn't
> > sure, so I tried it and rebased my ivi-test-5 branch on top your
> > complete series:
> > http://cgit.collabora.com/git/user/pq/weston.git/log/?h=ivi-test-5b
> > 
> > After my rebase, weston-tests-env now looks like this:
> > --------------------------------------------------
> > 
> > #!/bin/bash
> > 
> > TEST_FILE=${1##*/}
> > TEST_NAME=${TEST_FILE%.*}
> > 
> > if [ -z "$TEST_NAME" ]; then
> > 	echo "usage: $(basename $0) <test name>"
> > 	exit 1;
> > fi
> > 
> > WESTON=$abs_builddir/weston
> > LOGDIR=$abs_builddir/logs
> > mkdir -p "$LOGDIR" || exit
> > 
> > OUTLOG="$LOGDIR/${TEST_NAME}-log.txt"
> > SERVERLOG="$LOGDIR/${TEST_NAME}-serverlog.txt"
> > rm -f "$SERVERLOG" || exit
> > 
> > client=
> > BACKEND=${BACKEND:-headless-backend.so}
> > MODDIR=$abs_builddir/.libs
> > TEST_MODULE=$MODDIR/${TEST_FILE/.la/.so}
> > SHELL_PLUGIN=$MODDIR/desktop-shell.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
> > 	CONFIG="--config=${abs_top_srcdir}/tests/${CONFIG_FILE}"
> > else
> > 	CONFIG="--no-config"
> > fi
> > 
> > if [[ ${TEST_NAME} = ivi-* ]]; then
> > 	SHELL_PLUGIN=$MODDIR/ivi-shell.so
> > 	if [[ "$CONFIG" = "--no-config" ]]; then
> > 		CONFIG="--config=$abs_builddir/tests/weston-ivi.ini"
> > 	fi
> > 	XWAYLAND_PLUGIN=""
> > fi
> > 
> > if [[ ${TEST_FILE} = ivi-*.la ]]; then
> > 	client="$client --ivi-module=$TEST_MODULE"
> > 	TEST_MODULE=$MODDIR/weston-test.so
> > fi
> > 
> > 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"
> > 
> > --------------------------------------------------
> > 
> > Is this what you meant?
> 
> Yes, that looks good.

Hi,

sorry, I find it very confusing to read. It took me half an hour
to figure out what I need to add to add my ivi-shell test cases. With
the old way
http://cgit.collabora.com/git/user/pq/weston.git/tree/tests/weston-tests-env?h=ivi-test-5
it was as simple as copying the block and changing what I needed.

When you read the above, is it clear that there are fundamentally four
different types of test setups? What are the runtime parameters to each
of them? I really can't see what they are without running that code
either mentally or for real, because there are so many combinations of
if-clauses that overwrite previously assigned variables. IOW, I find it
hard to read.

If I want to change the parameters to one test case, I cannot just
change one place. I have to again parse through every possible
combination of the if-clauses to see that I don't break an unrelated
case.

Sure, the above code is compact and duplicates nothing, but it also
intertwines things that would be best kept separate.

The old way in ivi-test-5 is not perfect either. It duplicates more
things than it should. May I suggest a compromise solution? Keep the
big case structure, but perhaps refactor the actual weston invocation
outside it. The case structure could assign variables, just once and
never overwriting, so it would be clear to read yet duplicate the
minimum. Things like --socket, --backend and --log shouldn't be
duplicated since they never vary by test type.

That's just the overall structure, there would still be many details to
clean up. Would that work?

If you want to work on that, I can hold up with the ivi testing series
another day or two, but I'd really like to land it this week.


Thanks,
pq