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

[Ilia Mirkin]

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.

>
> This looks good and the one comment I have is just a question, so any answer
> is fine.
>
>
>
> Reviewed-by: Dylan Baker <baker.dylan.c at gmail.com>

Awesome, thanks.