framework: fix binary assignment for shader_runner

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@intel.com>
cc: mark.a.janes@intel.com
cc: currojerez@riseup.net
Tested-by: Michel Dänzer <michel.daenzer@amd.com>

3 files changed, 88 insertions, 80 deletions

diff --git a/framework/test/opengl.py b/framework/test/opengl.py
index 000bd843d..51c8f194a 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 f3c945c06..e67c5eda4 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
@@ -52,7 +52,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 +82,58 @@ 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
 
+        # Select the correct binary to run the test, but be as conservative as
+        # possible by always selecting the lowest version that meets the
+        # criteria.
+        if gles_version:
+            if op in ['<', '<='] or op in ['=', '>='] and gles_version < 3:
+                prog = 'shader_runner_gles2'
+            else:
+                prog = 'shader_runner_gles3'
+        else:
+            prog = 'shader_runner'
+
+        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 bd54624dc..9cf8b6042 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."""