[Piglit] [PATCH] core: fix remove_tests logic

[Ilia Mirkin]

Another way to look at it is:

Here are all the tests. Here is the profile's filter function (of
which there can be many). Here is the user's filter function.
test_matches is the user's filter function -- and like I said, I'd be
happy to just stick that on as a predicate to the filters list, and
then just have a Iterables.all() style mega-predicate function that
checks all the predicates in the list before queuing the test.

Alternatively, what do you propose?

  -ilia

On Thu, Jan 16, 2014 at 7:02 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> I'd like to maintain clear separation between the profile filtering the list,
> and the user filtering the list. Currently that happens because remove_test is
> called in gpu.py, and prepare_test_list() is called by piglit-<run|resume>.py.
>
> A profile, say gpu.py, says: "I'm quick.py, except, I don't have and
> glsl_parser tests, asmparsertest, ARB_vertex_program tests or
> ARB_fragment_program tests". When gpu.tests returns a profile with it's test
> list it should already have removed all of those tests from quick.py's profile
> instance, they don't belong to gpu.py.
>
> test_matches on the other hand is for the user to say, "I'm working on
> GL_hamsandwhich, and I only want to run those tests", or "I don't care about
> glean tests, don't run those"
>
> On Thursday, January 16, 2014 06:42:35 PM Ilia Mirkin wrote:
>> 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']