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.
> + 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)])
>
> 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"
> - # 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?
>
> # 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
