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

[Ilia Mirkin]

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.

>
>> +                    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? :)

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

>
>>
>>  # Drop ARB_vertex_program/ARB_fragment_program compiler tests.
>>  del profile.tests['spec']['ARB_vertex_program']