[Piglit] [PATCH 5/7] status: Make use of the status objects
Kenneth Graunke
kenneth at whitecape.org
Mon Oct 14 08:29:02 CEST 2013
On 09/27/2013 02:39 PM, Dylan Baker wrote:
> This adds code in framework/summary.py and framework/core.py to make use
> of the status classes in status.py. This makes comparisons between
> statuses much simpler and cleaner
>
> v2: - Instead of importing all of the classes into the local namespace
> use the ``import ... as'' syntax. This means that if additional
> statuses are added they can easily be used without updating
> imports
> - Correct the sorting for fixes and regressions
> - Fix bug in TestResult that caused Fail, Warn, and Crash to be
> assigned the wrong status Objects
> - Adds support for dmesg-warn and dmesg-fail
>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
> framework/core.py | 29 ++++++++++++++--
> framework/summary.py | 97 ++++++++++++++--------------------------------------
> 2 files changed, 53 insertions(+), 73 deletions(-)
>
> diff --git a/framework/core.py b/framework/core.py
> index 09aebd4..9651e81 100644
> --- a/framework/core.py
> +++ b/framework/core.py
> @@ -1,4 +1,4 @@
> -#
> +
> # 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
> @@ -39,6 +39,7 @@ from textwrap import dedent
> from threads import ConcurrentTestPool
> from threads import synchronized_self
> import threading
> +import status
>
> __all__ = ['Environment',
> 'checkDir',
> @@ -218,7 +219,31 @@ if 'MESA_DEBUG' not in os.environ:
> os.environ['MESA_DEBUG'] = 'silent'
>
> class TestResult(dict):
> - pass
> + def __init__(self, *args):
> + dict.__init__(self, *args)
> +
> + # Replace the result with a status object
> + try:
> + if self['result'] == 'skip':
> + self['result'] = status.Skip()
> + elif self['result'] == 'pass':
> + self['result'] = status.Pass()
> + elif self['result'] == 'warn':
> + self['result'] = status.Warn()
> + elif self['result'] == 'crash':
> + self['result'] = status.Crash()
> + elif self['result'] == 'fail':
> + self['result'] = status.Fail()
> + elif self['result'] == 'dmesg-fail':
> + self['result'] = status.DmesgFail()
> + elif self['result'] == 'dmesg-warn':
> + self['result'] = status.DmesgWarn()
> + else:
> + raise KeyError
> + except:
> + # If there isn't a result (like when used py piglit-run), go on
> + # normally
> + pass
How about making a function that takes a string status name and returns
a status object? It could go in status.py, so it could be used anywhere
that deals with status objects.
You could also write it more succinctly as:
statusDict = {'skip': status.Skip,
'pass': status.Pass,
'warn': status.Warn,
'crash': status.Crash,
'fail': status.Fail,
'dmesg-fail': status.DmesgFail,
'dmesg-warn': status.DMesgWarn}
def statusFromString(statusName):
return statusDict[statusName]()
(or whatever you want to call it, and with proper style)
>
> class GroupResult(dict):
> diff --git a/framework/summary.py b/framework/summary.py
> index b579457..bab545d 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -28,6 +28,9 @@ from json import loads
> from mako.template import Template
>
> import core
> +# a local variable status exists, prevent accidental overloading by renaming
> +# the module
> +import status as so
>
> __all__ = [
> 'Summary',
> @@ -288,9 +291,9 @@ class Summary:
> def openGroup(name):
> stack.append((0, 0))
>
> - # Since skip is the "lowest" status for HTML generation, if
> + # Since NotRun is the "lowest" status for HTML generation, if
> # there is another status it will replace skip
> - currentStatus.append('skip')
> + currentStatus.append(so.NotRun())
>
> def closeGroup(group_name):
> # We're done with this group, record the number of pass/total
> @@ -304,36 +307,10 @@ class Summary:
> stack[-1] = (parent_pass + nr_pass, parent_total + nr_total)
>
> # Add the status back to the group hierarchy
> - if status_to_number(currentStatus[-2]) < \
> - status_to_number(currentStatus[-1]):
> + if currentStatus[-2] < currentStatus[-1]:
> currentStatus[-2] = currentStatus[-1]
> status[group_name] = currentStatus.pop()
>
> - def status_to_number(status):
> - """
> - like status_to_number in the constructor, this function
> - converts statuses into numbers so they can be comapared
> - logically/mathematically. The only difference between this and
> - init::status_to_number is the values assigned. The reason for
> - this is that here we are looking for the 'worst' status, while
> - in init::status_to_number we are looking for regressions in
> - status.
> - """
> - if status == 'skip':
> - return 1
> - elif status == 'pass':
> - return 2
> - elif status == 'dmesg-warn':
> - return 3
> - elif status == 'warn':
> - return 4
> - elif status == 'dmesg-fail':
> - return 5
> - elif status == 'fail':
> - return 6
> - elif status == 'crash':
> - return 7
> -
> openGroup('fake')
> openGroup('all')
>
> @@ -358,15 +335,14 @@ class Summary:
>
> # Add the current test
> (pass_so_far, total_so_far) = stack[-1]
> - if summary.tests[fulltest]['result'] == 'pass':
> + if summary.tests[fulltest]['result'] == so.Pass():
> pass_so_far += 1
> - if summary.tests[fulltest]['result'] != 'skip':
> + if summary.tests[fulltest]['result'] != so.Skip():
> total_so_far += 1
> stack[-1] = (pass_so_far, total_so_far)
>
> # compare the status
> - if status_to_number(summary.tests[fulltest]['result']) > \
> - status_to_number(currentStatus[-1]):
> + if summary.tests[fulltest]['result'] > currentStatus[-1]:
> currentStatus[-1] = summary.tests[fulltest]['result']
>
> # Work back up the stack closing groups as we go until we reach the
> @@ -415,58 +391,37 @@ class Summary:
> file is provided), and for JUnit and text which only need a limited
> subset of these lists
> """
> - def find_regressions(status):
> - """
> - Helper function to convert named statuses into number, since number
> - can more easily be compared using logical/mathematical operators.
> - The use of this is to look for regressions in status.
> - """
> - if status == 'pass':
> - return 1
> - elif status == 'dmesg-warn':
> - return 2
> - elif status == 'warn':
> - return 3
> - elif status == 'dmesg-fail':
> - return 4
> - elif status == 'fail':
> - return 5
> - elif status == 'crash':
> - return 6
> - elif status == 'skip':
> - return 7
> - elif status == 'special':
> - return 0
> -
> for test in self.tests['all']:
> status = []
> for each in self.results:
> try:
> - status.append(find_regressions(each.tests[test]['result']))
> + status.append(each.tests[test]['result'])
> except KeyError:
> - status.append(find_regressions("special"))
> + status.append(so.NotRun())
>
> if 'problems' in lists:
> - # Problems include: warn, dmes-warn, fail, dmesg-fail, and
> - # crash
> - if 7 > max(status) > 1:
> + # If that staus falls between Skip (The worst status) and
s/staus/status/, s/The worst/the worst/
> + # Pass (the best status) it's a problem
> + if so.Skip() > max(status) > so.Pass():
> self.tests['problems'].add(test)
>
> if 'skipped' in lists:
> # Find all tests with a status of skip
> - if 6 in status:
> + if so.Skip() in status:
> self.tests['skipped'].add(test)
>
> if 'fixes' in lists:
> - # Find both fixes and regressions, and append them to the
> - # proper lists
> + # find fixes, regressions, and changes
> for i in xrange(len(status) - 1):
> first = status[i]
> last = status[i + 1]
> - if first < last and 0 not in (first, last):
> + if first < last and so.NotRun() not in (first, last):
> self.tests['regressions'].add(test)
> - if first > last and 0 not in (first, last):
> + if first > last and so.NotRun() not in (first, last):
> self.tests['fixes'].add(test)
> + # Changes cannot be added in the fixes and regressions
> + # passes becasue NotRun is a change, but not a regression
s/becasue/because/
> + # or fix
> if first != last:
> self.tests['changes'].add(test)
>
> @@ -479,7 +434,7 @@ class Summary:
> 'dmesg-warn': 0, 'dmesg-fail': 0}
>
> for test in self.results[-1].tests.values():
> - self.totals[test['result']] += 1
> + self.totals[str(test['result'])] += 1
>
> def generateHTML(self, destination, exclude):
> """
> @@ -607,13 +562,13 @@ class Summary:
> if diff:
> for test in self.tests['changes']:
> print "%(test)s: %(statuses)s" % {'test': test, 'statuses':
> - ' '.join([i.tests.get(test, {'result': 'skip'})
> - ['result'] for i in self.results])}
> + ' '.join(map(str, [i.tests.get(test, {'result': so.Skip()})
> + ['result'] for i in self.results]))}
Hm, couldn't you just write this as:
' '.join([str(i.tests.get(test, {'result': so.Skip()})['result'] for i
in self.results])
Maybe I'm not reading things right.
> else:
> for test in self.tests['all']:
> print "%(test)s: %(statuses)s" % {'test': test, 'statuses':
> - ' '.join([i.tests.get(test, {'result': 'skip'})
> - ['result'] for i in self.results])}
> + ' '.join(map(str, [i.tests.get(test, {'result': so.Skip()})
> + ['result'] for i in self.results]))}
Ditto.
> # Print the summary
> print "summary:"
>
This is way nicer.
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
More information about the Piglit
mailing list