[Piglit] [PATCH v2] framework: fix comparing NOTRUN/SKIP to other statuses

[Ilia Mirkin]

On Mon, Apr 7, 2014 at 4:58 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Mon, Apr 7, 2014 at 4:40 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
>> On Saturday, April 05, 2014 12:00:41 PM Ilia Mirkin wrote:
>>> The current logic causes transitions involving notrun/skip to not show
>>> up in problems. Remove the special handling of lt/gt/etc so that
>>> max(status) can work, and reorder the logic that figures out which
>>> category to select.
>>>
>>> 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>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>  - revert accidental logic change wrt "changes" page; disabled tests
>>> shouldn't show up on it.
>>>
>>>  framework/status.py  | 12 ------------
>>>  framework/summary.py | 14 +++++---------
>>>  2 files changed, 5 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/framework/status.py b/framework/status.py
>>> index bb7c594..d5e5f8e 100644
>>> --- a/framework/status.py
>>> +++ b/framework/status.py
>>> @@ -195,12 +195,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 +205,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):
>>
>> Sorry it took so long to get back to you, I missed the v2 on saturday and
>> don't work sundays.
>>
>> So these are also ending up on the changes page, but not correctly. I think
>> the output of the nose tests will make it clear:
>>
>> skip -> pass should not be a change ... ok
>> skip -> dmesg-warn should not be a change ... FAIL
>> skip -> warn should not be a change ... FAIL
>> skip -> dmesg-fail should not be a change ... FAIL
>> skip -> fail should not be a change ... FAIL
>> skip -> crash should not be a change ... FAIL
>> notrun -> pass should not be a change ... ok
>> notrun -> dmesg-warn should not be a change ... FAIL
>> notrun -> warn should not be a change ... FAIL
>> notrun -> dmesg-fail should not be a change ... FAIL
>> notrun -> fail should not be a change ... FAIL
>> notrun -> crash should not be a change ... FAIL
>
> Hm, OK. I kinda figured these all should be a change. I guess if I
> stare at the old code enough it'll become apparent to me why that
> wasn't the case. Anyways, I'll make the adjustments... ~tonight,
> probably.

OK, so now that I've taken a closer look at it... these tests are
wrong IMO. They're testing things like

a < b where a = 'skip' and b = not-'skip'

and expecting odd behaviour (which was due to the former
implementation). It's not actually testing what's ending up in the
various arrays by running the problematic code. I'm going to adjust
these tests, and add some more that make logical sense (like
max([skip, problem]) = problem, which is what was causing this issue).

I believe that the actual output of summary.py remains unchanged with
my proposed changes, except for fixing the issue where problems
weren't showing up on the problems page.

  -ilia