[Piglit] [PATCH] core: fix remove_tests logic

[Ilia Mirkin]

Or perhaps I didn't understand your objection... what do you mean by
'same time'? It all happens before the tests run... are you saying you
prefer having multiple intermediate copies of the dictionaries
created?

On Thu, Jan 16, 2014 at 6:37 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> How would you feel about a patch that instead adds test_matches to
> filters and then does a filter on some function that only looks
> through self.filters?
>
> On Thu, Jan 16, 2014 at 6:35 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
>> I'm not really thrilled with this patch, here's my reasoning:
>>
>> I think it's important to separate the removal of tests by the profile and
>> those of an end-user. If they use the same code path to achieve that, great!
>> code duplication sucks; but I don't like the idea of filtering tests for the
>> profile and the user at the same time.
>>
>> On Thursday, January 16, 2014 10:13:41 AM 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.
>>>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>
>>> You can see the difference by running in a dry run mode with this patch and
>>> without on gpu.py. [Also the comment on the function was wrong... true keeps
>>> the test.]
>>>
>>>  framework/core.py | 17 +++++++----------
>>>  tests/gpu.py      |  2 +-
>>>  2 files changed, 8 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/framework/core.py b/framework/core.py
>>> index 368c996..f3cc270 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):
>>>          '''
>>> @@ -550,6 +551,9 @@ class TestProfile:
>>>
>>>          def test_matches(item):
>>>              path, test = item
>>> +            for f in self.filters:
>>> +                if not f(test):
>>> +                    return False
>>>              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))
>>> @@ -623,17 +627,10 @@ 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
>>>          """
>>> -        # 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..5190e20 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 x: not isinstance(x, GLSLParserTest))
>>>
>>>  # Drop ARB_vertex_program/ARB_fragment_program compiler tests.
>>>  del profile.tests['spec']['ARB_vertex_program']