[Piglit] [PATCH v3 1/2] framework: fix comparing NOTRUN/SKIP to other statuses

Tue Apr 8 13:50:30 PDT 2014

On Tue, Apr 8, 2014 at 4:08 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> On Tuesday, April 08, 2014 16:02:49 Ilia Mirkin wrote:
>
>> On Tue, Apr 8, 2014 at 3:53 PM, Dylan Baker <baker.dylan.c at gmail.com>
>> wrote:
>
>> > On Tuesday, April 08, 2014 15:44:54 Ilia Mirkin wrote:
>
>> >> On Tue, Apr 8, 2014 at 3:36 PM, Dylan Baker <baker.dylan.c at gmail.com>
>
>> >>
>
>> >> wrote:
>
>> >> > On Monday, April 07, 2014 22:51:27 Ilia Mirkin wrote:
>
>> >> >> The current logic causes transitions involving notrun/skip to not
>> >> >> show
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> up in problems. It also encodes display decisions in lt/gt/etc
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> operators. This change adjusts the status class to be logically
>
>> >> >> simpler
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> by removing the special handling of lt/gt/etc so that max(status)
>> >> >> can
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> work, and reorders the logic that figures out which category to
>
>> >> >> select.
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> Tests are updated to encode the new order -- notrun/skip are now
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> comparable to (and less than) the other statuses, but are unordered
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> between one another.
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> Cc: Chris Forbes <chrisf at ijw.co.nz>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> Cc: Dylan Baker <baker.dylan.c at gmail.com>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> ---
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> v2 -> v3:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - Modify tests to pass, add some more that exemplify the desired
>
>> >> >>
>
>> >> >> behaviour
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> framework/status.py | 23 ++++++-----------------
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> framework/summary.py | 14 +++++---------
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> framework/tests/status_tests.py | 30 +++++++++++++++++++++---------
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> framework/tests/summary_tests.py | 5 ++++-
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> 4 files changed, 36 insertions(+), 36 deletions(-)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> diff --git a/framework/status.py b/framework/status.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> index bb7c594..aea3650 100644
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> --- a/framework/status.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +++ b/framework/status.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> @@ -158,10 +158,10 @@ class Status(object):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> return unicode(self.name)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __lt__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return int(self) < int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + return not self.__ge__(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __le__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return int(self) <= int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + return not self.__gt__(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __eq__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> # This must be int or status, since status comparisons are done
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> using @@ -173,13 +173,14 @@ class Status(object):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> raise TypeError("Cannot compare type: {}".format(type(other)))
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __ne__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return int(self) != int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + return self.fraction != other.fraction or int(self) != int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __ge__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return int(self) >= int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + return self.fraction[1] > other.fraction[1] or (
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + self.fraction[1] == other.fraction[1] and int(self) >=
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> int(other))
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __gt__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return int(self) > int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + return self.fraction[1] > other.fraction[1] or int(self) >
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> int(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __int__(self):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> return self.value
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> @@ -195,12 +196,6 @@ class NoChangeStatus(Status):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __init__(self, name, value=0, fraction=(0, 0)):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> super(NoChangeStatus, self).__init__(name, value, fraction)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - def __lt__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return False
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - def __le__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return False
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __eq__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> if isinstance(other, (str, unicode, Status)):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> return unicode(self) == unicode(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> @@ -211,12 +206,6 @@ class NoChangeStatus(Status):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> return unicode(self) != unicode(other)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> raise TypeError("Cannot compare type: {}".format(type(other)))
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - def __ge__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return False
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - def __gt__(self, other):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - return False
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> NOTRUN = NoChangeStatus('Not Run')
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> diff --git a/framework/summary.py b/framework/summary.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> index 6d2a7bb..a4aa136 100644
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> --- a/framework/summary.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +++ b/framework/summary.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> @@ -325,20 +325,16 @@ class Summary:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> for i in xrange(len(status) - 1):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> first = status[i]
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> last = status[i + 1]
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - if first < last:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - self.tests['regressions'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - self.tests['changes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - continue
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - elif first > last:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - self.tests['fixes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - self.tests['changes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> - continue
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> if first in [so.SKIP, so.NOTRUN] and last not in [so.SKIP,
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> so.NOTRUN]: self.tests['enabled'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> self.tests['changes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> elif last in [so.SKIP, so.NOTRUN] and first not in
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> [so.SKIP, so.NOTRUN]: self.tests['disabled'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + elif first < last:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + self.tests['regressions'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + self.tests['changes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + elif first > last:
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + self.tests['fixes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> self.tests['changes'].add(test)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> def __find_totals(self):
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> diff --git a/framework/tests/status_tests.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> b/framework/tests/status_tests.py index ad0630a..ec6bf18 100644
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> --- a/framework/tests/status_tests.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +++ b/framework/tests/status_tests.py
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> @@ -33,13 +33,18 @@ import framework.status as status
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> # tested separately because of upcoming features for it
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> STATUSES = ["pass", "dmesg-warn", "warn", "dmesg-fail", "fail",
>
>> >> >> "crash"]
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +# all statuses except pass are problems
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +PROBLEMS = STATUSES[1:]
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> # Create lists of fixes and regressions programmatically based on
>> >> >> the
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> STATUSES # list. This means less code, and easier expansion changes.
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -REGRESSIONS = itertools.combinations(STATUSES, 2)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> -FIXES = itertools.combinations(reversed(STATUSES), 2)
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +REGRESSIONS = list(itertools.combinations(STATUSES, 2)) + \
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + list(itertools.combinations(["skip"] + PROBLEMS, 2))
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> +FIXES = list(itertools.combinations(reversed(STATUSES), 2)) + \
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> + list(itertools.combinations(list(reversed(PROBLEMS)) + ["skip"],
>
>> >> >>
>
>> >> >>
>
>> >> >>
>
>> >> >> 2))
>
>> >> >
>
>> >> > is there a reason to list the second part twice?
>
>> >>
>
>> >> Not sure which second part you're talking about... just making sure
>
>> >>
>
>> >> that transitions from skip -> warn counts as a "problem" and warn ->
>
>> >>
>
>> >> skip counts as a "fix" at least as far as lt and gt are concerned. The
>
>> >>
>
>> >> actual policy of what to show on the various pages is left in
>
>> >>
>
>> >> summary.py.
>
>> >
>
>> > list(itertools.combinations(list(reversed(PROBLEMS)) + ["skip"]
>
>>
>
>> Oh, can itertools.combinations take an iter? I'll check. I just
>
>> assumed it couldn't. [Hm, it clearly can, based on the code 1 line
>
>> above it. Oops. I'll change that before pushing.]
>
>>
>
>> -ilia
>
>
>
> I would find it very amusing if itertools functions couldn't take iterators
> as arguments.

Oh, right, of course.... because of the + ["skip"]. Could probably use
some fancy function to deal with it, but this is easier, readable, and
not too performance-sensitive.

  -ilia