[Piglit] [PATCH 1/2] summary: fix regression and fixes summaries
Marek Olšák
maraeo at gmail.com
Thu Nov 21 15:01:02 PST 2013
I once saw a crash status printed by piglit-run.py for a test with
subtests, but when I opened the html summary, the crashed test just
wasn't there. I think the subtests handling is broken for tests that
crash.
Marek
On Thu, Nov 21, 2013 at 6:22 PM, Tom Stellard <tom at stellard.net> wrote:
> On Wed, Nov 20, 2013 at 03:04:34PM -0800, Dylan Baker 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.
>>
>> 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.
>
> There is one use case that I run into a lot where it is useful to have
> "Not Run" show up on the changes pages. If a test with subtests crashes
> all of its subtests show up as "Not Run", so if you introduce a
> regressions that crashes a test with subtests, you will never find out about if
> "Not Run" status is not included on the changes page.
>
> -Tom
>
>
>> _______________________________________________
>> Piglit mailing list
>> Piglit at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/piglit
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
More information about the Piglit
mailing list