[Piglit] [PATCH] framework: Replace custom serialization format with json
Chad Versace
chad at chad-versace.us
Wed Jul 13 12:32:14 PDT 2011
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Paul, thank you for thoroughly reviewing this patch. You found many
mistakes and improvements.
I will resubmit a corrected patch this afternoon.
On 07/13/2011 11:20 AM, Paul Berry wrote:
> 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.
Before addressing your point, allow me point out a typo in the method signature.
``def make_tree(self, tests)`` should be ``def make_tree(cls, tests)``.
Observe that make_tree() never references self (or, after correction, cls). So, in
this code,
g = GroupResult()
# Do stuff with g, perhaps add subgroups
h = g.make_tree(tests)
h has no relation to g or any of its subgroups. To me, this indicates that make_tree()
should not be a member function.
> 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
>
I like this. Iteration here is *much* cleaner than recursion.
>>
>> - 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().
Noted. I'll fix that issue throughout the patch.
>
>>
>> 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).
This comment resulted in me finding a bug in my conversion. piglit-summary-html.py
*does* read globalkeys. Since TestrunResult.glxinfo has been moved to
TestrunResult.env.glxinfo, the test run's summary html lacks glxinfo's output.
Ditto for lspci.
>
> 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
Thanks for pointing this out. Explicitness here could have prevented my mishandling
of glxinfo and lspci.
I will use your suggestion here, and will return glxinfo and lspci to their original
locations (that is, to the toplevel of the TestrunResult dict).
>>
>>
>> #############################################################################
>> @@ -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?
It can die, and I will kill it in the resubmitted patch.
>
>>
>> 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"
Ok.
>
>> +
>> + :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:"
Sharp eyes.
>
>> 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".
Yes.
>
>> + 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)
Oops. That commented-out line should have been deleted.
I removed the check for ``type(name) != str`` because 1) after the json
conversion the TestResult keys were unicode, and 2) the check felt silly.
But you are right about the assertion. An assertion here is better than no
check at all.
>
>> 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:
Will fix.
>
> 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:
- --
Chad Versace
chad at chad-versace.us
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQIcBAEBAgAGBQJOHfK+AAoJEAIvNt057x8i4JcP/0ZiooBXxHwJzkFSOX2fmAS9
ZaRL8e/xtrfAqtULRhzW6qC/RLMfRsXCRbqFbQMZmr5tOGuhetxDhQMxPfdQOru6
fOc5LXmxzVX+enjacanduVRI0OfiGnH+umPOrJmYBaFPryCjheLETvhJClGzoN5Q
65Aem1EAJGesfqhHgHdqfadW9Rxfj0MkodAwxm4UjCCoAAkY0Hqjo3yF/wCOFwrd
vFmrKiwvTdcPtcvgBvy9W4hn2FSiJJyIpTI16LKYJONIzRyEUUjqhF+MjKqqyi1X
uo1OERMjrn5/DrQaLmoB+31c9c5KcI3zPxz2oOiKFI13L7ps5uyVfU9FFReeNgFX
6rsy0dnW37HhBrLwwgY+XJYExUu94roU16QMJGAEFs9ewmPlFdwUMG9hpa/lGsL8
1QvFodRoY83WfUmyRMp4qqhw2VKS0OY+hS+ecM6zxkl9JPnSLI/LsSsBxUfEOm5Z
/N75eH//6EMGMPDZeGI+7SirODYYx1u2MNsSdmEls6EzbGD/XGG/P/y28XyoRUah
1KoJLfr9w2Co6gbbx2d1Ov+8+9xmPiV33TOlgI5yXZF/xGhDUsPR8jst2ihHmWl3
LrcfIKT5i4f39C7//M2h1kQtgbz1rw3MzzGI4Pe21uMAlx6y7/CjuECM8cefOxVj
luiG0G0PWu8zzPVJ9beb
=rVgQ
-----END PGP SIGNATURE-----
More information about the Piglit
mailing list