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

Submitted by Bryce Harrington on Nov. 19, 2014, 11:06 p.m.

Details

Message ID 1416438386-4783-6-git-send-email-bryce@osg.samsung.com
State Accepted
Headers show

Not browsing as part of any series.

Commit Message

Bryce Harrington Nov. 19, 2014, 11:06 p.m.
From: Derek Foreman <derekf@osg.samsung.com>

Tests will now return the extra command line parameters they need
when run with --params

Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
---
 tests/weston-test-runner.c | 8 ++++++++
 tests/weston-tests-env     | 1 +
 2 files changed, 9 insertions(+)

Patch hide | download patch | download mbox

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

Comments

On Wed, 19 Nov 2014 15:06:20 -0800
Bryce Harrington <bryce@osg.samsung.com> wrote:

> From: Derek Foreman <derekf@osg.samsung.com>
> 
> Tests will now return the extra command line parameters they need
> when run with --params
> 
> Signed-off-by: Bryce Harrington <bryce@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.

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.

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.


Thanks,
pq
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@osg.samsung.com> wrote:
> 
> > From: Derek Foreman <derekf@osg.samsung.com>
> > 
> > Tests will now return the extra command line parameters they need
> > when run with --params
> > 
> > Signed-off-by: Bryce Harrington <bryce@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.)

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.
 
> 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.

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.

Bryce
On Fri, 21 Nov 2014 12:38:35 -0800
Bryce Harrington <bryce@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@osg.samsung.com> wrote:
> > 
> > > From: Derek Foreman <derekf@osg.samsung.com>
> > > 
> > > Tests will now return the extra command line parameters they need
> > > when run with --params
> > > 
> > > Signed-off-by: Bryce Harrington <bryce@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