On Fri, Oct 30, 2015 at 05:08:50PM +0200, Gabriel Feceoru wrote:
> This adds a new "summary feature" command to piglit which creates a HTML table
> with the feature x DUT status (useful for multiple features, multiple DUTs).
> Another use case is the feature status for subsequent test results (depending
> on the meaning of that test result - DUT or build)
> A feature readiness is defined by the piglit regexp which selects the tests
> relevant for that feature and the acceptance percentage threshold (pass rate).
>
> It requires an input json file containing the list of features, in the
> following format (this is just an example):
> {
> "glsl" : {
> "include_tests" : "glsl",
> "exclude_tests" : "",
> "target_rate" : "90"
> },
> "arb" : {
> "include_tests" : "arb_gpu",
> "exclude_tests" : "",
> "target_rate" : "10"json does have integers, just don't quote them, although you'll still need to cast to int or assert later to avoid issues with being passed a string instead of an int. > } > > } > > v2: > Apply review comments (Dylan, Thomas) > > Signed-off-by: Gabriel Feceoru <[email protected]> > --- > framework/programs/summary.py | 36 +++++++++++++++ > framework/summary/__init__.py | 2 +- > framework/summary/feature.py | 105 > ++++++++++++++++++++++++++++++++++++++++++ > framework/summary/html_.py | 20 ++++++++ > piglit | 5 ++ > templates/feature.mako | 73 +++++++++++++++++++++++++++++ > 6 files changed, 240 insertions(+), 1 deletion(-) > create mode 100644 framework/summary/feature.py > create mode 100644 templates/feature.mako > > diff --git a/framework/programs/summary.py b/framework/programs/summary.py > index 8711ee5..b42cab4 100644 > --- a/framework/programs/summary.py > +++ b/framework/programs/summary.py > @@ -35,6 +35,7 @@ __all__ = [ > 'console', > 'csv', > 'html', > + 'feature' > ] > > > @@ -224,3 +225,38 @@ def aggregate(input_): > > print("Aggregated file written to: {}.{}".format( > outfile, backends.compression.get_mode())) Please ensure that all top level function and call definitions have two newlines between them > + > +#@exceptions.handler This shouldn't be commented. BTW, if you want to see the exceptions from the exception handler, set the environment PIGLIT_DEBUG=1. > +def feature(input_): > + parser = argparse.ArgumentParser() > + parser.add_argument("-o", "--overwrite", > + action="store_true", > + help="Overwrite existing directories") > + parser.add_argument("featureFile", > + metavar="<Feature json file>", > + help="Json file containing the features description") > + parser.add_argument("summaryDir", > + metavar="<Summary Directory>", > + help="Directory to put HTML files in") > + parser.add_argument("resultsFiles", > + metavar="<Results Files>", > + nargs="*", > + help="Results files to include in HTML") > + args = parser.parse_args(input_) > + > + # If args.list and args.resultsFiles are empty, then raise an error > + if not args.featureFile and not args.resultsFiles: > + raise parser.error("Missing required option -l or <resultsFiles>") > + > + # If args.list and args.resultsFiles are empty, then raise an error > + if not args.resultsFiles or not path.exists(args.featureFile): > + raise parser.error("Missing json file") > + > + # if overwrite is requested delete the output directory > + if path.exists(args.summaryDir) and args.overwrite: > + shutil.rmtree(args.summaryDir) > + > + # If the requested directory doesn't exist, create it or throw an error > + core.checkDir(args.summaryDir, not args.overwrite) > + > + summary.feat(args.resultsFiles, args.summaryDir, args.featureFile) > diff --git a/framework/summary/__init__.py b/framework/summary/__init__.py > index 8a1ff8a..0f0f144 100644 > --- a/framework/summary/__init__.py > +++ b/framework/summary/__init__.py > @@ -25,5 +25,5 @@ > # public parts here, so that we have a nice interface to work with. > > from __future__ import absolute_import, division, print_function > -from .html_ import html > +from .html_ import html, feat > from .console_ import console > diff --git a/framework/summary/feature.py b/framework/summary/feature.py > new file mode 100644 > index 0000000..aa52f89 > --- /dev/null > +++ b/framework/summary/feature.py > @@ -0,0 +1,105 @@ > +# Copyright (c) 2015 Intel Corporation > + > +# Permission is hereby granted, free of charge, to any person > +# obtaining a copy of this software and associated documentation > +# files (the "Software"), to deal in the Software without > +# restriction, including without limitation the rights to use, > +# copy, modify, merge, publish, distribute, sublicense, and/or > +# sell copies of the Software, and to permit persons to whom the > +# Software is furnished to do so, subject to the following > +# conditions: > +# > +# This permission notice shall be included in all copies or > +# substantial portions of the Software. > +# > +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY > +# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE > +# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR > +# PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHOR(S) BE > +# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN > +# AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF > +# OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > +# DEALINGS IN THE SOFTWARE. > + > +from __future__ import print_function, division, absolute_import > + > +import copy > +import collections > + > +try: > + import simplejson as json > +except ImportError: > + import json > + > +from framework import core, exceptions, profile, status > + > + > +class FeatResults(object): # pylint: disable=too-few-public-methods > + """Container object for results. > + > + Has the results, feature profiles and feature computed results. > + > + """ > + def __init__(self, results, json_file): > + > + with open(json_file) as data: > + self.feature_data = json.load(data) This doesn't need to be an instance variable > + > + self.feat_fractions = {} > + self.feat_status = {} > + self.profile = {} This or this > + self.features = set() > + self.results = results > + > + _profile_orig = > profile.load_test_profile(self.results[0].options['profile'][0]) I think you're going to want to handle results with more than one profile > + > + for feature in self.feature_data : The should be no space between the last letter and the ':'. There are a bunch of instances of this > + self.features.add(feature) > + > + include_filter = > self.feature_data[feature]["include_tests"].split() > + exclude_filter = > self.feature_data[feature]["exclude_tests"].split() I think this is going to lead to surprising behavior. Take this example: "ARB_shader|deqp.arb shader" passed as in include. what will be matched is "ARB_shader|deqp.arb|shader", which obviously is not what the original author intended, and I don't see a good way to fix it other than to specify that the include/exclude are regular expressions and the author is responsible for building their regular expressions as they like, and then passing a one element list into include/exclude_filter. > + > + opts = core.Options(include_filter = include_filter, > + exclude_filter = exclude_filter) The indenting is wrong here, please line up the 'i' in 'include' and the 'e' in 'exclude'. Also, as arguments to function calls '=' does not get spaces around it, ie: foo(a=1, b=2) > + > + _profile = copy.deepcopy(_profile_orig) > + > + # An empty list will raise PiglitFatalError exception > + # But for reporting we need to handle this situation > + try : > + _profile._prepare_test_list(opts) > + except exceptions.PiglitFatalError: > + pass > + self.profile[feature] = _profile Our style is that 2 blank lines are used only between top level classes and functions, just one blank inside of function bodies. > + > + > + for results in self.results : > + self.feat_fractions[results.name] = {} > + self.feat_status[results.name] = {} > + > + for feature in self.feature_data : > + # Create two dictionaries that have a default factory: they > return > + # a default value instead of a key error. > + # This default key must be callable > + self.feat_fractions[results.name][feature] = \ > + collections.defaultdict(lambda: (0, 0)) > + self.feat_status[results.name][feature] = \ > + collections.defaultdict(lambda: status.NOTRUN) I dont think you need this hunk (up to the opening of the for loop) > + > + result_set = set(results.tests) > + profile_set = set(self.profile[feature].test_list) > + > + common_set = profile_set & result_set > + passed_list = [x for x in common_set if > results.tests[x].result == status.PASS] > + > + total = len(common_set) > + passed = len(passed_list) > + > + self.feat_fractions[results.name][feature] = (passed, total) > + if total == 0: > + self.feat_status[results.name][feature] = status.NOTRUN > + else: > + if int(100 * passed / total) >= > int(self.feature_data[feature]["target_rate"]) : You could rewrite this line as if 100 * passed // total >= int(self.feature_data[feature]["target_rate"]) : // is floor division in python 3.x and python 2.x with the __future__ division import > + self.feat_status[results.name][feature] = status.PASS > + else : > + self.feat_status[results.name][feature] = status.FAIL > diff --git a/framework/summary/html_.py b/framework/summary/html_.py > index 12172b7..453c069 100644 > --- a/framework/summary/html_.py > +++ b/framework/summary/html_.py > @@ -37,9 +37,11 @@ from mako.lookup import TemplateLookup > from framework import backends, exceptions > > from .common import Results, escape_filename, escape_pathname > +from .feature import FeatResults > > __all__ = [ > 'html', > + 'feature' You have 'feature' here, but the function is called 'feat'. These need to match > ] > > _TEMP_DIR = os.path.join( > @@ -146,6 +148,14 @@ def _make_comparison_pages(results, destination, > exclude): > page=page, pages=pages)) > > > +def _make_feature_info(results, destination): > + """Create the feature readiness page.""" > + > + with open(os.path.join(destination, "feature.html"), 'w') as out: > + out.write(_TEMPLATES.get_template('feature.mako').render( > + results=results)) > + > + > def html(results, destination, exclude): > """ > Produce HTML summaries. > @@ -161,3 +171,13 @@ def html(results, destination, exclude): > _copy_static_files(destination) > _make_testrun_info(results, destination, exclude) > _make_comparison_pages(results, destination, exclude) > + > + > +def feat(results, destination, feat_desc): > + """Produce HTML feature readiness summary.""" > + > + feat_res = FeatResults([backends.load(i) for i in results], feat_desc) > + > + _copy_static_files(destination) > + _make_testrun_info(feat_res, destination, {}) I think we should just make the exclude argument to _make_testrun_info a keyword argument, cause this is ugly to me. Do something like this: _make_testrun_info(arg1, arg2, exclude=None): exclude = exclude or {} And yes, this odd thing is needed to avoid some bizarre python corner cases. > + _make_feature_info(feat_res, destination) > diff --git a/piglit b/piglit > index 5ae43e9..1cc1323 100755 > --- a/piglit > +++ b/piglit > @@ -106,6 +106,7 @@ def setup_module_search_path(): > setup_module_search_path() > import framework.programs.run as run > import framework.programs.summary as summary > +import framework.programs.feat_summary as feat_summary This is leftover from v1 and breaks piglit from working. > > > def main(): > @@ -139,6 +140,10 @@ def main(): > add_help=False, > help="Aggregate incomplete piglit > run.") > aggregate.set_defaults(func=summary.aggregate) > + feature = summary_parser.add_parser('feature', > + add_help=False, > + help="generate feature readiness > html report.") The indentation is wrong here, all of the lines should align with the opening ' of 'feature' > + feature.set_defaults(func=summary.feature) > > # Parse the known arguments (piglit run or piglit summary html for > # example), and then pass the arguments that this parser doesn't know > about > diff --git a/templates/feature.mako b/templates/feature.mako > new file mode 100644 > index 0000000..7178fa9 > --- /dev/null > +++ b/templates/feature.mako > @@ -0,0 +1,73 @@ > +<%! > + import posixpath # this must be posixpath, since we want /'s not \'s > + import re > + > + > + def feat_result(result): > + """Percentage result string""" > + return '{}/{}'.format(result[0], result[1]) > + > + > + def escape_filename(key): > + """Avoid reserved characters in filenames.""" > + return re.sub(r'[<>:"|?*#]', '_', key) > + > + > + def escape_pathname(key): > + """ Remove / and \\ from names """ > + return re.sub(r'[/\\]', '_', key) > + > + > + def normalize_href(href): > + """Force backward slashes in URLs.""" > + return href.replace('\\', '/') > +%> > + > +<?xml version="1.0" encoding="UTF-8"?> > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" > + "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd";> > +<html xmlns="http://www.w3.org/1999/xhtml";> > + <head> > + <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> > + <title>Result summary</title> > + <link rel="stylesheet" href="index.css" type="text/css" /> > + </head> > + <body> > + <h1>Feature readiness</h1> > + <table> > + <colgroup> > + ## Name Column > + <col /> > + > + ## Status columns > + ## Create an additional column for each summary > + % for _ in xrange(len(results.results)): > + <col /> > + % endfor > + </colgroup> > + <tr> > + <th/> > + % for res in results.results: > + <th class="head"><b>${res.name}</b><br />\ > + (<a > href="${normalize_href(posixpath.join(escape_pathname(res.name), > 'index.html'))}">info</a>)</th> > + % endfor > + </tr> > + % for feature in results.features: > + <tr> > + ## Add the left most column, the feature name > + <td> > + <div class="group" style="margin-left: ${0}em"> I think you can drop the style here, since you're not setting it to non-zero. > + <b>${feature}</b> > + </div> > + </td> > + ## add each group's totals > + % for res in results.results: > + <td class="${results.feat_status[res.name][feature]}"> > + <b>${feat_result(results.feat_fractions[res.name][feature])}</b> > + </td> > + % endfor > + </tr> > + % endfor > + </table> > + </body> > +</html> > -- > 1.9.1 > > _______________________________________________ > Piglit mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/piglit Overall this is looking much better than the first go around. I know there is a daunting amount of review here, but I think we have the design/re-use issues largely ironed out at this point and most of the remaining critique I have is stylistic or cleanups. Dylan
signature.asc
Description: PGP signature
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
