[Piglit] [PATCH v2] Add support for feature readiness.
Dylan Baker
baker.dylan.c at gmail.com
Fri Oct 30 15:56:25 PDT 2015
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 <gabriel.feceoru at intel.com>
> ---
> 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
> Piglit at lists.freedesktop.org
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20151030/aa2db1de/attachment.sig>
More information about the Piglit
mailing list