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

[Chad Versace]

On 12/08/2014 03:45 AM, Dylan Baker wrote:
> Hi Chad,
> 
> I have a few comments below for you, mostly relating to the use of the
> global keyword.

I admit that I had trouble with the global variables in this patch.
Python wasn't behaving like C, and that confused me.




>> 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)

'bin' does feel more Linuxy than 'exe'. I'll change it.




>> +import ConfigParser
>> +import os
>> +import shutil
> 
> unused
> 
>> +import subprocess
>> +import sys
> 
> unused
> 
>> +import tempfile
> 
> unused

You found the breadcrumbs of the patch's evolution ;) I'll remove the
unused imports.



>> +
>> +import framework.profile
>> +
>> +
> 
> You can lose a newline here if you want
> 
>> +__all__ = ['profile']
>> +
>> +
> 
> And here

I thought the PEP8 tools complained if toplevel objects were not
separated by two newlines... I'll squash the space as you suggest.



>> +_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

Ok.



>> +
>> +    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.

Ok, I'll initialize the global constant near the top of file.

By the way, does GMail properly syntax-highlight your email when you
write "```python"? Or is that just a habit of yours?



>> +    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.

Good to know. Will fix.



>> +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__.

Ok.



>> +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.

Oh... you've solved my mystery: add_tests() believed 'profile' was a
local variable because 'profile' was declared after add_tests(). I'll
fix the patch to remove the global keywords and resubmit.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141208/88f5c3b7/attachment.sig>