[v2] Add support for feature readiness.

Submitted by Feceoru, Gabriel on Oct. 30, 2015, 3:08 p.m.

Details

Message ID 1446217730-5706-1-git-send-email-gabriel.feceoru@intel.com
State New
Headers show
Series "Add support for feature readiness." ( rev: 2 ) in Piglit

Not browsing as part of any series.

Commit Message

Feceoru, Gabriel Oct. 30, 2015, 3:08 p.m.
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"
    }

}

v2:
Apply review comments (Dylan, Thomas)

Signed-off-by: Gabriel Feceoru <gabriel.feceoru@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

Patch hide | download patch | download mbox

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()))
+
+#@exceptions.handler
+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)
+
+        self.feat_fractions = {}
+        self.feat_status = {}
+        self.profile = {}
+        self.features = set()
+        self.results = results
+
+        _profile_orig = profile.load_test_profile(self.results[0].options['profile'][0])
+
+        for feature in self.feature_data :
+            self.features.add(feature)
+
+            include_filter = self.feature_data[feature]["include_tests"].split()
+            exclude_filter = self.feature_data[feature]["exclude_tests"].split()
+
+            opts = core.Options(include_filter = include_filter,
+                        exclude_filter = exclude_filter)
+
+            _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
+
+
+        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)
+
+                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"]) :
+                        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'
 ]
 
 _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, {})
+    _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
 
 
 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.")
+    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">
+            <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>

Comments

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@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@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