[PATCH v1 weston 05/11] tests: Allow tests to use customized command line parameters

[Pekka Paalanen]

On Fri, 21 Nov 2014 12:38:35 -0800
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Fri, Nov 21, 2014 at 04:56:03PM +0200, Pekka Paalanen wrote:
> > On Wed, 19 Nov 2014 15:06:20 -0800
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> > > From: Derek Foreman <derekf at osg.samsung.com>
> > > 
> > > Tests will now return the extra command line parameters they need
> > > when run with --params
> > > 
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > > ---
> > >  tests/weston-test-runner.c | 8 ++++++++
> > >  tests/weston-tests-env     | 1 +
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/tests/weston-test-runner.c b/tests/weston-test-runner.c
> > > index ef45bae..ce0a670 100644
> > > --- a/tests/weston-test-runner.c
> > > +++ b/tests/weston-test-runner.c
> > > @@ -34,6 +34,8 @@
> > >  
> > >  #define SKIP 77
> > >  
> > > +char __attribute__((weak)) *server_parameters="";
> > > +
> > >  extern const struct weston_test __start_test_section, __stop_test_section;
> > >  
> > >  static const struct weston_test *
> > > @@ -154,6 +156,12 @@ int main(int argc, char *argv[])
> > >  			exit(EXIT_SUCCESS);
> > >  		}
> > >  
> > > +		if (strcmp(testname, "--params") == 0 ||
> > > +		    strcmp(testname, "-p") == 0) {
> > > +			printf("%s", server_parameters);
> > > +			exit(EXIT_SUCCESS);
> > > +		}
> > > +
> > >  		t = find_test(argv[1]);
> > >  		if (t == NULL) {
> > >  			fprintf(stderr, "unknown test: \"%s\"\n", argv[1]);
> > > diff --git a/tests/weston-tests-env b/tests/weston-tests-env
> > > index e332354..aaf3ee1 100755
> > > --- a/tests/weston-tests-env
> > > +++ b/tests/weston-tests-env
> > > @@ -46,5 +46,6 @@ case $TESTNAME in
> > >  			--shell=$SHELL_PLUGIN \
> > >  			--log="$SERVERLOG" \
> > >  			--modules=$TEST_PLUGIN,$XWAYLAND_PLUGIN \
> > > +			$($abs_builddir/$TESTNAME --params) \
> > >  			&> "$OUTLOG"
> > >  esac
> > 
> > Hi,
> > 
> > this is pretty clever. I do wonder though if we are going to need a
> > test-specific weston.ini at some point, too. If we do, do we also need
> > to control command line options? Well, apart from specifying the config
> > file location.
> 
> There are three different external, direct mechanisms for adjusting
> weston's behavior:
> 
>   * command line options and parameters
>   * weston.ini config file
>   * environment variables
> 
> Now, afaik we don't have any policies that one of these should be
> preferred and/or a superset of the others, so presumably future tests
> are going to want to test settings against any of the three mechanisms.
> Which means we will need to enable tests to have control over server
> command line options, environment variables, and weston.ini.
> 
> If we were to set as a policy that, say, all command line options (and
> maybe environment variables) must have an equivalent setting in
> weston.ini, then we could provide only that interface inside tests.
> This would simplify things from a testing perspective, but it would
> require the rest of the project to adhere to this constraint.
> 
> OTOH, we could take the inverse route, and add a command line option
> that allows setting/overriding any arbitrary parameter that can be set
> in weston.ini.  (We took this approach in Inkscape with a --verb
> command, that ended up handy both for testing and for other mechanized
> uses of Inkscape.)

Mm, yes, I'm not sure which way to go.

Currently Weston does not have a command line option to read a specific
config file, but there is --no-config. Adding a standard mapping from
all weston.ini options to command line options would be fairly
straightforward I assume, something like '-c/--conf=section:key=value'
and easy to document.

The problem with command line options is that we do not pass them to
helper clients. Helpers like weston-desktop-shell just parse weston.ini
on their own. I'm not sure even --no-config gets propagated to the
helpers.

Something should probably be done to tie the helpers better to the
compositor's config, however that is received.

From security perspective, there is no difference between command line
options and config file, is there?

OTOH, crafting custom config files at test runtime is a bit labourious.

Environment variables we probably need to check case by case. I think
few are because we didn't have a way to give any custom options to
Weston from the tests.

> With respect to the environment variables, fortunately there's not too
> many of them so far.  I've started compiling a list and documenting what
> they do.

Excellent!

> > While this patch is fine for now, we are probably going to need
> > something more flexible soon. Currently we only run tests on a single
> > backend at a time, and a single shell at a time. There will become time
> > when we need to test different shell plugins. Oh hey, but this could
> > also work for shell plugins, as tests would likely be shell-specific.
> > 
> > It might also be nice to have a bit of heuristics and by default run on
> > all backends that might work:
> > - always headless with each renderer
> > - if DISPLAY is set, x11-backend
> > - if WAYLAND_DISPLAY is set, wayland-backend
> > - if neither DISPLAY nor WAYLAND_DISPLAY set, drm-backend, if DRM
> >   available
> > Hm, or maybe that doesn't make much sense... but being able to list
> > which all backends should run through all (the specified) tests would
> > be nice.
> 
> So like a --list-backends?  That does sound handy.

I meant more like 'make check BACKENDS=headless,x11'.

> Then, an automated test could iterate over those and then just call with
> --backend=XXX as needed, and can do whatever logic it wants if a given
> backend is missing.
> 
> For an individual tester, usually the preference is for it to run
> quickly, and with as little fiddling as possible.  I imagine they
> probably only care about one backend at a time anyway, so in this case I
> think you're right that running against more than one may not be the
> desired behavior.  For this use case heuristics should probably be used
> only insofar as to figure that out when they're not stating it
> explicitly.  And they'd be able to utilize --list-backends to help
> sleuth out why their desired backend isn't available.
> 
> > Oh well, random thoughts for the future, nothing to complain about
> > this now. Anyway, I started to look at this series and got this far this
> > week, I wanted to say. Looks good so far. :-)
> > 
> > I figured I want to merge this series first, so we have some tests
> > in place when I look at the matrix rewrite series, since that has some
> > potential to break some rendering.
> 
> Nice, sounds good.

Thanks,
pq