[Piglit] [PATCH] deqp: Add test profile for external dEQP-GLES3 tests

[Dylan Baker]

Hi Chad,

I have a few comments below for you, mostly relating to the use of the
global keyword.

On Sunday, December 07, 2014 08:54:06 PM Chad Versace wrote:
> Google opensourced the drawElements Quality Product Testsuite (dEQP) as
> part of the Android Lollipop release. It's git repo lives in the Android
> tree at [https://android.googlesource.com/platform/external/deqp/].
> 
> This patch adds a new module, deqp_gles3.py, that runs the dEQP-GLES3
> tests.  It queries the 'deqp-gles3' executable at runtime for the list
> of testcases.  The module attempts queries the 'deqp-gles3' executable
> only if the environment variable PIGLIT_DEQP_GLES3_EXE or the
> piglit.conf option deqp-gles3.exe is set.
> 
> Why do we need to use Pigit as the testrunner for dEQP? (After all, dEQP has
> its own testrunner). Because dEQP runs all tests in a single process. If test
> 17530 of 55409 crashes, then the dEQP testrunner crashes and the remaining
> tests never run. Piglit doesn't suffer from that problem, because it runs each
> test as a separate process.
> 
> Tested on Intel Ivybridge by running the command
>     piglit run -t dEQP-GLES3/info/vendor \
>       tests/deqp_gles3.py tests/quick.py results
> with and without PIGLIT_DEQP_GLES3_EXE set and with and without
> piglit.conf:deqp-gles.exe set.
> 
> Cc: Dylan Baker <dylanx.c.baker at intel.com>
> Cc: Daniel Kurtz <djkurtz at chromium.org>
> Cc: Ilja H. Friedel <ihf at chromium.org>
> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> ---
> 
> You can find this patch on top of my 'deqp-gles3' branch.
> 
>   git://github.com/chadversary/piglit refs/heads/deqp-gles3
> 
> Daniel and Ilja, I CC'd you just so you would be aware of the dEQP/Piglit work
> Intel is doing. It would be nice to see it go into Chrome OS enventually.
> 
> 
>  piglit.conf.example |   7 +++
>  tests/deqp_gles3.py | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+)
>  create mode 100644 tests/deqp_gles3.py
> 
> diff --git a/piglit.conf.example b/piglit.conf.example
> index a864c01..0ac0f4f 100644
> --- a/piglit.conf.example
> +++ b/piglit.conf.example
> @@ -33,6 +33,13 @@ bindir=/home/usr/oclconform
>  testA
>  testB
>  
> +;[deqp-gles3]
> +;
> +; Path to the deqp-gles3 executable. You can also set this with the environment
> +; variable PIGLIT_DEQP_GLES3_EXE. Piglit will run the dEQP-GLES3 tests if and
> +; only if this option is set.
> +;exe=/home/knuth/deqp/modules/gles3/deqp-gles3

nit: bin over exe? (feel free to ignore, not a strong opinion)

> +
>  ; Section for specific oclconform test.  One of these sections is required for
>  ; each test list in the oclconform section and must be called:
>  ; oclconform-$testname
> diff --git a/tests/deqp_gles3.py b/tests/deqp_gles3.py
> new file mode 100644
> index 0000000..422d1fa
> --- /dev/null
> +++ b/tests/deqp_gles3.py
> @@ -0,0 +1,137 @@
> +# Copyright 2014 Intel Corporation
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a copy
> +# of this software and associated documentation files (the "Software"), to deal
> +# in the Software without restriction, including without limitation the rights
> +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> +# copies of the Software, and to permit persons to whom the Software is
> +# furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice shall be included in
> +# all copies or substantial portions of the Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +# SOFTWARE.
> +
> +import ConfigParser
> +import os
> +import shutil

unused

> +import subprocess
> +import sys

unused

> +import tempfile

unused

> +
> +import framework.profile
> +
> +

You can lose a newline here if you want

> +__all__ = ['profile']
> +
> +

And here

> +_deqp_gles3_exe = None
> +
> +
> +def get_option(env_varname, config_option):
> +    """Query the given environment variable and then piglit.conf for the
> +    option. Return None if the option is unset.
> +    """

