[Piglit] [PATCH v2] core: fix remove_tests logic

[Dylan Baker]

On Friday, January 17, 2014 12:33:11 AM Ilia Mirkin wrote:
> On Fri, Jan 17, 2014 at 12:26 AM, Dylan Baker <baker.dylan.c at gmail.com> 
wrote:
> > This looks pretty, good I have a few comments.
> > 
> > On Thursday, January 16, 2014 11:35:01 PM Ilia Mirkin wrote:
> >> The sole user is tests/gpu.py, which was passing in a predicate that
> >> worked (a) as a filter, and (b) expected to receive a test. However the
> >> implementation operated on the dict-of-dicts. Instead keep track of all
> >> the filtering functions, and apply the filters when flattening the tests
> >> tree.
> >> 
> >> Take this opportunity to pass in a second argument to the filter
> >> function: the full test name. This should allow future conversion of the
> >> 'del' style of filtering.
> >> 
> >> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> >> ---
> >> 
> >> Dylan, I've made enough changes here that I don't feel good about
> >> applying
> >> your R-b without checking.
> >> 
> >>  framework/core.py | 28 ++++++++++++++++------------
> >>  tests/gpu.py      |  2 +-
> >>  2 files changed, 17 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/framework/core.py b/framework/core.py
> >> index 368c996..4601986 100644
> >> --- a/framework/core.py
> >> +++ b/framework/core.py
> >> 
> >> @@ -519,6 +519,7 @@ class TestProfile:
> >>      def __init__(self):
> >>          self.tests = Group()
> >>          self.test_list = {}
> >> 
> >> +        self.filters = []
> >> 
> >>      def flatten_group_hierarchy(self):
> >>          '''
> >> 
> >> @@ -548,14 +549,21 @@ class TestProfile:
> >>          def matches_any_regexp(x, re_list):
> >>              return True in map(lambda r: r.search(x) is not None,
> >>              re_list)
> >> 
> >> -        def test_matches(item):
> >> -            path, test = item
> >> +        def test_matches(path, test):
> >> +            """Filter for user-specified restrictions"""
> >> 
> >>              return ((not env.filter or matches_any_regexp(path,
> >> 
> >> env.filter)) and not path in env.exclude_tests and
> >> 
> >>                      not matches_any_regexp(path, env.exclude_filter))
> >> 
> >> +        filters = self.filters + [test_matches]
> >> +        def check_all(item):
> >> +            for f in filters:
> > 
> >> +                if not f(*item):
> > I really don't like using * magic if we can avoid it, and I think we can.
> 
> Fine. I'm a big fan of it,, along with its ** friend, but wtvr. It can
> be confusing to people not super-familiar with python.
> 

If you don't know what it does it's really had to search for ;), and I 
remember reading somewhere that upstream recomends limiting it's use to 
situation where you can't know how many elements an argument will have.

> >> +                    return False
> >> +            return True
> >> +
> >> 
> >>          # Filter out unwanted tests
> >> 
> >> -        self.test_list = dict(filter(test_matches,
> >> self.test_list.items())) +        self.test_list =
> >> dict(filter(check_all,
> >> self.test_list.iteritems()))
> > 
> > filter is deprecated, while you're rewriting this can you use a
> > comprehension? something like:
> > self.test_list = dict([(x, y) for x, y in self.test_list.iteritems() if
> > check_all(x, y)])
> 
> I'll go one further and use a generator! What is this, python 2.2? :)

we need to be 2.6 compliant, otherwise I'd suggest a dictionary comprehension.

> 
> >>      def run(self, env, json_writer):
> >>          '''
> >> 
> >> @@ -623,17 +631,13 @@ class TestProfile:
> >>              group = group[group_name]
> >>          
> >>          del group[l[-1]]
> >> 
> >> -    def remove_tests(self, function):
> >> -        """ Remove tests that return true from function
> >> -
> >> -        This is a destructive method, and passing an incorrect function
> >> could -        result in undesired behavior.
> >> +    def filter_tests(self, function):
> >> +        """Remove tests that return false from function
> >> 
> >> +        The function should expect to receive two arguments: the test
> >> name, +        and the test object itself.
> >> 
> >>          """
> > 
> > This is confusing, since clearly this function only takes on argument. I
> > would say something like:
> > "Arguments:
> > function -- a callable for filtering, should accept a two arguments as a
> > key value/pair"
> 
> OK. key/value doesn't mean much to most people, I'll describe what the
> two values actually are.
> 
> >> -        # What we really want is a dictionary comprehension, but since
> >> python -        # 2.6 is officially supported we can't use one
> >> -        # {k:v for k, v in self.tests.iteritems() if function(v)}
> >> -        self.tests = dict((k,v) for k, v in self.tests.iteritems() if
> >> function(v)) +        self.filters.append(function)
> >> 
> >>      def update(self, *profiles):
> >>          """ Updates the contents of this TestProfile instance with
> >>          another
> >> 
> >> diff --git a/tests/gpu.py b/tests/gpu.py
> >> index d7690ae..24ab766 100644
> >> --- a/tests/gpu.py
> >> +++ b/tests/gpu.py
> >> @@ -8,7 +8,7 @@ from framework.glsl_parser_test import GLSLParserTest
> >> 
> >>  __all__ = ['profile']
> >>  
> >>  # Remove all glsl_parser_tests, as they are compiler test
> >> 
> >> -profile.remove_tests(lambda x: not isinstance(x, GLSLParserTest))
> >> +profile.filter_tests(lambda n, t: not isinstance(t, GLSLParserTest))
> > 
> > what does n do here?
> 
> name of the test. how about 'p' for path if 'n' is confusing? (and it
> doesn't do anything, but it has to conform to the ABI which is 2 args)

What about the _ place holder value? And a comment to mention you need that to 
fit the ABI?

> 
> >>  # Drop ARB_vertex_program/ARB_fragment_program compiler tests.
> >>  del profile.tests['spec']['ARB_vertex_program']
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140116/079f416d/attachment.pgp>