[Piglit] [PATCH v2 02/11] framework/summary.py: Adds new summary class and two support classes
Kenneth Graunke
kenneth at whitecape.org
Wed May 22 15:13:26 PDT 2013
On 05/17/2013 09:31 AM, Dylan Baker wrote:
> This new summary class (currently NewSummary), is a complete redesign of
> the summary generation system. Much of the code in summary was not
> commented, and also not clear as to what it was doing, making it very
> difficult to maintain and even more difficult to improve. The other
> problem with the code was architecturally it was clobbered together,
> doing some of the work in the summary.py file, and some of the work in
> the piglit-summary-html.py file.
>
> The new summary class does all of it's work in the summary.py file, and
> from an API point of view, in one class. The summary class uses it's
> constructor to build lists of passes, failures, skips, problems, etc.
> and then uses methods to generate output. The reference implementation
> of that kind of method is for generating HTML, but in theory it could be
> used to generate anything: XML, CSV lists, printed lists ala
> piglit-summary, etc.
>
> Also updates the .gitignore file to ignore some generated content
> created by this class.
>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
> .gitignore | 2 +
> framework/summary.py | 585 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 587 insertions(+)
>
> diff --git a/.gitignore b/.gitignore
> index 65ece5d..9180a6c 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -20,3 +20,5 @@ cmake_install.cmake
> CMakeCache.txt
> target_api
> tests/glslparsertest/glslcompiler
> +
> +.makotmp
> diff --git a/framework/summary.py b/framework/summary.py
> index 091329e..867dbad 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -20,8 +20,21 @@
> # OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> # DEALINGS IN THE SOFTWARE.
>
> +import os
> +import os.path as path
> +import string
> +from itertools import izip_longest
> +from shutil import copy
> +from json import loads
> +from mako.template import Template
> +
> import core
>
> +__all__ = [
> + 'Summary',
> + 'NewSummary',
> +]
> +
>
> class PassVector:
> """
> @@ -248,3 +261,575 @@ testruns is an array of TestrunResult instances
> Returns an array of all child TestSummary instances.
> """
> return self.root.allTests()
> +
> +## New Summary
> +
> +class Result:
> + """
> + Object that opens, reads, and stores the data in a resultfile.
> + """
> + def __init__(self, resultfile):
> +
> + # Load the json file, or if that fails assume that the locations given
> + # is a folder containing a json file
> + try:
> + result = loads(open(resultfile, 'r').read())
> + except IOError:
> + result = loads(open(path.join(resultfile, 'main'), 'r').read())
I like how succinct this code is. Unfortunately, it leaves the file
open...you should close it when you're done.
You'll probably want to do this as:
try:
with open(resultfile, 'r') as f:
result = loads(f.read())
except IOError:
with open(path.join(resultfile, 'main'), 'r') as f:
result = loads(f.read())
"with" opens the file, but automatically closes it at the end of the
indented block.
You can also omit the 'r' parameter if you like - that's the default.
> +
> + self.options = result['options']
> + self.glxinfo = result['glxinfo']
> + self.lspci = result['lspci']
> + self.time = result['time_elapsed']
You might want to do self.time = result.get('time_elapsed', '') in case
it doesn't exist. Sometimes we run summary on partially written files
to see work in progress, and time_elapsed won't exist yet.
> + self.name = result['name']
> + self.tests = result['tests']
> +
> +
> +class BuildHTMLIndex(list):
This is bizarre. In object oriented programming, class names are
virtually always nouns, not verbs. You create an object (instance) of a
particular class, and then do stuff with that object.
Furthermore, inheritance relationships can usually be expressed with the
word "is a". For example, "an Apple is a Fruit" or "a CheckingAccount
is a BankAccount." Saying "a BuildHTMLIndex is a list" makes no sense.
I originally was going to suggest that since the name you chose is a
verb and your constructor does a bunch of work and returns a value, that
perhaps this really ought to be a function (with nested helper
functions) that /returns a list/ rather than a class which /is a list/.
However, I think I see why you want to use a class now...you could
potentially have a subclass that implements something different for
_endRow() etc. to output a different kind of report.
Perhaps you could just drop "Build" from the name. "An HTMLIndex is a
List" makes sense.
> + """
> + Builds HTML output to be passed to the index mako template, which will be
> + rendered into HTML pages. It does this by parsing the lists provided by
> + the Summary object, and returns itself, an object with one accessor, a
> + list of html strings that will be printed by the mako template.
> + """
> +
> + def __init__(self, summary, page):
> + """
> + Steps through the list of groups and tests from all of the
> + results and generates a list of HTML items for groups and
> + tests.
> +
> + takes an argument for which page to generate, and follows
> + slightly different logic depending on which page it is
> + generating.
> + """
> +
> + def returnList(open, close):
> + """
> + As buildHTMLDepth iterates through the groups and tests
There is nothing called buildHTMLDepth in this patch. Presumably you
renamed the thing you're trying to refer to here.
It would also be nice to document what the "open" and "close" parameters
are, as it's not clear from reading the function body.
> + it uses this function to determine which groups to close
> + (and thus reduce the depth of the next write) and which
> + ones to open (thus increasing the depth)
> +
> + To that end one of two things happens, the path to the
> + previous group (close) and the next group (open) are equal,
> + in that event we don't want to open and close, becasue that
> + will result in a sawtooth pattern of a group with one test
> + followed by the same group with one test, over and over.
> + Instead we simply return two empty lists, which will result in
> + writing a long list of test results.
> + The second option is that the paths are different, and the
> + function determines any commonality between the paths, and
> + returns the differences as close (the groups which are
> + completly written) and open (the new groups to write).
> + """
> + common = []
> +
> + if open == close:
> + return [], []
> + else:
> + for i, j in izip_longest(open, close):
> + if i != j:
> + for k in common:
> + open.remove(k)
> + close.remove(k)
> + return open, close
> + else:
> + common.append(i)
> +
> + # set a starting depth of 2, 0 is used for 'all' so 1 is the
> + # next available group
> + depth = 1
> + # currentDir is the current working directory
> + currentDir = []
The current working directory is os.getcwd() and it is a string.
currentDir is a list, so clearly it is something else.
> +
> + # Add a new 'tab' for each result
> + self._newRow()
> + self.append({'type': 'other', 'text': '<td />'})
> + for each in summary.results:
> + self.append({'type': 'other',
> + 'text': '<td class="head"><b>%(name)s</b><br />'
> + '(<a href="%(href)s">info</a>)'
> + '</td>' % {'name': each.name,
> + 'href': path.join(each.name,
> + "index.html")}})
> + self._endRow()
> +
> + # Add the toplevel 'all' group
> + self._newRow()
> + self._groupRow("head", 0, 'all')
> + for each in summary.results:
> + self._groupResult(summary.fractions[each.name]['all'],
> + summary.status[each.name]['all'])
> + self._endRow()
> +
> + # Add the groups and tests to the out list
> + for key in sorted(page):
> +
> + # Split the group names and test names, then determine
> + # which groups to close and which to open
> + openList = key.split('/')
> + test = openList.pop()
> + openList, closeList = returnList(openList, list(currentDir))
> +
> + # Close any groups in the close list
> + # for each group closed, reduce the depth by one
> + for i in reversed(closeList):
> + currentDir.remove(i)
> + depth -= 1
> +
> + # Open new groups
> + for localGroup in openList:
> + self._newRow()
> +
> + # Add the left-most column: the name of the group
> + self._groupRow("head", depth, localGroup)
> +
> + # Add the group that we just opened to the currentDir,
> + # which will then be used to add that group to the
> + # HTML list. If there is a KeyError (the group doesn't
> + # exist), use (0, 0) which will result in skip
> + currentDir.append(localGroup)
> + for each in summary.results:
> + # Decide which fields need to be updated
> + try:
> + self._groupResult(
> + summary.fractions[each.name][path.join(*currentDir)],
> + summary.status[each.name][path.join(*currentDir)])
> + except KeyError:
> + self._groupResult((0, 0), 'skip')
> +
> + # After each group increase the depth by one
> + depth += 1
> + self._endRow()
> +
> + # Add the tests for the current group
> + self._newRow()
> +
> + # Add the left-most column: the name of the test
> + self._testRow("group", depth, test)
> +
> + # Add the result from each test result to the HTML summary
> + # If there is a KeyError (a result doesn't contain a
> + # particular test), return skip
> + for each in summary.results:
> + try:
> + self._testResult(each.name, key, each.tests[key]['result'])
> + except KeyError:
> + self.append({'type': 'other',
> + 'text': '<td class="skip">skip</td>'})
> + self._endRow()
> +
> + def _newRow(self):
> + self.append({'type': 'newRow'})
> +
> + def _endRow(self):
> + self.append({'type': 'endRow'})
> +
> + def _groupRow(self, cssclass, depth, groupname):
> + """
> + Helper function for appending new groups to be written out
> + in HTML.
> +
> + This particular function is used to write the left most
> + column of the summary. (the one with the indents)
> + """
> + self.append({'type': "groupRow",
> + 'class': cssclass,
> + 'indent': (1.75 * depth),
> + 'text': groupname})
> +
> + def _groupResult(self, value, css):
> + """
> + Helper function for appending the results of groups to the
> + HTML summary file.
> + """
> +
> + self.append({'type': "groupResult",
> + 'class': css,
> + 'text': "%s/%s" % (value[0], value[1])})
> +
> + def _testRow(self, cssclass, depth, groupname):
> + """
> + Helper function for appending new tests to be written out
> + in HTML.
> +
> + This particular function is used to write the left most
> + column of the summary. (the one with the indents)
> + """
> + self.append({'type': "testRow",
> + 'class': cssclass,
> + 'indent': (1.75 * depth),
> + 'text': groupname})
> +
> + def _testResult(self, group, href, text):
> + """
> + Helper function for writing the results of tests
> +
> + This function writes the cells other than the left-most cell,
> + displaying pass/fail/crash/etc and formatting the cell to the
> + correct color.
> + """
> + self.append({'type': 'testResult',
> + 'class': text,
> + 'href': path.join(self._sanitizePath(group),
> + self._sanitizePath(href + ".html")),
> + 'text': text})
> +
> + def _subtestResult(self, group, text):
> + """
> + Helper method for writin the results of subtests
> +
> + Subtests are slightly different than normal tests becasue they never
> + get a results summary page, only entries in the index.html and friends
> + pages, and _testResult cannot be cleanly expanded to handle subtests,
> + so Use this instead
> + """
> + self.append({'type': 'subtestResult',
> + 'class': text,
> + 'text': text})
> +
> + # This will be removed from here in a later patch
> + def _sanitizePath(self, path):
> + """
> + Private: convert the testname to only contain valid characters for
> + a path
> +
> + This attempts to faithfully reconstruct the output of
> + pigli-summary-html.testPathToHtmlFilename, but without the use of
> + lambda. The advantage to not using lambda is readability, and the
> + desire to make future changes to the way that sub-tests are written
> + """
> + valid_chars = frozenset("_-.%s%s" % (string.ascii_letters,
> + string.digits))
> + out = []
> + for c in path:
> + if c in valid_chars:
> + out.append(c)
> + elif c == "/":
> + out.append("__")
> + else:
> + out.append("_")
> + return ''.join(out)
This is totally duplicated with the other _sanitizePath method at the
end of your patch. It also doesn't use self at all, so I would just
make it a helper function at the top level outside of any classes.
> +
> +
> +class NewSummary:
> + """
> + This Summary class creates an initial object containing lists of tests
> + including all, changes, problems, skips, regressions, and fixes. It then
> + uses methods to generate various kinds of output. The reference
> + implimentation is HTML output through mako, aptly named generateHTML().
"implementation" (typo)
> + """
> +
> + def __init__(self, resultfiles):
> + """
> + Create an initial object with all of the result information rolled up
> + in an easy to process form.
> +
> + The constructor of the summary class has an attribute for each HTML
> + summary page, which are fed into the index.mako file to produce HTML
> + files
> + """
> +
> + def appendResult(status):
appendResult is a weird name for this function, as it doesn't actually
involve appending things at all. It converts a status string into a
numerical value.
> + """
> + smaller helper function to convert named stati into number, since
Smaller than what? I'd just say "Helper function". Also, the plural
of status is "statuses":
http://www.merriam-webster.com/dictionary/status
> + number can more easily be compared using logical/mathimatical
"mathematical" (typo)
> + operators
> + """
> + if status == 'pass':
> + return 1
> + elif status == 'warn':
> + return 2
> + elif status == 'fail':
> + return 3
> + elif status == 'skip':
> + return 4
> + elif status == 'crash':
> + return 5
This also looks redundant with statusCompare below, except that the
numerical values you assign are different. I'm not sure why.
> + # Create a Result object for each piglit result and append it to the
> + # results list
> + self.results = [Result(i) for i in resultfiles]
> +
> + self.status = {}
> + self.fractions = {}
> + self.alltests = []
> + self.changes = []
> + self.problems = []
> + self.skipped = []
> + self.regressions = []
> + self.fixes = []
> +
> + for each in self.results:
> + # Build a dict of the status output of all of the tests, with the
> + # name of the test run as the key for that result, this will be
> + # used to write pull the statuses later
> + fraction, status = self._buildDictionary(each)
> + self.fractions.update({each.name: fraction})
> + self.status.update({each.name: status})
> +
> + # Create a list with all the test names in it
> + self.alltests = list(set(self.alltests) | set(each.tests))
> +
> + # Create lists similar to self.alltests, but for the other root pages,
> + # (regressions, skips, ect)
> + for test in self.alltests:
> + status = []
> + for each in self.results:
> + try:
> + status.append(appendResult(each.tests[test]['result']))
> + except KeyError:
> + status.append(appendResult("skip"))
> +
> + # Check and append self.changes
> + # A set cannot contain duplicate entries, so creating a set out
> + # the list will reduce it's length to 1 if all entries are the
> + # same, meaning it is not a change
> + if len(set(status)) > 1:
> + self.changes.append(test)
> +
> + # Problems
> + # If the result contains a value other than 1 (pass) or 4 (skip)
> + # it is a problem. Skips are not problems becasuse they have
> + # Their own page.
> + if [i for e in [2, 3, 5] for i in status if e is i]:
> + self.problems.append(test)
> +
> + # skipped
> + if 4 in status:
> + self.skipped.append(test)
> +
> + # fixes and regressions
> + # check each member against the next member. If the second member
> + # has a greater value it is a regression.
> + # Fixes on the other hand are a former non 1 value, followed by
> + # a value of 1
> + for i in xrange(len(status) - 1):
> + if status[i] < status[i + 1]:
> + self.regressions.append(test)
> + if status[i] > 1 and status[i + 1] == 1:
> + self.fixes.append(test)
> +
> + def generateHTML(self, destination):
> + """
> + Produce HTML summaries.
> +
> + Basically all this does is takes the information provided by the
> + constructor, and passes it to mako templates to generate HTML files.
> + The beauty of this approach is that mako is leveraged to do the
> + heavy lifting, this method just passes it a bunch of dicts and lists
> + of dicts, which mako turns into pretty HTML.
> + """
> + # Copy static files
> + copy("templates/index.css", path.join(destination, "index.css"))
> + copy("templates/result.css", path.join(destination, "result.css"))
> +
> + # Create the mako object for creating the test/index.html file
> + testindex = Template(filename = "templates/test_index.mako",
> + output_encoding = "utf-8",
> + module_directory = ".makotmp")
> +
> + # Create the mako object for the individual result files
> + testfile = Template(filename = "templates/test_result.mako",
> + output_encoding = "utf-8",
> + module_directory = ".makotmp")
> +
> + # Iterate across the tests creating the various test specific files
> + for each in self.results:
> + os.mkdir(path.join(destination, each.name))
> +
> + file = open(path.join(destination, each.name, "index.html"), 'w')
> + file.write(testindex.render(name = each.name,
> + time = each.time,
> + options = each.options,
> + glxinfo = each.glxinfo,
> + lspci = each.lspci))
> + file.close()
> +
> + # Then build the individual test results
> + for key, value in each.tests.iteritems():
> + file = open(path.join(destination,
> + each.name,
> + self._sanitizePath(key + ".html")), 'w')
> + file.write(testfile.render(testname = key,
> + status = value['result'],
> + returncode = value['returncode'],
> + time = value['time'],
> + info = value['info'],
> + command = value['command']))
> + file.close()
> +
> + # Finally build the root html files: index, regressions, etc
> + index = Template(filename = "templates/index.mako",
> + output_encoding = "utf-8",
> + module_directory = ".makotmp")
> +
> + # Index.html
> + file = open(path.join(destination, "index.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self, self.alltests),
> + page = 'all',
> + colnum = len(self.results)))
> + file.close()
I would just do this as:
with open(path.join(destination, 'index.html'), 'w') as file:
file.write(index.render(...))
Then it automatically gets closed without needing to do file.close()
explicitly. Ditto for the rest of these.
> +
> + # changes.html
> + file = open(path.join(destination, "changes.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self, self.changes),
> + page = 'changes',
> + colnum = len(self.results)))
> + file.close()
> +
> + # problems.html
> + file = open(path.join(destination, "problems.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self, self.problems),
> + page = 'problems',
> + colnum = len(self.results)))
> + file.close()
> +
> + # skipped.html
> + file = open(path.join(destination, "skipped.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self, self.skipped),
> + page = 'skipped',
> + colnum = len(self.results)))
> + file.close()
> +
> + # fixes.html
> + file = open(path.join(destination, "fixes.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self, self.fixes),
> + page = 'fixes',
> + colnum = len(self.results)))
> + file.close()
> +
> + # regressions.html
> + file = open(path.join(destination, "regressions.html"), 'w')
> + file.write(index.render(results = BuildHTMLIndex(self,
> + self.regressions),
> + page = 'regressions',
> + colnum = len(self.results)))
> + file.close()
> +
> + def _buildDictionary(self, summary):
> + # Build a dictionary from test name to pass count/total count, i.e.
> + # counts['spec/glsl/foo'] == (456, 800)
> + counts = {}
> +
> + if not summary.tests:
> + return {}
> +
> + lastGroup = ''
> +
> + # Build a dictionary of group stati, passing groupname = status.
statuses
> + # This is the "worst" status of the group in descending order: crash,
> + # skip, fail, warn, pass
> + status = {}
> +
> + # currentStack is a stack containing numerical values that that relate
> + # to a status output, 5 for crash, 4 for skip, 3 for fail, 2 for warn,
> + # 1 for pass
> + currentStatus = []
> +
> + # Stack contains tuples like: (pass count, total count)
> + stack = []
> +
> + def openGroup(name):
> + stack.append((0, 0))
> +
> + # Since skip is the "lowest" status for HTML generation, if there
> + # is another status it will replace skip
> + currentStatus.append('skip')
> +
> + def closeGroup(group_name):
> + # We're done with this group, record the number of pass/total in
> + # the dictionary.
> + (nr_pass, nr_total) = stack.pop()
> + counts[group_name] = (nr_pass, nr_total)
> +
> + # Also add our pass/total count to the parent group's counts
> + # (which are still being computed)
> + (parent_pass, parent_total) = stack[-1]
> + stack[-1] = (parent_pass + nr_pass, parent_total + nr_total)
> +
> + # Add the status back to the group heirarchy
hierarchy (typo)
> + if statusCompare(currentStatus[-2]) < \
> + statusCompare(currentStatus[-1]):
> + currentStatus[-2] = currentStatus[-1]
> + status[group_name] = currentStatus.pop()
> +
> + def statusCompare(status):
The name of this is strange. A function called "compare" usually takes
two or more things and compares them. This just turns a status
enumeration into an integer value.
> + if status == 'skip':
> + return 1
> + elif status == 'pass':
> + return 2
> + elif status == 'warn':
> + return 3
> + elif status == 'fail':
> + return 4
> + elif status == 'crash':
> + return 5
> +
> + openGroup('fake')
> + openGroup('all')
> +
> + # fulltest is a full test name,
> + # i.e. tests/shaders/glsl-algebraic-add-zero
> + for fulltest in sorted(summary.tests):
> + group, test = path.split(fulltest) # or fulltest.rpartition('/')
> +
> + if group != lastGroup:
> + # We're in a different group now. Close the old ones
> + # and open the new ones.
> + for x in path.relpath(group, lastGroup).split('/'):
> + if x != '..':
> + openGroup(x)
> + else:
> + closeGroup(lastGroup)
> + lastGroup = path.normpath(path.join(lastGroup, ".."))
> +
> + lastGroup = group
> +
> + # Add the current test
> + (pass_so_far, total_so_far) = stack[-1]
> + if summary.tests[fulltest]['result'] == 'pass':
> + pass_so_far += 1
> + if summary.tests[fulltest]['result'] != 'skip':
> + total_so_far += 1
> + stack[-1] = (pass_so_far, total_so_far)
> +
> + # compare the status
> + if statusCompare(summary.tests[fulltest]['result']) > \
> + statusCompare(currentStatus[-1]):
> + currentStatus[-1] = summary.tests[fulltest]['result']
> +
> + # Work back up the stack closing groups as we go until we reach the
> + # top, where we need to manually close this as "all"
> + while len(stack) > 2:
> + closeGroup(lastGroup)
> + lastGroup = path.dirname(lastGroup)
> + closeGroup("all")
> +
> + assert(len(stack) == 1)
> + assert(len(currentStatus) == 1)
> +
> + return counts, status
> +
> + def _sanitizePath(self, path):
> + """
> + Private: convert the testname to only contain valid characters for
> + a path
> +
> + This attempts to faithfully reconstruct the output of
> + pigli-summary-html.testPathToHtmlFilename, but without the use of
> + lambda. The advantage to not using lambda is readability, and the
> + desire to make future changes to the way that sub-tests are written
> + """
> + valid_chars = frozenset("_-.%s%s" % (string.ascii_letters,
> + string.digits))
> + out = []
> + for c in path:
> + if c in valid_chars:
> + out.append(c)
> + elif c == "/":
> + out.append("__")
> + else:
> + out.append("_")
> + return ''.join(out)
Whoa. You just replaced a one line function with 10x the code including
sets, loops, and control flow. This is /significantly/ less readable.
You could perhaps do:
return ''.join(c for c in path.replace('/', '__') if c.isalnum() or c ==
'_')
Or use a regular expression substitution. Or just use the original code.
More information about the Piglit
mailing list