[Piglit] [PATCH 2/4] all.tests: Make concurrent test setup a general helper function.

[Eric Anholt]

On Mon, 12 Sep 2011 12:02:11 -0700, Paul Berry <stereotype441 at gmail.com> wrote:
Non-text part: multipart/alternative
> On 8 September 2011 23:07, Eric Anholt <eric at anholt.net> wrote:
> 
> > ---
> >  tests/all.tests |   14 +++++++-------
> >  1 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/all.tests b/tests/all.tests
> > index 03dcd59..fc66ff6 100644
> > --- a/tests/all.tests
> > +++ b/tests/all.tests
> > @@ -25,6 +25,12 @@ generatedTestDir = os.path.join(
> >                os.path.join(os.path.dirname(__file__), '..')),
> >        'generated_tests')
> >
> > +# Quick wrapper for PlainExecTest for our usual concurrent args.
> > +def concurrent_test(args):
> > +       test = PlainExecTest(args + " -auto -fbo")
> > +       test.runConcurrent = True
> > +       return test
> > +
> >  ######
> >  # Collecting all tests
> >  profile = TestProfile()
> > @@ -293,12 +299,6 @@ add_fbo_depthstencil_tests(general, 'default_fb')
> >
> >  shaders = Group()
> >
> > -def add_shader_test(group, filepath, test_name):
> > -       """Add a shader test to the given group."""
> > -       group[test_name] = PlainExecTest(['shader_runner', '-auto', '-fbo',
> > -                                         filepath])
> > -       group[test_name].runConcurrent = True
> > -
> >  def add_shader_test_dir(group, dirpath, recursive=False):
> >        """Add all shader tests in a directory to the given group."""
> >        for filename in os.listdir(dirpath):
> > @@ -314,7 +314,7 @@ def add_shader_test_dir(group, dirpath,
> > recursive=False):
> >                        if ext != 'shader_test':
> >                                continue
> >                        testname = filename[0:-(len(ext) + 1)] # +1 for '.'
> > -                       add_shader_test(group, filepath, testname)
> > +                       group[testname] = concurrent_test('shader_runner '
> > + filepath)
> >
> >  def add_getactiveuniform_count(group, name, expected):
> >        path = 'shaders/'
> > --
> > 1.7.5.4
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
> >
> 
> This patch introduces a limitation to add_shader_test_dir(): it no longer
> works properly when a test name or directory contains spaces.  Although
> spaces in file/directory names are generally frowned upon in the UNIX world
> (and Piglit currently doesn't contain any such files or directories), I
> don't think that's any excuse to make the code fragile.
> 
> In addition, the new function concurrent_test() has the limitation that it
> can *only* be called with a string, not a list of strings.  Since the
> intended purpose of concurrent_test() is to take the place of ExecTest() for
> tests that should be executed concurrently, it would avoid confusion if it
> accepted inputs in the same form that ExecTest() accepts.
> 
> In my experience, the best way to avoid issues like these is to convert from
> a space-separated string to a list as early as possible (at the top of
> concurrent_test()), and then manipulate everything as lists of strings
> thereafter.

Oops, I managed to archive extra too much mail while catching up, and
only saw the last one that I talked to you in person about (shlex).

Overall, I've found the list form of arguments to work poorly in
all.tests -- the texwrap instances are a particularly egregious example
of where we've ended up.  I think if we could get to consistently using
strings, we'd have a more readable and maintainable file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20110919/a6e29921/attachment.pgp>