[Piglit] [PATCH] framework: Replace custom serialization format with json
Paul Berry
stereotype441 at gmail.com
Wed Jul 13 11:20:12 PDT 2011
Nice work, Chad. I like the direction you've taken things. I have a
bunch of minor comments below.
On 12 July 2011 16:56, Chad Versace <chad at chad-versace.us> wrote:
> The results file produced by piglit-run.py contains a serialized
> TestrunResult, where the serialization format is horridly homebrew. This
> commit replaces that insanity with json.
>
> Benefits:
> - Net loss of 164 lines of code.
> - By using the json module in the Python standard library, serializing
> and unserializing is nearly as simple as `json.dump(object, file)`
> and `json.load(object, file)`.
> - By using a format that is easy to format, it is now possible for
> users to embed custom data into the results file.
>
> As a side effect, the summary file is no longer needed, so it is no longer
> produced.
>
> CC: Kenneth Graunke <kenneth at whitecape.org>
> CC: Paul Berry <stereotype441 at gmail.com>
> Signed-off-by: Chad Versace <chad at chad-versace.us>
> ---
> framework/core.py | 277 ++++++++++++++--------------------------------
> framework/exectest.py | 2 +-
> framework/summary.py | 8 +-
> piglit-create-summary.py | 76 -------------
> piglit-merge-results.py | 4 +-
> piglit-run.py | 54 ++++++---
> piglit-summary-html.py | 9 +-
> 7 files changed, 135 insertions(+), 295 deletions(-)
> delete mode 100755 piglit-create-summary.py
>
> diff --git a/framework/core.py b/framework/core.py
> index ee56852..204b4e8 100644
> --- a/framework/core.py
> +++ b/framework/core.py
> @@ -24,6 +24,7 @@
> # Piglit core
>
> import errno
> +import json
> import os
> import platform
> import stat
> @@ -73,13 +74,6 @@ def checkDir(dirname, failifexists):
> if e.errno != errno.EEXIST:
> raise
>
> -# Encode a string
> -def encode(text):
> - return text.encode("string_escape")
> -
> -def decode(text):
> - return text.decode("string_escape")
> -
> if 'PIGLIT_BUILD_DIR' in os.environ:
> testBinDir = os.environ['PIGLIT_BUILD_DIR'] + '/bin/'
> else:
> @@ -91,175 +85,56 @@ else:
> #############################################################################
>
> class TestResult(dict):
> - def __init__(self, *args):
> - dict.__init__(self)
> -
> - assert(len(args) == 0 or len(args) == 2)
> -
> - if len(args) == 2:
> - for k in args[0]:
> - self.__setattr__(k, args[0][k])
> -
> - self.update(args[1])
> -
> - def __repr__(self):
> - attrnames = set(dir(self)) - set(dir(self.__class__()))
> - return '%(class)s(%(dir)s,%(dict)s)' % {
> - 'class': self.__class__.__name__,
> - 'dir': dict([(k, self.__getattribute__(k)) for k in attrnames]),
> - 'dict': dict.__repr__(self)
> - }
> -
> - def allTestResults(self, name):
> - return {name: self}
> -
> - def write(self, file, path):
> - result = StringIO()
> - print >> result, "@test: " + encode(path)
> - for k in self:
> - v = self[k]
> - if type(v) == list:
> - print >> result, k + "!"
> - for s in v:
> - print >> result, " " + encode(str(s))
> - print >> result, "!"
> - else:
> - print >> result, k + ": " + encode(str(v))
> - print >> result, "!"
> - file.write(result.getvalue())
> + pass
>
> class GroupResult(dict):
> - def __init__(self, *args):
> - dict.__init__(self)
> -
> - assert(len(args) == 0 or len(args) == 2)
> + @classmethod
> + def make_tree(self, tests):
> + def get_subgroup(group, path):
> + s = path.split('/')
>
> - if len(args) == 2:
> - for k in args[0]:
> - self.__setattr__(k, args[0][k])
> + if len(s) == 0:
> + return group
> + else:
> + subname = s[0]
> + if subname not in group:
> + group[subname] = GroupResult()
> + subgroup = group[subname]
>
> - self.update(args[1])
> + if len(s) == 1:
> + return subgroup
> + else:
> + subpath = '/'.join(s[1:])
> + return get_subgroup(subgroup, subpath)
Minor nit-picks about make_tree's nested function get_subgroup():
1. I think this function would make more sense as a regular member
function of GroupResult.
2. It seems like a loop would be clearer than recursion here. Did you
consider something like the following?
def get_subgroup(group, path):
for subname in path.split('/'):
if subname not in group:
group[subname] = GroupResult()
group = group[subname]
return group
>
> - def __repr__(self):
> - attrnames = set(dir(self)) - set(dir(self.__class__()))
> - return '%(class)s(%(dir)s,%(dict)s)' % {
> - 'class': self.__class__.__name__,
> - 'dir': dict([(k, self.__getattribute__(k)) for k in attrnames]),
> - 'dict': dict.__repr__(self)
> - }
> + root = GroupResult()
>
> - def allTestResults(self, groupName):
> - collection = {}
> - for name, sub in self.items():
> - subfullname = name
> - if len(groupName) > 0:
> - subfullname = groupName + '/' + subfullname
> - collection.update(sub.allTestResults(subfullname))
> - return collection
> + for (path, result) in tests.items():
> + group_path = os.path.dirname(path)
> + test_name = os.path.basename(path)
>
> - def write(self, file, groupName):
> - for name, sub in self.items():
> - subfullname = name
> - if len(groupName) > 0:
> - subfullname = groupName + '/' + subfullname
> - sub.write(file, subfullname)
> + group = get_subgroup(root, group_path)
> + group[test_name] = TestResult(result)
>
> + return root
>
> class TestrunResult:
> - def __init__(self, *args):
> - self.name = ''
> - self.globalkeys = ['name', 'href', 'glxinfo', 'lspci', 'time']
> - self.results = GroupResult()
> + globalkeys = ['name', 'href', 'glxinfo', 'lspci', 'time']
>
> - def allTestResults(self):
> - '''Return a dictionary containing (name: TestResult) mappings.
> - Note that writing to this dictionary has no effect.'''
> - return self.results.allTestResults('')
> + def __init__(self):
> + self.name = None
> + self.env = None
> + self.tests = dict()
The standard Python idiom is to use {} instead of dict().
>
> def write(self, file):
> - for key in self.globalkeys:
> - if key in self.__dict__:
> - print >>file, "%s: %s" % (key, encode(self.__dict__[key]))
> -
> - self.results.write(file,'')
> + json.dump(self.__dict__, file, indent=4)
>
> def parseFile(self, file):
> - def arrayparser(a):
> - def cb(line):
> - if line == '!':
> - del stack[-1]
> - else:
> - a.append(decode(line[1:]))
> - return cb
> -
> - def dictparser(d):
> - def cb(line):
> - if line == '!':
> - del stack[-1]
> - return
> -
> - colon = line.find(':')
> - if colon < 0:
> - excl = line.find('!')
> - if excl < 0:
> - raise Exception("Line %(linenr)d: Bad format" % locals())
> -
> - key = line[:excl]
> - d[key] = []
> - stack.append(arrayparser(d[key]))
> - return
> -
> - key = line[:colon]
> - value = decode(line[colon+2:])
> - d[key] = value
> - return cb
> -
> - def toplevel(line):
> - colon = line.find(':')
> - if colon < 0:
> - raise Exception("Line %(linenr)d: Bad format" % locals())
> -
> - key = line[:colon]
> - value = decode(line[colon+2:])
> - if key in self.globalkeys:
> - self.__dict__[key] = value
> - elif key == '@test':
> - comp = value.split('/')
> - group = self.results
> - for name in comp[:-1]:
> - if name not in group:
> - group[name] = GroupResult()
> - group = group[name]
> + self.__dict__ = json.load(file)
>
> - result = TestResult()
> - group[comp[-1]] = result
> -
> - stack.append(dictparser(result))
> - else:
> - raise Exception("Line %d: Unknown key %s" % (linenr, key))
> -
> - stack = [toplevel]
> - linenr = 1
> - for line in file:
> - if line[-1] == '\n':
> - stack[-1](line[0:-1])
> - linenr = linenr + 1
> -
> - def parseDir(self, path, PreferSummary):
> - main = None
> - filelist = [path + '/main', path + '/summary']
> - if PreferSummary:
> - filelist[:0] = [path + '/summary']
> - for filename in filelist:
> - try:
> - main = open(filename, 'U')
> - break
> - except:
> - pass
> - if not main:
> - raise Exception("Failed to open %(path)s" % locals())
> - self.parseFile(main)
> - main.close()
> + # Replace each raw dict in self.tests with a TestResult.
> + for (path, result) in self.tests.items():
> + self.tests[path] = TestResult(result)
Serializing self.__dict__ as JSON is a clever hack, but it has the
disadvantage that one can no longer tell by reading the definition of
TestrunResult what attributes are expected to be present in it. (I'm
guessing that you intended globalkeys to document this, but looks to
me like globalkeys is out of date, since it doesn't include 'env' or
'tests'. Also, since no code actually refers to globalkeys, it's
unlikely that people will keep it up to date in the future).
Would it be possible to make the serialization functions slightly more
verbose, in the interest of explicitness? I'm thinking of something
like this:
def write(self, file):
raw_dict = dict([(key, self.__dict__[key]) for key in self.globalkeys])
json.dump(raw_dict, file, indent=4)
def parseFile(self, file):
for key, value in json.load(file).items():
if key == 'tests':
# Replace each raw dict in value with a TestResult.
for (path, result) in value.items():
self.tests[path] = TestResult(result)
else:
assert key in self.globalkeys
self.__dict[key] = value
>
>
> #############################################################################
> @@ -283,11 +158,12 @@ class Environment:
> return stderr+stdout
>
> def collectData(self):
> + result = dict()
> if platform.system() != 'Windows':
> - self.file.write("glxinfo:", '@@@' + encode(self.run('glxinfo')), "\n")
> + result['glxinfo'] = self.run('glxinfo')
> if platform.system() == 'Linux':
> - self.file.write("lspci:", '@@@' + encode(self.run('lspci')), "\n")
> -
> + result['lspci'] = self.run('lspci')
> + return result
Can we get rid of Environment.file now, or is it used elsewhere?
>
> class Test:
> ignoreErrors = []
> @@ -304,13 +180,26 @@ class Test:
> def run(self):
> raise NotImplementedError
>
> - def doRun(self, env, path):
> + def doRun(self, env, path, testrun):
> + '''
> + Run test, or if running tests concurrently, schedule test to be ran
"to be run"
> +
> + :path:
> + Fully qualified test name as a string. For example,
> + ``spec/glsl-1.30/preprocessor/compiler/keywords/void.frag``.
> +
> + :testrun:
> + A TestrunResult object that accumulates test results.
> + After this test has executed, the test's ``TestResult`` is
> + assigned to ``testrun.tests[path]``
> + '''
> + args = (env, path, testrun)
> if self.runConcurrent:
> - ConcurrentTestPool().put(self.__doRunWork, args = (env, path,))
> + ConcurrentTestPool().put(self.__doRunWork, args=args)
> else:
> - self.__doRunWork(env, path)
> + self.__doRunWork(*args)
>
> - def __doRunWork(self, env, path):
> + def __doRunWork(self, env, path, testrun):
> # Exclude tests that don't match the filter regexp
> if len(env.filter) > 0:
> if not True in map(lambda f: f.search(path) != None, env.filter):
> @@ -343,11 +232,11 @@ class Test:
> result = TestResult()
> result['result'] = 'fail'
> result['exception'] = str(sys.exc_info()[0]) + str(sys.exc_info()[1])
> - result['traceback'] = '@@@' + "".join(traceback.format_tb(sys.exc_info()[2]))
> + result['traceback'] = "".join(traceback.format_tb(sys.exc_info()[2]))
>
> status(result['result'])
>
> - result.write(env.file, path)
> + testrun.tests[path] = result
> if Test.sleep:
> time.sleep(Test.sleep)
> else:
> @@ -379,12 +268,17 @@ class Test:
>
>
> class Group(dict):
> - def doRun(self, env, path):
> + def doRun(self, env, path, testrun):
> + '''
> + Schedule all tests in group for execution.
> +
> + See ``Test.doRun``.
> + '''
> for sub in sorted(self):
> spath = sub
> if len(path) > 0:
> spath = path + '/' + spath
> - self[sub].doRun(env, spath)
> + self[sub].doRun(env, spath, testrun)
>
>
> class TestProfile:
> @@ -392,12 +286,14 @@ class TestProfile:
> self.tests = Group()
> self.sleep = 0
>
> - def run(self, env):
> - time_start = time.time()
> - self.tests.doRun(env, '')
> + def run(self, env, testrun):
> + '''
> + Schedule all tests in profile for execution.
> +
> + See ``Test.doRun``.
> + '''
> + self.tests.doRun(env, '', testrun)
> ConcurrentTestPool().join()
> - time_end = time.time()
> - env.file.write("time:", time_end-time_start, "\n")
>
> def remove_test(self, test_path):
> """Remove a fully qualified test from the profile.
> @@ -428,24 +324,19 @@ def loadTestProfile(filename):
> traceback.print_exc()
> raise Exception('Could not read tests profile')
>
> -def loadTestResults(path, PreferSummary=False):
> +def loadTestResults(path):
> + if os.path.isdir(path):
> + filepath = os.path.join(path, 'main')
> + else:
> + filepath = path
> +
> + testrun = TestrunResult()
> try:
> - mode = os.stat(path)[stat.ST_MODE]
> - testrun = TestrunResult()
> - if stat.S_ISDIR(mode):
> - testrun.parseDir(path, PreferSummary)
> - else:
> - file = open(path, 'r')
> + with open(filepath, 'r') as file:
> testrun.parseFile(file)
> - file.close()
> -
> - if len(testrun.name) == 0:
> - if path[-1] == '/':
> - testrun.name = os.path.basename(path[0:-1])
> - else:
> - testrun.name = os.path.basename(path)
> -
> - return testrun
> - except:
> + except OSError as e:
Since you're not using e, this can just be "except OSError:"
> traceback.print_exc()
> raise Exception('Could not read tests results')
> +
> + assert(len(testrun.name) > 0)
Since you've changed TestrunResult to default name to None, I think
the assertion you want here is "assert testrun.name is not None".
> + return testrun
> diff --git a/framework/exectest.py b/framework/exectest.py
> index c80c156..e41cd21 100644
> --- a/framework/exectest.py
> +++ b/framework/exectest.py
> @@ -94,7 +94,7 @@ class PlainExecTest(Test):
>
> self.handleErr(results, err)
>
> - results['info'] = "@@@Returncode: %d\n\nErrors:\n%s\n\nOutput:\n%s" % (proc.returncode, err, out)
> + results['info'] = "Returncode: %d\n\nErrors:\n%s\n\nOutput:\n%s" % (proc.returncode, err, out)
> results['returncode'] = proc.returncode
> results['command'] = ' '.join(self.command)
> else:
> diff --git a/framework/summary.py b/framework/summary.py
> index c69fd2f..cac0254 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -156,7 +156,7 @@ results is an array of GroupResult instances, one per testrun
> childresults
> )
> else:
> - childresults = [r.get(name, core.TestResult({}, { 'result': 'skip' }))
> + childresults = [r.get(name, core.TestResult({ 'result': 'skip' }))
> for r in self.results]
>
> self.children[name] = TestSummary(
> @@ -188,8 +188,12 @@ class Summary:
> """\
> testruns is an array of TestrunResult instances
> """
> + groups = [
> + core.GroupResult.make_tree(testrun.tests)
> + for testrun in testruns
> + ]
> self.testruns = testruns
> - self.root = GroupSummary(self, '', 'All', [tr.results for tr in testruns])
> + self.root = GroupSummary(self, '', 'All', groups)
>
> def allTests(self):
> """\
> diff --git a/piglit-create-summary.py b/piglit-create-summary.py
> deleted file mode 100755
> index 914d09e..0000000
> --- a/piglit-create-summary.py
> +++ /dev/null
> @@ -1,76 +0,0 @@
> -#!/usr/bin/env python
> -#
> -# 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 getopt import getopt, GetoptError
> -import sys
> -
> -import framework.core as core
> -
> -
> -
> -#############################################################################
> -##### Main program
> -#############################################################################
> -def usage():
> - USAGE = """\
> -Usage: %(progName)s [options] [main results file]
> -
> -Options:
> - -h, --help Show this message
> -
> -Example:
> - %(progName)s results/main > results/summary
> -"""
> - print USAGE % {'progName': sys.argv[0]}
> - sys.exit(1)
> -
> -def main():
> - env = core.Environment()
> -
> - try:
> - options, args = getopt(sys.argv[1:], "h", [ "help" ])
> - except GetoptError:
> - usage()
> -
> - OptionName = ''
> -
> - for name, value in options:
> - if name in ('-h', '--help'):
> - usage()
> -
> - if len(args) != 1:
> - usage()
> -
> - resultsFilename = args[0]
> -
> - results = core.loadTestResults(resultsFilename)
> - for testname, result in results.allTestResults().items():
> - if 'info' in result:
> - if len(result['info']) > 4096:
> - result['info'] = result['info'][0:4096]
> - results.write(sys.stdout)
> -
> -
> -if __name__ == "__main__":
> - main()
> diff --git a/piglit-merge-results.py b/piglit-merge-results.py
> index 4095e48..eff8d5c 100755
> --- a/piglit-merge-results.py
> +++ b/piglit-merge-results.py
> @@ -68,8 +68,8 @@ def main():
> for resultsDir in args:
> results = core.loadTestResults(resultsDir)
>
> - for testname, result in results.allTestResults().items():
> - combined.results[testname] = result
> + for testname, result in results.tests.items():
> + combined.tests[testname] = result
>
> combined.write(sys.stdout)
>
> diff --git a/piglit-run.py b/piglit-run.py
> index 1e6d515..7d5725f 100755
> --- a/piglit-run.py
> +++ b/piglit-run.py
> @@ -23,8 +23,12 @@
>
>
> from getopt import getopt, GetoptError
> +import json
> +import os.path as path
> import re
> import sys, os
> +import time
> +import traceback
>
> import framework.core as core
> from framework.threads import synchronized_self
> @@ -120,27 +124,43 @@ def main():
>
> core.checkDir(resultsDir, False)
>
> + results = core.TestrunResult()
> +
> + # Set results.name
> + if OptionName is '':
> + results.name = path.basename(resultsDir)
> + else:
> + results.name = OptionName
> +
> + results.env = env.collectData()
> +
> profile = core.loadTestProfile(profileFilename)
> - env.file = SyncFileWriter(resultsDir + '/main')
> - env.file.writeLine("name: %(name)s" % { 'name': core.encode(OptionName) })
> - env.collectData()
> - profile.run(env)
> - env.file.close()
> -
> - print "Writing summary file..."
> - results = core.loadTestResults(resultsDir)
> - for testname, result in results.allTestResults().items():
> - if 'info' in result:
> - if len(result['info']) > 4096:
> - result['info'] = result['info'][0:4096]
> - file = open(resultsDir + '/summary', "w")
> - results.write(file)
> - file.close()
> + time_start = time.time()
> +
> + try:
> + profile.run(env, results)
> + except Exception as e:
> + if isinstance(e, KeyboardInterrupt):
> + # When the user interrupts the test run, he may still
> + # want the partial test results. So ignore
> + # KeyboardInterruption and proceed to writing the
> + # result files.
> + pass
> + else:
> + traceback.print_exc()
> + sys.exit(1)
> +
> + time_end = time.time()
> + results.time_elapsed = time_end - time_start
> +
> + result_filepath = os.path.join(resultsDir, 'main')
> + print("Writing results file...")
> + with open(result_filepath, 'w') as f:
> + results.write(f)
>
> print
> print 'Thank you for running Piglit!'
> - print 'Summary for submission has been written to ' + resultsDir + '/summary'
> -
> + print 'Results have been written to ' + result_filepath
>
> if __name__ == "__main__":
> main()
> diff --git a/piglit-summary-html.py b/piglit-summary-html.py
> index 4e94237..116c50f 100755
> --- a/piglit-summary-html.py
> +++ b/piglit-summary-html.py
> @@ -88,8 +88,8 @@ def buildDetailValue(detail):
> items = items + ResultListItem % { 'detail': buildDetailValue(d) }
>
> return ResultList % { 'items': items }
> - elif type(detail) == str and detail[0:3] == '@@@':
> - return ResultMString % { 'detail': cgi.escape(detail[3:]) }
> + elif isinstance(detail, str) or isinstance(detail, unicode):
> + return ResultMString % { 'detail': cgi.escape(detail) }
>
> return cgi.escape(str(detail))
>
> @@ -97,7 +97,8 @@ def buildDetailValue(detail):
> def buildDetails(testResult):
> details = []
> for name in testResult:
> - if type(name) != str or name == 'result':
> + #if type(name) != str or name == 'result':
> + if name == 'result':
I assume you commented out the "if type(name) != str" because name is
always a string? If that's the case, consider doing this instead of
the comment:
assert isinstance(name, basestring)
> continue
>
> value = buildDetailValue(testResult[name])
> @@ -281,7 +282,7 @@ def parse_listfile(filename):
> return eval(code)
>
> def loadresult(descr, OptionPreferSummary):
> - result = core.loadTestResults(descr[0], OptionPreferSummary)
> + result = core.loadTestResults(descr[0])
> if len(descr) > 1:
> result.__dict__.update(descr[1])
> return result
> --
> 1.7.6
>
>
One final comment: since TestResult is now effectively a synonym for
dict, I noticed you needed to clean up a lot of constructor calls to
pass just a single argument instead of two. It looks like you missed
one call site:
diff --git a/framework/core.py b/framework/core.py
index 204b4e8..6a6777d 100644
--- a/framework/core.py
+++ b/framework/core.py
@@ -225,7 +225,7 @@ class Test:
if 'result' not in result:
result['result'] = 'fail'
if not isinstance(result, TestResult):
- result = TestResult({}, result)
+ result = TestResult(result)
result['result'] = 'warn'
result['note'] = 'Result not
returned as an instance of TestResult'
except:
More information about the Piglit
mailing list