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

[Dylan Baker]

On Monday, December 08, 2014 07:42:18 AM Chad Versace wrote:
> 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.
> 

two lines between toplevel functions and classes, one between imports
and constants.

Since you use vim, may I suggest Syntastic + pylint? This plus the
missing imports was generated by that tool.

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

I was just looking for a good way to differentiate python code from the
rest of the comment. Although, since I use gvim to write emails I'm
pretty sure I could code something up...

> 
> 
> 
> >> +    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: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20141208/3ea06448/attachment-0001.sig>