On Friday, January 17, 2014 12:33:11 AM Ilia Mirkin wrote: > On Fri, Jan 17, 2014 at 12:26 AM, Dylan Baker <[email protected]> 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 <[email protected]> > >> --- > >> > >> 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. >
If you don't know what it does it's really had to search for ;), and I
remember reading somewhere that upstream recomends limiting it's use to
situation where you can't know how many elements an argument will have.
> >> + 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? :)
we need to be 2.6 compliant, otherwise I'd suggest a dictionary comprehension.
>
> >> 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)
What about the _ place holder value? And a comment to mention you need that to
fit the ABI?
>
> >> # Drop ARB_vertex_program/ARB_fragment_program compiler tests.
> >> del profile.tests['spec']['ARB_vertex_program']
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
