[Piglit] [PATCH 1/2] summary: fix regression and fixes summaries
Marek Olšák
maraeo at gmail.com
Wed Nov 20 15:30:53 PST 2013
On Thu, Nov 21, 2013 at 12:04 AM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> On Monday, November 18, 2013 03:33:33 PM Marek Olšák wrote:
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> Somebody broke this.
>> ---
>> framework/status.py | 48 +++++++++++++--------------------
>> framework/summary.py | 6 ++---
>> framework/tests/summary.py | 66
>> ++++++++++++++++------------------------------ 3 files changed, 44
>> insertions(+), 76 deletions(-)
>>
>> diff --git a/framework/status.py b/framework/status.py
>> index 3a9e2d3..c8f7375 100644
>> --- a/framework/status.py
>> +++ b/framework/status.py
>> @@ -21,6 +21,8 @@
>>
>> """ Status ordering from best to worst:
>>
>> +NotRun
>> +skip
>> pass
>> dmesg-warn
>> warn
>> @@ -28,33 +30,21 @@ dmesg-fail
>> fail
>> crash
>> timeout
>> -skip
>> -
>> -
>> -The following are regressions:
>> -
>> -pass|warn|dmesg-warn|fail|dmesg-fail|crash|timeout -> skip
>> -pass|warn|dmesg-warn|fail|dmesg-fail|crash -> timeout|skip
>> -pass|warn|dmesg-warn|fail|dmesg-fail -> crash|timeout|skip
>> -pass|warn|dmesg-warn|fail -> dmesg-fail|crash|timeout|skip
>> -pass|warn|dmesg-warn -> fail|dmesg-fail|crash|timeout|skip
>> -pass|warn -> dmesg-warn|fail|dmesg-fail|crash|timeout|skip
>> -pass -> warn|dmesg-warn|fail|dmesg-fail|crash|timeout|skip
>>
>> +(NotRun, pass, skip) are considered equivalent for regression testing.
>>
>> -The following are fixes:
>> +The motivation is if you accidentally expose a feature that doesn't work,
>> +you'll get skip->fail, which is a regression. If you disable the feature,
>> +you'll get fail->skip, which is a fix.
>>
>> -skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash|timeout
>> -timeout|skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash
>> -crash|timeout|skip - >pass|warn|dmesg-warn|fail|dmesg-fail
>> -dmesg-fail|crash|timeout|skip -> pass|warn|dmesg-warn|fail
>> -fail|dmesg-fail|crash|timeout|skip -> pass|warn|dmesg-warn
>> -dmesg-warn|fail|dmesg-fail|crash|timeout|skip -> pass|warn
>> -warn|dmesg-warn|fail|dmesg-fail|crash|timeout|skip -> pass
>> +NotRun->fail should also be considered a regression for you not to miss
>> +new failing tests.
>>
>> +The formula for determining regressions is:
>> + max(old_status, pass) < new_status
>>
>> -NotRun -> * and * -> NotRun is a change, but not a fix or a regression.
>> This is -because NotRun is not a status, but a representation of an unknown
>> status. +The formula for determining fixes is:
>> + old_status > max(new_status, pass)
>>
>> """
>>
>> @@ -159,6 +149,13 @@ class NotRun(Status):
>> def __init__(self):
>> pass
>>
>> +class Skip(Status):
>> + name = 'skip'
>> + value = 5
>> + fraction = (0, 0)
>> +
>> + def __init__(self):
>> + pass
>>
>> class Pass(Status):
>> name = 'pass'
>> @@ -217,10 +214,3 @@ class Timeout(Status):
>> pass
>>
>>
>> -class Skip(Status):
>> - name = 'skip'
>> - value = 60
>> - fraction = (0, 0)
>> -
>> - def __init__(self):
>> - pass
>> diff --git a/framework/summary.py b/framework/summary.py
>> index bbb423f..6ee1226 100644
>> --- a/framework/summary.py
>> +++ b/framework/summary.py
>> @@ -306,7 +306,7 @@ class Summary:
>>
>> # Problems include: warn, dmesg-warn, fail, dmesg-fail, and
>> crash. # Skip does not go on this page, it has the 'skipped' page -
>> if so.Skip() > max(status) > so.Pass():
>> + if max(status) > so.Pass():
>> self.tests['problems'].add(test)
>>
>> # Find all tests with a status of skip
>> @@ -317,9 +317,9 @@ class Summary:
>> for i in xrange(len(status) - 1):
>> first = status[i]
>> last = status[i + 1]
>> - if first < last and so.NotRun() not in (first, last):
>> + if max(first, so.Pass()) < last:
>> self.tests['regressions'].add(test)
>> - if first > last and so.NotRun() not in (first, last):
>> + if first > max(last, so.Pass()):
>> self.tests['fixes'].add(test)
>> # Changes cannot be added in the fixes and regressions
>> passes # becasue NotRun is a change, but not a regression or fix diff --git
>> a/framework/tests/summary.py b/framework/tests/summary.py index
>> de67c1d..b0905aa 100644
>> --- a/framework/tests/summary.py
>> +++ b/framework/tests/summary.py
>> @@ -38,37 +38,7 @@ from helpers import test_iterations, create_testresult,
>> create_test
>>
>> """ Status ordering from best to worst:
>>
>> -pass
>> -dmesg-warn
>> -warn
>> -dmesg-fail
>> -fail
>> -crash
>> -skip
>> -
>> -
>> -The following are regressions:
>> -
>> -pass|warn|dmesg-warn|fail|dmesg-fail|crash -> skip
>> -pass|warn|dmesg-warn|fail|dmesg-fail -> crash|skip
>> -pass|warn|dmesg-warn|fail -> dmesg-fail|crash|skip
>> -pass|warn|dmesg-warn -> fail|dmesg-fail|crash|skip
>> -pass|warn -> dmesg-warn|fail|dmesg-fail|crash|skip
>> -pass -> warn|dmesg-warn|fail|dmesg-fail|crash|skip
>> -
>> -
>> -The following are fixes:
>> -
>> -skip -> pass|warn|dmesg-warn|fail|dmesg-fail|crash
>> -crash|skip - >pass|warn|dmesg-warn|fail|dmesg-fail
>> -dmesg-fail|crash|skip -> pass|warn|dmesg-warn|fail
>> -fail|dmesg-fail|crash|skip -> pass|warn|dmesg-warn
>> -dmesg-warn|fail|dmesg-fail|crash|skip -> pass|warn
>> -warn|dmesg-warn|fail|dmesg-fail|crash|skip -> pass
>> -
>> -
>> -NotRun -> * and * -> NotRun is a change, but not a fix or a regression.
>> This is -because NotRun is not a status, but a representation of an unknown
>> status. +See ../summary.py.
>>
>> """
>>
>> @@ -85,47 +55,55 @@ REGRESSIONS = [("pass", "warn"),
>> ("pass", "dmesg-warn"),
>> ("pass", "fail"),
>> ("pass", "dmesg-fail"),
>> - ("pass", "skip"),
>> ("pass", "crash"),
>> ("dmesg-warn", "warn"),
>> ("dmesg-warn", "dmesg-fail"),
>> ("dmesg-warn", "fail"),
>> ("dmesg-warn", "crash"),
>> - ("dmesg-warn", "skip"),
>> ("warn", "fail"),
>> ("warn", "crash"),
>> - ("warn", "skip"),
>> ("warn", "dmesg-fail"),
>> ("dmesg-fail", "crash"),
>> - ("dmesg-fail", "skip"),
>> ("dmesg-fail", "fail"),
>> ("fail", "crash"),
>> - ("fail", "skip"),
>> - ("crash", "skip")]
>> + ("skip", "crash"),
>> + ("skip", "fail"),
>> + ("skip", "dmesg-fail"),
>> + ("skip", "warn"),
>> + ("skip", "dmesg-warn"),
>> + ("notrun", "crash"),
>> + ("notrun", "fail"),
>> + ("notrun", "dmesg-fail"),
>> + ("notrun", "warn"),
>> + ("notrun", "dmesg-warn")]
>>
>>
>> # List of possible fixes
>> -FIXES = [("skip", "crash"),
>> - ("skip", "fail"),
>> - ("skip", "dmesg-fail"),
>> - ("skip", "warn"),
>> - ("skip", "dmesg-warn"),
>> - ("skip", "pass"),
>> - ("crash", "fail"),
>> +FIXES = [("crash", "fail"),
>> ("crash", "dmesg-fail"),
>> ("crash", "warn"),
>> ("crash", "dmesg-warn"),
>> ("crash", "pass"),
>> + ("crash", "skip"),
>> + ("crash", "notrun"),
>> ("fail", "dmesg-fail"),
>> ("fail", "warn"),
>> ("fail", "dmesg-warn"),
>> ("fail", "pass"),
>> + ("fail", "skip"),
>> + ("fail", "notrun"),
>> ("dmesg-fail", "warn"),
>> ("dmesg-fail", "dmesg-warn"),
>> ("dmesg-fail", "pass"),
>> + ("dmesg-fail", "skip"),
>> + ("dmesg-fail", "notrun"),
>> ("warn", "dmesg-warn"),
>> ("warn", "pass"),
>> + ("warn", "skip"),
>> + ("warn", "notrun"),
>> ("dmesg-warn", "pass")]
>> + ("dmesg-warn", "skip")]
>> + ("dmesg-warn", "notrun")]
>>
>>
>> # List of statuses that should be problems.
>
> I finally have had a chance to look at this and have some concerns.
> I don't like your interpretation of skips as fixes and regressions, but I'm
> not strongly attached to them, so if other developers like that whatever.
It's the way I originally implemented the "fixes" and "regressions"
pages. Since when is "skip -> crash" a fix?
>
> However, I'm completely against "not run" being a fix or a regeression,
> becasue they are not. a test with that status litterly wasn't run. It doesn't
> have any status. IF it had been run it might have been the same, or it might
> have been different, It cannot be a fix or a regression.
Well, changes like "fail -> notrun" usually don't happen in my case,
because if I create a summary from multiple test runs, there are the
same sets of tests.
Now that I fixed it :), I don't have to look at the "changes" page
anymore, and it will be very useful for me to be able to see new tests
that fail in the regressions page. That was the whole point of it: not
having to look at the changes.
If people don't like this, I think we should add either new pages for
different usage cases or a command-line switch to select the desired
behavior.
Marek
More information about the Piglit
mailing list