[Piglit] replace glslparsertest standalone functionality

[Ilia Mirkin]

On Thu, Feb 27, 2014 at 4:21 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> On Thursday, February 27, 2014 15:32:06 you wrote:
>> On Thu, Feb 27, 2014 at 2:43 PM, Dylan Baker <baker.dylan.c at gmail.com>
> wrote:
>> > This small series replaces glsl_parser_test.py's standalone
>> > functionality with a small utility script for running a single
>> > glslparsertest.
>> >
>> > The output of this file is less than ideal, however, that is becasue
>> > core.Test does some very silly things with the output of a test. That is
>> > a patch for another day.
>> >
>> > I'm not set on where to put the utility files, Fabian suggested that it go
>> > in tests/ somewhere, I like the idea of keeping little utilites
>> > together (I'd like to move generate-glean-test.py,
>> > piglit-print-commands.py, and piglit-merge-results.py into the utils
>> > folder as well), and putting them in tests feels odd to me, since that
>> > is a folder for tests, not little utilities.
>>
>> These ideas may be apparent to everyone, but thought I'd mention them
>> anyways:
>>
>> Piglit is composed of 2 largely unrelated bits -- a bunch of tests for
>> various opengl/etc functionality, and a framework to run tests (which,
>> perhaps not entirely coincidentally, are those very same
>> opengl/opencl/etc tests). There is a funny overlap region, which sets
>> up mechanisms to run individual tests which are fancier than just
>> "execute this command". But the fact that opengl tests are executed is
>> dictacted by passing in tests/all.py as the "seed the tests" file. I
>> instead could pass in /path/to/other_file.py which would add tests to
>> the framework that are of an entirely different nature, and as long as
>> those tests abide by the API for returning results, the framework
>> would be none-the-wiser.
>
> We have had a lot of discussions about the 2 parts actually. There are some
> real advantages to splitting the two parts (and obviously some drawbacks as
> well). On the all.py test file, I'd like to move to a purely descriptive format
> for tests (I like XML, but I know that many people don't)

That sounds nice in theory. In practice there are things like
generated tests/etc. I suppose you could create a format that
encompasses all of the current custom logic... and then have to extend
it any time you want more. I'm actually a big fan of having python
files for that -- the majority of the time they do something very
simple, but are able to do more complicated things. But usually I do
restrict what they can do, e.g. no importing, and give them a few
built-in functions like glob() and so on that help them do what they
need to do. That way people don't get too creative. (e.g. see
https://github.com/yext/icbm/blob/master/icbm/data.py#L607)

>
>>
>> IMHO GLSLParserTest does not belong in framework at all, and should
>> instead be in tests (perhaps tests/util or tests/bin). Similarly for
>> ShaderTest -- the reason it exists at all seems to be to determine
>> whether a particular shader test is GL/GLES2/GLES3 and execute the
>> appropriate binary. And it's advantageous to have only one of those
>> due to the regex compiling... but it's not exactly a surprise to the
>> author of a shader_test which API it specifies (be it a human being or
>> a python script), one could certainly imagine using diff extensions
>> for the diff ones, much as diff binaries are used.
>
> I have some patches out to clean up glsl_parser_test.py and shader_test.py (If
> you're bored and want to have a look at them :) ). While I was working on that
> I began to wonder if the "right" solution is to just move all of the parsing
> out of python into the c binary (a single shader_test binary and a single
> glslparsertest binary), that can figure out whether the test is OpenGL or
> OpenGLES version x and react appropriately. At that point a glslparsertest or

That's no different than declaring shader_runner.py as being the thing
that runs the test instead of shader_runner. (Which would be fine,
IMO, although we'd have to check for slowdown -- I think that just
using different extensions for the different ones would be quite easy
and the most efficient.)

> shadertest would just be a PlainExecTest with default options. It moves the
> complexity around, but it seems ideal to be able to say "this is a
> shader_test, I'll pass it to ShaderTest and it'll work"

The question I think is appropriate is whether a different set of
tests using the test-running framework would make use of ShaderTest
and GLSLParserTest. My take is that "no" :) So then they don't really
belong in framework.

  -ilia