[Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
In piglit when a test generates subtests we treat the "worst" subtest as the test status, and in most cases this works as expected. There is at least one case where this is not correct, and that is the case of crash. There are a couple of reasons that crash should not be masked. One is that it is generated by the framework when the test binary hits an assert or segfaults, or any number of similar cases. The second is that it may mean that all of the subtests did not run, as such we don't want the status to be masked by the "worst" subtest which would be fail at worst. Signed-off-by: Dylan Baker --- framework/results.py | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/framework/results.py b/framework/results.py index 469abeb..2095d90 100644 --- a/framework/results.py +++ b/framework/results.py @@ -173,10 +173,13 @@ class TestResult(object): """Return the result of the test. If there are subtests return the "worst" value of those subtests. If -there are not return the stored value of the test. +there are not return the stored value of the test. There is an +exception to this rule, and that's if the status is crash; since this +status is set by the framework, and can be generated even when some or +all unit tests pass. """ -if self.subtests: +if self.subtests and self.__result != status.CRASH: return max(six.itervalues(self.subtests)) return self.__result -- 2.8.2 ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
On 05/09/2016 03:46 PM, Dylan Baker wrote:
In piglit when a test generates subtests we treat the "worst" subtest as
the test status, and in most cases this works as expected. There is at
least one case where this is not correct, and that is the case of crash.
There are a couple of reasons that crash should not be masked. One is
that it is generated by the framework when the test binary hits an
assert or segfaults, or any number of similar cases. The second is that
it may mean that all of the subtests did not run, as such we don't want
the status to be masked by the "worst" subtest which would be fail at
worst.
Signed-off-by: Dylan Baker
---
framework/results.py | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/framework/results.py b/framework/results.py
index 469abeb..2095d90 100644
--- a/framework/results.py
+++ b/framework/results.py
@@ -173,10 +173,13 @@ class TestResult(object):
"""Return the result of the test.
If there are subtests return the "worst" value of those subtests. If
-there are not return the stored value of the test.
+there are not return the stored value of the test. There is an
+exception to this rule, and that's if the status is crash; since this
+status is set by the framework, and can be generated even when some or
+all unit tests pass.
"""
-if self.subtests:
+if self.subtests and self.__result != status.CRASH:
return max(six.itervalues(self.subtests))
return self.__result
This looks OK to me, and it fixes the incorrect "pass" result in the
json file.
Reviewed-by: Brian Paul
Tested-by: Brian Paul
However, when I run piglit-summary.py -s on the results file I get:
name:new
--
pass: 70
fail: 2
crash: 0
skip: 19
timeout: 0
warn: 0
incomplete: 0
dmesg-warn: 0
dmesg-fail: 0
changes: 0
fixes: 0
regressions: 0
total: 91
The "crash" result in the json file doesn't show up here. I believe
when there's subtests, we're counting the subtest results but not the
overall test result. Is there a way to make sure that the crash result
shows up in the summary?
-Brian
___
Piglit mailing list
Piglit@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
Shouldn't the test's result just be included into the "max" no matter what? On Mon, May 9, 2016 at 5:46 PM, Dylan Baker wrote: > In piglit when a test generates subtests we treat the "worst" subtest as > the test status, and in most cases this works as expected. There is at > least one case where this is not correct, and that is the case of crash. > > There are a couple of reasons that crash should not be masked. One is > that it is generated by the framework when the test binary hits an > assert or segfaults, or any number of similar cases. The second is that > it may mean that all of the subtests did not run, as such we don't want > the status to be masked by the "worst" subtest which would be fail at > worst. > > Signed-off-by: Dylan Baker > --- > > framework/results.py | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/framework/results.py b/framework/results.py > index 469abeb..2095d90 100644 > --- a/framework/results.py > +++ b/framework/results.py > @@ -173,10 +173,13 @@ class TestResult(object): > """Return the result of the test. > > If there are subtests return the "worst" value of those subtests. If > -there are not return the stored value of the test. > +there are not return the stored value of the test. There is an > +exception to this rule, and that's if the status is crash; since this > +status is set by the framework, and can be generated even when some > or > +all unit tests pass. > > """ > -if self.subtests: > +if self.subtests and self.__result != status.CRASH: > return max(six.itervalues(self.subtests)) > return self.__result > > -- > 2.8.2 > > ___ > Piglit mailing list > Piglit@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/piglit ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
Quoting Ilia Mirkin (2016-05-10 06:41:33) > Shouldn't the test's result just be included into the "max" no matter what? > > On Mon, May 9, 2016 at 5:46 PM, Dylan Baker wrote: > > In piglit when a test generates subtests we treat the "worst" subtest as > > the test status, and in most cases this works as expected. There is at > > least one case where this is not correct, and that is the case of crash. > > > > There are a couple of reasons that crash should not be masked. One is > > that it is generated by the framework when the test binary hits an > > assert or segfaults, or any number of similar cases. The second is that > > it may mean that all of the subtests did not run, as such we don't want > > the status to be masked by the "worst" subtest which would be fail at > > worst. > > > > Signed-off-by: Dylan Baker > > --- > > > > framework/results.py | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/framework/results.py b/framework/results.py > > index 469abeb..2095d90 100644 > > --- a/framework/results.py > > +++ b/framework/results.py > > @@ -173,10 +173,13 @@ class TestResult(object): > > """Return the result of the test. > > > > If there are subtests return the "worst" value of those subtests. > > If > > -there are not return the stored value of the test. > > +there are not return the stored value of the test. There is an > > +exception to this rule, and that's if the status is crash; since > > this > > +status is set by the framework, and can be generated even when > > some or > > +all unit tests pass. > > > > """ > > -if self.subtests: > > +if self.subtests and self.__result != status.CRASH: > > return max(six.itervalues(self.subtests)) > > return self.__result > > > > -- > > 2.8.2 > > > > ___ > > Piglit mailing list > > Piglit@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/piglit Well, the problem is that a lot of tests were retrofitted with subtests, and now the overall status is bunk, and for new tests we generally don't add an overall result when there's subtests. Dylan signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
On Tue, May 10, 2016 at 5:32 PM, Dylan Baker wrote: > Quoting Ilia Mirkin (2016-05-10 06:41:33) >> Shouldn't the test's result just be included into the "max" no matter what? >> > > Well, the problem is that a lot of tests were retrofitted with subtests, > and now the overall status is bunk, and for new tests we generally don't > add an overall result when there's subtests. It's surprising if the status of a test says fail but piglit-summary reports it as pass. In the case where one of the subtests fails but the overall status is pass, this would still say fail, no? What situation are you protecting against by only incorporating the overall status on crash? ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
Quoting Ilia Mirkin (2016-05-10 14:32:59) > On Tue, May 10, 2016 at 5:32 PM, Dylan Baker wrote: > > Quoting Ilia Mirkin (2016-05-10 06:41:33) > >> Shouldn't the test's result just be included into the "max" no matter what? > >> > > > > Well, the problem is that a lot of tests were retrofitted with subtests, > > and now the overall status is bunk, and for new tests we generally don't > > add an overall result when there's subtests. > > It's surprising if the status of a test says fail but piglit-summary > reports it as pass. In the case where one of the subtests fails but > the overall status is pass, this would still say fail, no? > > What situation are you protecting against by only incorporating the > overall status on crash? I have this gut feeling that says skip or notrun might play havok here. But I guess I should look test it and see if that's the case. The other thing I guess is that there's only about 100 tests (or, c/c++ files, really) using subtests. So maybe it would just be easier to audit them to make sure that they do the right thing and just add the overall result to the max call. signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
On Tue, May 10, 2016 at 8:05 PM, Dylan Baker wrote: > Quoting Ilia Mirkin (2016-05-10 14:32:59) >> On Tue, May 10, 2016 at 5:32 PM, Dylan Baker wrote: >> > Quoting Ilia Mirkin (2016-05-10 06:41:33) >> >> Shouldn't the test's result just be included into the "max" no matter >> >> what? >> >> >> > >> > Well, the problem is that a lot of tests were retrofitted with subtests, >> > and now the overall status is bunk, and for new tests we generally don't >> > add an overall result when there's subtests. >> >> It's surprising if the status of a test says fail but piglit-summary >> reports it as pass. In the case where one of the subtests fails but >> the overall status is pass, this would still say fail, no? >> >> What situation are you protecting against by only incorporating the >> overall status on crash? > > I have this gut feeling that says skip or notrun might play havok here. > But I guess I should look test it and see if that's the case. > > The other thing I guess is that there's only about 100 tests (or, c/c++ > files, really) using subtests. So maybe it would just be easier to audit > them to make sure that they do the right thing and just add the overall > result to the max call. I dunno... I think in at least some cases some "stuff" happens between subtests that can fail. I don't think it's correct to look at a test as a shell for a bunch of disconnected subtests, and thus anything the shell itself does is meaningless. The shell can also fail. Feel free to audit everything :) -ilia ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] framework/results: Fix subtests masking crashes
Quoting Ilia Mirkin (2016-05-10 17:45:49) > On Tue, May 10, 2016 at 8:05 PM, Dylan Baker wrote: > > Quoting Ilia Mirkin (2016-05-10 14:32:59) > >> On Tue, May 10, 2016 at 5:32 PM, Dylan Baker wrote: > >> > Quoting Ilia Mirkin (2016-05-10 06:41:33) > >> >> Shouldn't the test's result just be included into the "max" no matter > >> >> what? > >> >> > >> > > >> > Well, the problem is that a lot of tests were retrofitted with subtests, > >> > and now the overall status is bunk, and for new tests we generally don't > >> > add an overall result when there's subtests. > >> > >> It's surprising if the status of a test says fail but piglit-summary > >> reports it as pass. In the case where one of the subtests fails but > >> the overall status is pass, this would still say fail, no? > >> > >> What situation are you protecting against by only incorporating the > >> overall status on crash? > > > > I have this gut feeling that says skip or notrun might play havok here. > > But I guess I should look test it and see if that's the case. > > > > The other thing I guess is that there's only about 100 tests (or, c/c++ > > files, really) using subtests. So maybe it would just be easier to audit > > them to make sure that they do the right thing and just add the overall > > result to the max call. > > I dunno... I think in at least some cases some "stuff" happens between > subtests that can fail. I don't think it's correct to look at a test > as a shell for a bunch of disconnected subtests, and thus anything the > shell itself does is meaningless. The shell can also fail. Feel free > to audit everything :) > > -ilia You know. Thinking back there was a point where the framework defaulted to FAIL instead of NOTRUN. This might be an artifact of that time. Either way I guess just seeing what happens is the easiest way to figure out if it's going to work or not signature.asc Description: signature ___ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
