[Piglit] [PATCH] framework: fix binary assignment for shader_runner

Dylan Baker dylan at pnwbakers.com
Tue Aug 9 00:05:53 UTC 2016


Currently the python layer tries to dispatch shader_tests to the right
binary (gles2, gles3 or gl). But the implementation is pretty dumb. It
basically assumes either >= or == are the only operators used, and makes
wrong assignments for < or <=, which are allowed. Is also only
understands 2.0 and 3.0 as GLES versions, which is problematic.

This version uses the closes equivalent of a match statement that python
has, to try to pick the right GLES version, otherwise it uses the GL
version. It also shares this picking mechanism with the Fast Skip
mechanism, so they always have the same result.

Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
cc: mark.a.janes at intel.com
cc: currojerez at riseup.net
---

I'm waiting for Jenkins to finish to confirm that nothing was broken,
but the tests at least run now and don't die during initialization of
the profile like they did before.

 framework/test/opengl.py                     |  20 ++--
 framework/test/shader_test.py                | 148 ++++++++++++++++-----------
 unittests/framework/test/test_shader_test.py |  44 +++++---
 3 files changed, 132 insertions(+), 80 deletions(-)

diff --git a/framework/test/opengl.py b/framework/test/opengl.py
index 000bd84..51c8f19 100644
--- a/framework/test/opengl.py
+++ b/framework/test/opengl.py
@@ -318,13 +318,15 @@ class FastSkipMixin(object):
     # other than querying each value before starting the thread pool.
     __info = WflInfo()
 
-    def __init__(self, *args, **kwargs):
-        super(FastSkipMixin, self).__init__(*args, **kwargs)
-        self.gl_required = set()
-        self.gl_version = None
-        self.gles_version = None
-        self.glsl_version = None
-        self.glsl_es_version = None
+    def __init__(self, command, gl_required=None, gl_version=None,
+                 gles_version=None, glsl_version=None, glsl_es_version=None,
+                 **kwargs):  # pylint: disable=too-many-arguments
+        super(FastSkipMixin, self).__init__(command, **kwargs)
+        self.gl_required = gl_required or set()
+        self.gl_version = gl_version
+        self.gles_version = gles_version
+        self.glsl_version = glsl_version
+        self.glsl_es_version = glsl_es_version
 
     def is_skip(self):
         """Skip this test if any of it's feature requirements are unmet.
@@ -340,6 +342,7 @@ class FastSkipMixin(object):
                         'Test requires extension {} '
                         'which is not available'.format(extension))
 
+        # TODO: Be able to handle any operator
         if (self.__info.gl_version is not None
                 and self.gl_version is not None
                 and self.gl_version > self.__info.gl_version):
@@ -348,6 +351,7 @@ class FastSkipMixin(object):
                 'but only {} is available'.format(
                     self.gl_version, self.__info.gl_version))
 
+        # TODO: Be able to handle any operator
         if (self.__info.gles_version is not None
                 and self.gles_version is not None
                 and self.gles_version > self.__info.gles_version):
@@ -356,6 +360,7 @@ class FastSkipMixin(object):
                 'but only {} is available'.format(
                     self.gles_version, self.__info.gles_version))
 
+        # TODO: Be able to handle any operator
         if (self.__info.glsl_version is not None
                 and self.glsl_version is not None
                 and self.glsl_version > self.__info.glsl_version):
@@ -364,6 +369,7 @@ class FastSkipMixin(object):
                 'but only {} is available'.format(
                     self.glsl_version, self.__info.glsl_version))
 
+        # TODO: Be able to handle any operator
         if (self.__info.glsl_es_version is not None
                 and self.glsl_es_version is not None
                 and self.glsl_es_version > self.__info.glsl_es_version):
diff --git a/framework/test/shader_test.py b/framework/test/shader_test.py
index f3c945c..f29c318 100644
--- a/framework/test/shader_test.py
+++ b/framework/test/shader_test.py
@@ -26,8 +26,8 @@
 from __future__ import (
     absolute_import, division, print_function, unicode_literals
 )
-import re
 import io
+import re
 
 from framework import exceptions
 from .opengl import FastSkipMixin
@@ -38,6 +38,53 @@ __all__ = [
 ]
 
 
+class _SelectProg(object):
+    """A class for selecting the correct program.
+
+    This is implemented as a class with a call method to avoid having to
+    reinitialize the _find as a function, or placing it in the global
+    namspace.
+    """
+    # This is sort of the python equivalent of a case statement.
+    _find = {
+        # pylint: disable=bad-whitespace
+        ('<=', 2.0): 'shader_runner_gles2',
+        ('=',  2.0): 'shader_runner_gles2',
+        ('>=', 2.0): 'shader_runner_gles2',
+        ('>',  2.0): 'shader_runner_gles3',
+
+        ('<',  3.0): 'shader_runner_gles2',
+        ('<=', 3.0): 'shader_runner_gles2',
+        ('=',  3.0): 'shader_runner_gles3',
+        ('>=', 3.0): 'shader_runner_gles3',
+        ('>',  3.0): 'shader_runner_gles3',
+
+        ('<',  3.1): 'shader_runner_gles2',
+        ('<=', 3.1): 'shader_runner_gles2',
+        ('=',  3.1): 'shader_runner_gles3',
+        ('>=', 3.1): 'shader_runner_gles3',
+        ('>',  3.1): 'shader_runner_gles3',
+
+        ('<',  3.2): 'shader_runner_gles2',
+        ('<=', 3.2): 'shader_runner_gles2',
+        ('=',  3.2): 'shader_runner_gles3',
+        ('>=', 3.2): 'shader_runner_gles3',
+    }
+
+    def __call__(self, is_gles, version, op):
+        if is_gles:
+            try:
+                return self._find[(op, version)]
+            except KeyError:
+                raise exceptions.PiglitInternalError(
+                    'No binary available for GLES {} {}'.format(op, version))
+        else:
+            return 'shader_runner'
+
+
+select_prog = _SelectProg()
+
+
 class ShaderTest(FastSkipMixin, PiglitBaseTest):
     """ Parse a shader test file and return a PiglitTest instance
 
