[Piglit] [PATCH 1/2] summary: fix regression and fixes summaries
Dylan Baker
baker.dylan.c at gmail.com
Wed Nov 20 16:38:23 PST 2013
On Thursday, November 21, 2013 12:30:53 AM Marek Olšák wrote:
> 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.'
This seems like a very common case for people bringing up new features
however; I wonder if Paul could weigh in with his experience in geometry
shaders
>
> 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.
Since you are changing the behavior of the tests by adding "not run" to fixes
and regressions, (I based those unit tests on the behavior of piglit at the
time, chaging only a couple of cases of inconsistant behavior.), could we
settling on adding a "not run" page, similiar to the skipped page, that
contains all of the tests with a not run status? That way there would be a
clear way to see what tests didn't run, without cluttering the fixes and
regressions pages with thousands of tests when comparing a small, focused set
of tests against a broader more comprehensive one?
Then can I go further to suggest that we put out an RFC on what statuses
should be fixes, regressions, changes, etc. write some unit tests for them and
call them authoratative?
>
> Marek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20131120/be9fb350/attachment.pgp>
More information about the Piglit
mailing list