[Piglit] [PATCH] core: fix remove_tests logic
Dylan Baker
baker.dylan.c at gmail.com
Thu Jan 16 16:42:55 PST 2014
I thinking working toward getting rid of the del thing is worthwhile at
another time.
On Thursday, January 16, 2014 07:31:30 PM Ilia Mirkin wrote:
> 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).
Unfortunately we have a group (including me) who think having the nested
dictionaries is stupid, and we should just have the / seperated keys. and a
group that still uses groups. so yeah, it's a mess right now.
>
> 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']
-------------- 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/c97895ff/attachment.pgp>
More information about the Piglit
mailing list