@@ -52,7 +99,13 @@ class ShaderTest(FastSkipMixin, PiglitBaseTest):
         r'^GLSL\s+(?P<es>ES)?\s*(?P<op>(<|<=|=|>=|>))\s*(?P<ver>\d\.\d+)')
 
     def __init__(self, filename):
-        self.gl_required = set()
+        gl_required = set()
+        gl_version = None
+        gles_version = None
+        glsl_version = None
+        glsl_es_version = None
+        op = None
+        sl_op = None
 
         # Iterate over the lines in shader file looking for the config section.
         # By using a generator this can be split into two for loops at minimal
@@ -76,74 +129,55 @@ class ShaderTest(FastSkipMixin, PiglitBaseTest):
                 raise exceptions.PiglitFatalError(
                     "In file {}: Config block not found".format(filename))
 
-            lines = list(lines)
-
-        prog = self.__find_gl(lines, filename)
-
-        super(ShaderTest, self).__init__([prog, filename], run_concurrent=True)
-
-        # This needs to be run after super or gl_required will be reset
-        self.__find_requirements(lines)
-
-    def __find_gl(self, lines, filename):
-        """Find the OpenGL API to use."""
-        for line in lines:
-            line = line.strip()
-            if line.startswith('GL ES'):
-                if line.endswith('3.0'):
-                    prog = 'shader_runner_gles3'
-                elif line.endswith('2.0'):
-                    prog = 'shader_runner_gles2'
-                # If we don't set gles2 or gles3 continue the loop,
-                # probably htting the exception in the for/else
-                else:
-                    raise exceptions.PiglitFatalError(
-                        "In File {}: No GL ES version set".format(filename))
-                break
-            elif line.startswith('[') or self._is_gl.match(line):
-                # In the event that we reach the end of the config black
-                # and an API hasn't been found, it's an old test and uses
-                # "GL"
-                prog = 'shader_runner'
-                break
-        else:
-            raise exceptions.PiglitFatalError(
-                "In file {}: No GL version set".format(filename))
-
-        return prog
-
-    def __find_requirements(self, lines):
-        """Find any requirements in the test and record them."""
         for line in lines:
             if line.startswith('GL_') and not line.startswith('GL_MAX'):
-                self.gl_required.add(line.strip())
+                gl_required.add(line.strip())
                 continue
 
-            if not (self.gl_version or self.gles_version):
-                # Find any gles requirements
+            # Find any GLES requirements.
+            if not (gl_version or gles_version):
                 m = self._match_gl_version.match(line)
                 if m:
-                    if m.group('op') not in ['<', '<=']:
-                        if m.group('es'):
-                            self.gles_version = float(m.group('ver'))
-                        else:
-                            self.gl_version = float(m.group('ver'))
-                        continue
-
-            if not (self.glsl_version or self.glsl_es_version):
+                    op = m.group('op')
+                    if m.group('es'):
+                        gles_version = float(m.group('ver'))
+                    else:
+                        gl_version = float(m.group('ver'))
+                    continue
+
+            if not (glsl_version or glsl_es_version):
                 # Find any GLSL requirements
                 m = self._match_glsl_version.match(line)
                 if m:
