[Piglit] [PATCH] core: fix remove_tests logic

[Ilia Mirkin]

On Thu, Jan 16, 2014 at 7:26 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> Okay, I can see it your way, and I reasoned out my biggest conerns.
> I'm down to just one concern: We are already filtering in the modules using
> "del" on dictionaries in the profile. It seems like we really want to do all
> of our filtering in one place, so, do we move it all out of the modules into
> the profile internal logic, or do we keep it all where it is?

I actually really hate the 'del' thing too, but it does function, so I
didn't want to touch it in this change. I'd love to either change the
API for filter_tests to take the k,v or to create a filter_names
thing. For example the texelFetch tests don't end up in
[spec][glsl-1.x] but rather in [spec/glsl-1.x] (I forget the exact
details, but that's basically what it is). So trying to e.g. filter
out all the glsl stuff can be a pain (I was trying to create a gl1.x
profile for nouveau_vieux).

Then a del profile[foo][bar] becomes

profile.filter_names(lambda x: not x.startswith('foo/bar'))

Or we could provide a more targeted API, but there aren't a lot of
these profiles, and they don't try to del too much stuff, and having
an easy to understand mechanism seems more important than having
something that takes the absolute smallest number of characters.

>
> On Thursday, January 16, 2014 07:06:46 PM Ilia Mirkin wrote:
>> 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']