On Tue, Apr 8, 2014 at 3:36 PM, Dylan Baker <[email protected]> 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 <[email protected]> > >> Cc: Dylan Baker <[email protected]> > >> Signed-off-by: Ilia Mirkin <[email protected]> > >> --- > >> > >> 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 <[email protected]> Awesome, thanks. _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
