[Piglit] [PATCH 5/7] status: Make use of the status objects
Dylan Baker
baker.dylan.c at gmail.com
Mon Oct 14 17:11:32 CEST 2013
On Sunday, October 13, 2013 11:29:02 PM Kenneth Graunke wrote:
> 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)
I like this. I was searching for something more elgant than the long if/elif
statement and was drawing a blank.
>
> > 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/
This comment was replaced in an earlier patch fixup
>
> > + # 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/
fixed
>
> > + # 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.
You're close, but right in theory. Changed
>
> > 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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131014/d592487c/attachment-0001.pgp>
More information about the Piglit
mailing list