newline before the closing """ not after

> +
> +    opt = os.environ.get(env_varname, None)
> +    if opt is not None:
> +        return opt
> +
> +    try:
> +        opt = framework.core.PIGLIT_CONFIG.get(config_option[0],
> +                                               config_option[1])
> +    except ConfigParser.NoSectionError, ConfigParser.NoOptionError:
> +        pass
> +
> +    return opt
> +
> +
> +def get_deqp_gles3_exe():
> +    """Get path to the deqp-gles3 executable. Idempotent."""
> +    global _deqp_gles3_exe

No global please.

I'd use a pattern like this:

```python
def _set_deqp_gles3_exe():
	...

_DEQP_GLES3_EXE = _set_deqp_gles3_exe()

[rest of module]
```

Then you can simply rely on the constant at that point.

> +
> +    if _deqp_gles3_exe is None:
> +        _deqp_gles3_exe = get_option('PIGLIT_DEQP_GLES3_EXE',
> +                                     ('deqp-gles3', 'exe'))
> +
> +    return _deqp_gles3_exe
> +
> +
> +def gen_caselist_txt():
> +    """Generate dEQP-GLES3-cases.txt and return its path."""
> +    # dEQP is stupid (2014-12-07):
> +    #   1. To generate the caselist file, dEQP requires that the process's
> +    #      current directory must be that same as that of the executable.
> +    #      Otherwise, it fails to find its data files.
> +    #   2. dEQP creates the caselist file in the process's current directory
> +    #      and provides no option to change its location.
> +    #   3. dEQP creates a GL context when generating the caselist. Therefore,
> +    #      the caselist must be generated on the test target rather than the
> +    #      build host. In other words, when the build host and test target
> +    #      differ then we cannot pre-generate the caselist on the build host:
> +    #      we must *dynamically* generate it during the testrun.
> +    basedir = os.path.dirname(get_deqp_gles3_exe())
> +    caselist_path = os.path.join(basedir, 'dEQP-GLES3-cases.txt')
> +    subprocess.check_call(
> +        [get_deqp_gles3_exe(), '--deqp-runmode=txt-caselist'],
> +        cwd=basedir)
> +    assert(os.path.exists(caselist_path))

assert is a keyword not a function, it doesn't need parentheses. It's
also advisable not to use them, because assert + a tuple doesn't do what
you would expect.

> +    return caselist_path
> +
> +
> +def iter_deqp_test_cases():
> +    """Iterate over original dEQP GLES3 testcase names."""
> +    caselist_path = gen_caselist_txt()
> +    with open(caselist_path) as caselist_file:
> +        for i, line in enumerate(caselist_file):
> +            if line.startswith('GROUP:'):
> +                continue
> +            elif line.startswith('TEST:'):
> +                yield line[len('TEST:'):].strip()
> +            else:
> +                raise Exception('{0}:{1}: ill-formed line'.format(caselist_txt, i))
> +
> +
> +class DEQPTest(framework.profile.Test):
> +
> +    def __init__(self, case_name):
> +        command = [
> +            get_deqp_gles3_exe(),
> +            '--deqp-case=' + case_name,
> +            '--deqp-visibility=hidden',
> +        ]
> +        framework.profile.Test.__init__(self, command)

Please use super instead an explicit call to Test.__init__.

> +
> +        # dEQP's working directory must be the same as that of the executable,
> +        # otherwise it cannot find its data files (2014-12-07).
> +        self.cwd = os.path.dirname(get_deqp_gles3_exe())
> +
> +    def interpret_result(self):
> +        if self.result['returncode'] == 0:
> +            self.result['result'] = 'pass'
> +        else:
> +            self.result['result'] = 'fail'
> +
> +
> +def add_tests():
> +    global profile

eww... global. Please don't use global.

I think you could solve this by moving the profile declaration to the
top of the module, or you could use a pattern like xts.py does, where
the populate_profile() function returns a framework.profile.TestProfile
object.

> +
> +    if get_deqp_gles3_exe() is None:
> +        return
> +
> +    for deqp_testname in iter_deqp_test_cases():
> +        # dEQP uses '.' as the testgroup separator.
> +        piglit_testname = deqp_testname.replace('.', '/')
> +
> +        test = DEQPTest(deqp_testname)
> +        profile.test_list[piglit_testname] = test
> +
> +
> +profile = framework.profile.TestProfile()
> +add_tests()
> -- 
> 1.9.3
> 
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141208/d166b332/attachment.sig>