<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN" "http://www.w3.org/TR/REC-html40/strict.dtd"> <html><head><meta name="qrichtext" content="1" /><style type="text/css"> p, li { white-space: pre-wrap; } </style></head><body style=" font-family:'DejaVu Sans Mono'; font-size:9pt; font-weight:400; font-style:normal;"> 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@ijw.co.nz> > Cc: Dylan Baker <baker.dylan.c@gmail.com> > Signed-off-by: Ilia Mirkin <imirkin@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? > > -# all statuses except pass are problems > -PROBLEMS = STATUSES[1:] > +# The statuses that don't cause changes when transitioning from one another > +NO_OPS = ('skip', 'notrun') > > > def initialize_status(): > @@ -147,13 +152,20 @@ def test_not_change(): > """ Skip and NotRun should not count as changes """ > yieldable = check_not_change > > - for nochange, stat in itertools.product(['skip', 'notrun'], STATUSES): > + for nochange, stat in itertools.permutations(NO_OPS, 2): > yieldable.description = "{0} -> {1} should not be a change".format( > nochange, stat) > yield yieldable, status.status_lookup(nochange), > status.status_lookup(stat) > > > -def test_compare_statuses(): > - """ Compare NOTRUN -> PASS returns false """ > - nt.assert_equal(False, status.NOTRUN < status.PASS, > - msg="NOTRUN -> PASS returned True but should return > False") +def test_max_statuses(): > + """ Verify that max() works between skip and non-skip statuses """ > + for nochange, stat in itertools.product(NO_OPS, STATUSES): > + nochange = status.status_lookup(nochange) > + stat = status.status_lookup(stat) > + nt.assert_equal( > + stat, max(nochange, stat), > + msg="max({nochange}, {stat}) = {stat}".format(**locals())) > + nt.assert_equal( > + stat, max(stat, nochange), > + msg="max({stat}, {nochange}) = {stat}".format(**locals())) > diff --git a/framework/tests/summary_tests.py > b/framework/tests/summary_tests.py index d6856a9..1ad51b5 100644 > --- a/framework/tests/summary_tests.py > +++ b/framework/tests/summary_tests.py > @@ -48,7 +48,10 @@ def test_summary_add_to_set(): > ('skip', 'dmesg-warn', 'enabled'), > ('fail', 'notrun', 'disabled'), > ('crash', 'skip', 'disabled'), > - ('skip', 'skip', 'skipped')]: > + ('skip', 'skip', 'skipped'), > + ('notrun', 'fail', 'problems'), > + ('fail', 'notrun', 'problems'), > + ('pass', 'fail', 'problems')]: > check_sets.description = "{0} -> {1} should be added to > {2}".format( ostat, nstat, set_) 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@gmail.com></body></html>