-                    if m.group('op') not in ['<', '<=']:
-                        if m.group('es'):
-                            self.glsl_es_version = float(m.group('ver'))
-                        else:
-                            self.glsl_version = float(m.group('ver'))
-                        continue
+                    sl_op = m.group('op')
+                    if m.group('es'):
+                        glsl_es_version = float(m.group('ver'))
+                    else:
+                        glsl_version = float(m.group('ver'))
+                    continue
 
             if line.startswith('['):
                 break
 
+        try:
+            prog = select_prog(bool(gles_version),
+                               gles_version or gl_version,
+                               op)
+        except exceptions.PiglitInternalError as e:
+            raise exceptions.PiglitFatalError(
+                str(e) + ' in file {}'.format(filename))
+
+        super(ShaderTest, self).__init__(
+            [prog, filename],
+            run_concurrent=True,
+            gl_required=gl_required,
+            # FIXME: the if here is related to an incomplete feature in the
+            # FastSkipMixin
+            gl_version=gl_version if op not in ['<', '<='] else None,
+            gles_version=gles_version if op not in ['<', '<='] else None,
+            glsl_version=glsl_version if sl_op not in ['<', '<='] else None,
+            glsl_es_version=glsl_es_version if sl_op not in ['<', '<='] else None)
+
     @PiglitBaseTest.command.getter
     def command(self):
         """ Add -auto to the test command """
diff --git a/unittests/framework/test/test_shader_test.py b/unittests/framework/test/test_shader_test.py
index bd54624..9cf8b60 100644
--- a/unittests/framework/test/test_shader_test.py
+++ b/unittests/framework/test/test_shader_test.py
@@ -63,28 +63,40 @@ teardown_module = _setup.teardown
 class TestConfigParsing(object):
     """Tests for ShaderRunner config parsing."""
 
-    def test_no_decimal(self, tmpdir):
-        """test.shader_test.ShaderTest: raises if version lacks decminal."""
+    @pytest.mark.parametrize('gles,operator,expected', [
+        # pylint: disable=bad-whitespace
+        ('2.0', '>=', 'shader_runner_gles2'),
+        ('2.0', '<=', 'shader_runner_gles2'),
+        ('2.0', '=',  'shader_runner_gles2'),
+        ('2.0', '>',  'shader_runner_gles3'),
+        ('3.0', '>=', 'shader_runner_gles3'),
+        ('3.0', '<=', 'shader_runner_gles2'),
+        ('3.0', '=',  'shader_runner_gles3'),
+        ('3.0', '>',  'shader_runner_gles3'),
+        ('3.0', '<',  'shader_runner_gles2'),
+        ('3.1', '>=', 'shader_runner_gles3'),
+        ('3.1', '<=', 'shader_runner_gles2'),
+        ('3.1', '=',  'shader_runner_gles3'),
+        ('3.1', '>',  'shader_runner_gles3'),
+        ('3.1', '<',  'shader_runner_gles2'),
+        ('3.2', '>=', 'shader_runner_gles3'),
+        ('3.2', '<=', 'shader_runner_gles2'),
+        ('3.2', '=',  'shader_runner_gles3'),
+        ('3.2', '<',  'shader_runner_gles2'),
+    ])
+    def test_bin(self, gles, operator, expected, tmpdir):
+        """Test that the shader_runner parse picks the correct binary."""
         p = tmpdir.join('test.shader_test')
         p.write(textwrap.dedent("""\
             [require]
-            GL = 2
-            """))
-
-        with pytest.raises(exceptions.PiglitFatalError):
-            shader_test.ShaderTest(six.text_type(p))
-
-    def test_gles2_bin(self, tmpdir):
-        """test.shader_test.ShaderTest: Identifies GLES2 tests successfully."""
-        p = tmpdir.join('test.shader_test')
-        p.write(textwrap.dedent("""\
-            [require]
-            GL ES >= 2.0
+            GL ES {} {}
             GLSL ES >= 1.00
-            """))
+
+            [next section]
+            """.format(operator, gles)))
         test = shader_test.ShaderTest(six.text_type(p))
 
-        assert os.path.basename(test.command[0]) == "shader_runner_gles2"
+        assert os.path.basename(test.command[0]) == expected
 
     def test_gles3_bin(self, tmpdir):
         """test.shader_test.ShaderTest: Identifies GLES3 tests successfully."""
-- 
2.9.2



More information about the Piglit mailing list